From d26acb79379a93498a76aa8f58e18f93cd58d666 Mon Sep 17 00:00:00 2001 From: Hermes Agent Date: Thu, 25 Jun 2026 06:15:20 +0000 Subject: [PATCH] refactor(cli): share diagnostics hook inspection --- crates/git-smee-cli/src/diagnostics.rs | 224 +++++++++++++++++++++++++ crates/git-smee-cli/src/doctor.rs | 91 ++++------ crates/git-smee-cli/src/main.rs | 1 + crates/git-smee-cli/src/status.rs | 166 ++++++------------ 4 files changed, 309 insertions(+), 173 deletions(-) create mode 100644 crates/git-smee-cli/src/diagnostics.rs diff --git a/crates/git-smee-cli/src/diagnostics.rs b/crates/git-smee-cli/src/diagnostics.rs new file mode 100644 index 0000000..dff6e47 --- /dev/null +++ b/crates/git-smee-cli/src/diagnostics.rs @@ -0,0 +1,224 @@ +use std::{ + env, fs, + path::{Path, PathBuf}, +}; + +use git_smee_core::{config::LifeCyclePhase, installer}; + +use crate::config_path::normalize_config_path_for_hook_script; + +pub(crate) struct ExpectedHookScript { + config_path: String, + executable_path: Option, +} + +impl ExpectedHookScript { + pub(crate) fn from_current_process(config_path: &Path, repository_root: &Path) -> Self { + let normalized_config_path = + normalize_config_path_for_hook_script(config_path, repository_root) + .unwrap_or_else(|_| config_path.to_path_buf()); + Self { + config_path: normalized_config_path.to_string_lossy().to_string(), + executable_path: env::current_exe() + .ok() + .map(|path| path.to_string_lossy().to_string()), + } + } + + pub(crate) fn stale_reasons(&self, hook_content: &str) -> Vec { + let mut reasons = Vec::new(); + if !hook_content.contains(&self.config_path) { + reasons.push(format!("expected config path {}", self.config_path)); + } + match &self.executable_path { + Some(expected_exe) if !hook_content.contains(expected_exe) => { + reasons.push(format!("expected executable {expected_exe}")); + } + _ => {} + } + reasons + } +} + +#[derive(Debug)] +pub(crate) struct HookInspection { + phase: LifeCyclePhase, + path: PathBuf, + display_path: String, + state: HookInspectionState, +} + +impl HookInspection { + pub(crate) fn phase(&self) -> LifeCyclePhase { + self.phase + } + + pub(crate) fn path(&self) -> &Path { + &self.path + } + + pub(crate) fn display_path(&self) -> &str { + &self.display_path + } + + pub(crate) fn state(&self) -> &HookInspectionState { + &self.state + } +} + +#[derive(Debug)] +pub(crate) enum HookInspectionState { + Missing, + InvalidPath, + Unmanaged, + Managed { content: String }, + Unreadable { error: String }, +} + +pub(crate) fn inspect_hook( + repository_root: &Path, + hooks_dir: &Path, + phase: LifeCyclePhase, +) -> HookInspection { + let path = hooks_dir.join(phase.as_str()); + let display_path = display_repo_path(repository_root, &path); + let state = if !path.exists() { + HookInspectionState::Missing + } else if !path.is_file() { + HookInspectionState::InvalidPath + } else { + match installer::has_managed_header(&path) { + Ok(false) => HookInspectionState::Unmanaged, + Ok(true) => match fs::read_to_string(&path) { + Ok(content) => HookInspectionState::Managed { content }, + Err(error) => HookInspectionState::Unreadable { + error: error.to_string(), + }, + }, + Err(error) => HookInspectionState::Unreadable { + error: error.to_string(), + }, + } + }; + + HookInspection { + phase, + path, + display_path, + state, + } +} + +pub(crate) fn inspect_obsolete_managed_hooks( + repository_root: &Path, + hooks_dir: &Path, + configured_phases: &[LifeCyclePhase], +) -> Vec { + LifeCyclePhase::all() + .iter() + .copied() + .filter(|phase| !configured_phases.contains(phase)) + .filter_map(|phase| { + let inspection = inspect_hook(repository_root, hooks_dir, phase); + matches!(inspection.state(), HookInspectionState::Managed { .. }).then_some(inspection) + }) + .collect() +} + +pub(crate) fn display_repo_path(repository_root: &Path, path: &Path) -> String { + path.strip_prefix(repository_root) + .unwrap_or(path) + .display() + .to_string() + .replace('\\', "/") +} + +#[cfg(test)] +mod tests { + use std::fs; + + use git_smee_core::installer; + + use super::*; + + #[test] + fn display_repo_path_uses_forward_slashes_for_repo_relative_paths() { + assert_eq!( + display_repo_path(Path::new("/repo"), Path::new("/repo/.git/hooks/pre-commit")), + ".git/hooks/pre-commit" + ); + } + + #[test] + fn display_repo_path_keeps_external_paths_visible() { + assert_eq!( + display_repo_path(Path::new("/repo"), Path::new("/tmp/hooks/pre-commit")), + "/tmp/hooks/pre-commit" + ); + } + + #[test] + fn inspect_hook_treats_marker_only_in_body_as_unmanaged() { + let temp_dir = tempfile::tempdir().expect("failed to create tempdir"); + let repository_root = temp_dir.path(); + let hooks_dir = repository_root.join(".git/hooks"); + fs::create_dir_all(&hooks_dir).expect("failed to create hooks dir"); + fs::write( + hooks_dir.join("pre-commit"), + format!("#!/bin/sh\necho '{}'\n", installer::MANAGED_FILE_MARKER), + ) + .expect("failed to write hook"); + + let inspection = inspect_hook(repository_root, &hooks_dir, LifeCyclePhase::PreCommit); + + assert!(matches!(inspection.state(), HookInspectionState::Unmanaged)); + } + + #[test] + fn stale_reasons_report_missing_config_and_executable() { + let expected = ExpectedHookScript { + config_path: ".git-smee.toml".to_string(), + executable_path: Some("/bin/git-smee".to_string()), + }; + + assert_eq!( + expected.stale_reasons("#!/bin/sh\n"), + vec![ + "expected config path .git-smee.toml".to_string(), + "expected executable /bin/git-smee".to_string(), + ] + ); + } + + #[test] + fn obsolete_managed_hook_inspection_skips_configured_phases() { + let temp_dir = tempfile::tempdir().expect("failed to create tempdir"); + let repository_root = temp_dir.path(); + let hooks_dir = repository_root.join(".git/hooks"); + fs::create_dir_all(&hooks_dir).expect("failed to create hooks dir"); + fs::write( + hooks_dir.join("pre-commit"), + installer::with_managed_header("#!/bin/sh\n"), + ) + .expect("failed to write managed hook"); + fs::write( + hooks_dir.join("pre-push"), + installer::with_managed_header("#!/bin/sh\n"), + ) + .expect("failed to write managed hook"); + + let inspections = inspect_obsolete_managed_hooks( + repository_root, + &hooks_dir, + &[LifeCyclePhase::PreCommit], + ); + + assert_eq!( + inspections + .iter() + .map(|inspection| inspection.phase().as_str()) + .collect::>(), + vec!["pre-push"] + ); + } +} diff --git a/crates/git-smee-cli/src/doctor.rs b/crates/git-smee-cli/src/doctor.rs index 6c68212..c7d9027 100644 --- a/crates/git-smee-cli/src/doctor.rs +++ b/crates/git-smee-cli/src/doctor.rs @@ -1,9 +1,12 @@ -use std::{env, fs, path::Path}; +use std::path::Path; use git_smee_core::{installer, repository}; use serde::Serialize; -use crate::config_path::{normalize_config_path_for_hook_script, read_config_file}; +use crate::{ + config_path::read_config_file, + diagnostics::{ExpectedHookScript, HookInspectionState, inspect_hook}, +}; #[derive(Debug, Serialize)] struct DoctorReport { @@ -121,71 +124,39 @@ fn build_doctor_report(config_path: &Path) -> DoctorReport { } }; - let expected_config_path = normalize_config_path_for_hook_script(config_path, &repository_root) - .unwrap_or_else(|_| config_path.to_path_buf()); - let expected_exe = env::current_exe().ok(); - let expected_config = expected_config_path.to_string_lossy().to_string(); + let expected_hook_script = + ExpectedHookScript::from_current_process(config_path, &repository_root); let mut phases: Vec<_> = config.hooks.keys().copied().collect(); phases.sort_by_key(|phase| phase.as_str()); for phase in phases { - let hook_path = hooks_dir.join(phase.as_str()); - if !hook_path.exists() { - report.errors.push(format!( + let inspection = inspect_hook(&repository_root, &hooks_dir, phase); + match inspection.state() { + HookInspectionState::Missing => report.errors.push(format!( "missing managed wrapper for {phase} at {}; run git smee install", - hook_path.display() - )); - continue; - } - if !hook_path.is_file() { - report.errors.push(format!( + inspection.path().display() + )), + HookInspectionState::InvalidPath => report.errors.push(format!( "hook path for {phase} is not a regular file: {}; remove it or fix core.hooksPath", - hook_path.display() - )); - continue; - } - let is_managed = match installer::has_managed_header(&hook_path) { - Ok(is_managed) => is_managed, - Err(error) => { - report.errors.push(format!( - "cannot read hook wrapper for {phase} at {}: {error}", - hook_path.display() - )); - continue; - } - }; - if !is_managed { - report.errors.push(format!( + inspection.path().display() + )), + HookInspectionState::Unmanaged => report.errors.push(format!( "unmanaged hook file blocks install for {phase} at {}; move it aside or run git smee install --force", - hook_path.display() - )); - continue; - } - let content = match fs::read_to_string(&hook_path) { - Ok(content) => content, - Err(error) => { - report.errors.push(format!( - "cannot read hook wrapper for {phase} at {}: {error}", - hook_path.display() - )); - continue; - } - }; - report - .ok - .push(format!("managed wrapper is installed for {phase}")); - if !content.contains(&expected_config) { - report.warnings.push(format!( - "stale managed wrapper for {phase}: expected config path {}; run git smee install", - expected_config - )); - } - if let Some(expected_exe) = &expected_exe { - let expected_exe = expected_exe.to_string_lossy().to_string(); - if !content.contains(&expected_exe) { - report.warnings.push(format!( - "stale managed wrapper for {phase}: expected executable {expected_exe}; run git smee install" - )); + inspection.path().display() + )), + HookInspectionState::Unreadable { error } => report.errors.push(format!( + "cannot read hook wrapper for {phase} at {}: {error}", + inspection.path().display() + )), + HookInspectionState::Managed { content } => { + report + .ok + .push(format!("managed wrapper is installed for {phase}")); + for stale_reason in expected_hook_script.stale_reasons(content) { + report.warnings.push(format!( + "stale managed wrapper for {phase}: {stale_reason}; run git smee install" + )); + } } } } diff --git a/crates/git-smee-cli/src/main.rs b/crates/git-smee-cli/src/main.rs index c986d49..df75165 100644 --- a/crates/git-smee-cli/src/main.rs +++ b/crates/git-smee-cli/src/main.rs @@ -14,6 +14,7 @@ use git_smee_core::{ }; mod config_path; +mod diagnostics; mod doctor; mod status; diff --git a/crates/git-smee-cli/src/status.rs b/crates/git-smee-cli/src/status.rs index f3691f1..62ebbcd 100644 --- a/crates/git-smee-cli/src/status.rs +++ b/crates/git-smee-cli/src/status.rs @@ -1,9 +1,14 @@ -use std::{env, fs, path::Path}; +use std::path::Path; -use git_smee_core::{config::LifeCyclePhase, installer, repository}; +use git_smee_core::repository; use serde::Serialize; -use crate::config_path::{normalize_config_path_for_hook_script, read_config_file}; +use crate::{ + config_path::read_config_file, + diagnostics::{ + ExpectedHookScript, HookInspectionState, inspect_hook, inspect_obsolete_managed_hooks, + }, +}; #[derive(Debug, Serialize)] struct StatusReport { @@ -64,15 +69,13 @@ pub(crate) fn run_status(config_path: &Path, json: bool) -> Result<(), Box Result> { let repository_root = repository::find_git_root()?; - let hooks_dir = repository::resolve_git_path( + let hooks_dir = git_smee_core::repository::resolve_git_path( &repository_root, - installer::FileSystemHookInstaller::HOOKS_GIT_PATH_KEY, + git_smee_core::installer::FileSystemHookInstaller::HOOKS_GIT_PATH_KEY, )?; let config = read_config_file(config_path)?; - let expected_config_path = normalize_config_path_for_hook_script(config_path, &repository_root) - .unwrap_or_else(|_| config_path.to_path_buf()); - let expected_exe = env::current_exe().ok(); - let expected_config = expected_config_path.to_string_lossy().to_string(); + let expected_hook_script = + ExpectedHookScript::from_current_process(config_path, &repository_root); let mut phases: Vec<_> = config.hooks.keys().copied().collect(); phases.sort_by_key(|phase| phase.as_str()); @@ -80,67 +83,46 @@ fn build_status_report(config_path: &Path) -> Result ( HookState::Missing, Some(format!("run git smee install to create {phase}")), - ) - } else if !hook_path.is_file() { - ( + ), + HookInspectionState::InvalidPath => ( HookState::InvalidPath, Some(format!( "remove {} or fix core.hooksPath before reinstalling", - display_repo_path(&repository_root, &hook_path) + inspection.display_path() )), - ) - } else { - match installer::has_managed_header(&hook_path) { - Ok(false) => ( - HookState::Unmanaged, - Some(format!( - "move {} aside or run git smee install --force", - display_repo_path(&repository_root, &hook_path) - )), - ), - Ok(true) => match fs::read_to_string(&hook_path) { - Ok(content) => { - if !content.contains(&expected_config) { - stale_reasons.push(format!("expected config path {expected_config}")); - } - if let Some(expected_exe) = &expected_exe { - let expected_exe = expected_exe.to_string_lossy().to_string(); - if !content.contains(&expected_exe) { - stale_reasons.push(format!("expected executable {expected_exe}")); - } - } - if stale_reasons.is_empty() { - (HookState::Installed, None) - } else { - ( - HookState::Stale, - Some(format!("run git smee install to refresh {phase}")), - ) - } - } - Err(error) => ( - HookState::Unreadable, - Some(format!( - "fix permissions for {} ({error})", - display_repo_path(&repository_root, &hook_path) - )), - ), - }, - Err(error) => ( - HookState::Unreadable, - Some(format!( - "fix permissions for {} ({error})", - display_repo_path(&repository_root, &hook_path) - )), - ), + ), + HookInspectionState::Unmanaged => ( + HookState::Unmanaged, + Some(format!( + "move {} aside or run git smee install --force", + inspection.display_path() + )), + ), + HookInspectionState::Managed { content } => { + stale_reasons = expected_hook_script.stale_reasons(content); + if stale_reasons.is_empty() { + (HookState::Installed, None) + } else { + ( + HookState::Stale, + Some(format!("run git smee install to refresh {phase}")), + ) + } } + HookInspectionState::Unreadable { error } => ( + HookState::Unreadable, + Some(format!( + "fix permissions for {} ({error})", + inspection.display_path() + )), + ), }; if let Some(action) = &next_action { @@ -150,35 +132,22 @@ fn build_status_report(config_path: &Path) -> Result = phases.iter().map(|phase| phase.as_str()).collect(); let mut obsolete_managed_hooks = Vec::new(); - for phase in LifeCyclePhase::all() { - if configured_phase_names.contains(&phase.as_str()) { - continue; - } - let hook_path = hooks_dir.join(phase.as_str()); - if !hook_path.is_file() { - continue; - } - let Ok(is_managed) = installer::has_managed_header(&hook_path) else { - continue; - }; - if is_managed { - let path = display_repo_path(&repository_root, &hook_path); - let next_action = format!("remove obsolete managed hook {path}"); - next_actions.push(next_action.clone()); - obsolete_managed_hooks.push(ObsoleteHookStatus { - phase: phase.to_string(), - path, - next_action, - }); - } + for inspection in inspect_obsolete_managed_hooks(&repository_root, &hooks_dir, &phases) { + let path = inspection.display_path().to_string(); + let next_action = format!("remove obsolete managed hook {path}"); + next_actions.push(next_action.clone()); + obsolete_managed_hooks.push(ObsoleteHookStatus { + phase: inspection.phase().to_string(), + path, + next_action, + }); } next_actions.sort(); @@ -273,32 +242,3 @@ fn print_status_section(name: &str, items: &[String]) { } } } - -fn display_repo_path(repository_root: &Path, path: &Path) -> String { - path.strip_prefix(repository_root) - .unwrap_or(path) - .display() - .to_string() - .replace('\\', "/") -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn display_repo_path_uses_forward_slashes_for_repo_relative_paths() { - assert_eq!( - display_repo_path(Path::new("/repo"), Path::new("/repo/.git/hooks/pre-commit")), - ".git/hooks/pre-commit" - ); - } - - #[test] - fn display_repo_path_keeps_external_paths_visible() { - assert_eq!( - display_repo_path(Path::new("/repo"), Path::new("/tmp/hooks/pre-commit")), - "/tmp/hooks/pre-commit" - ); - } -}