From 521ab91309d2a9bbad55969e4b3cc3e809c6277a Mon Sep 17 00:00:00 2001 From: extrawurst <776816+extrawurst@users.noreply.github.com> Date: Fri, 8 Dec 2023 14:33:22 +0100 Subject: [PATCH] git2-hooks: allows customizing what places to look for hooks (#1975) * allows customizing what places to look for hooks --- Cargo.lock | 2 +- asyncgit/Cargo.toml | 2 +- asyncgit/src/sync/hooks.rs | 17 ++- git2-hooks/Cargo.toml | 3 +- git2-hooks/README.md | 4 + git2-hooks/src/error.rs | 4 + git2-hooks/src/hookspath.rs | 107 +++++++++++---- git2-hooks/src/lib.rs | 262 +++++++++++++++++++++++++----------- 8 files changed, 288 insertions(+), 113 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ab434728..e78c049e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -714,7 +714,7 @@ dependencies = [ [[package]] name = "git2-hooks" -version = "0.2.0" +version = "0.3.0" dependencies = [ "git2", "git2-testing", diff --git a/asyncgit/Cargo.toml b/asyncgit/Cargo.toml index e6046872..ae3cdf70 100644 --- a/asyncgit/Cargo.toml +++ b/asyncgit/Cargo.toml @@ -17,7 +17,7 @@ crossbeam-channel = "0.5" easy-cast = "0.5" fuzzy-matcher = "0.3" git2 = "0.17" -git2-hooks = { path = "../git2-hooks", version = "0.2" } +git2-hooks = { path = "../git2-hooks", version = "0.3" } log = "0.4" # git2 = { path = "../../extern/git2-rs", features = ["vendored-openssl"]} # git2 = { git="https://github.com/extrawurst/git2-rs.git", rev="fc13dcc", features = ["vendored-openssl"]} diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index ab7bd44d..f480da07 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -14,10 +14,13 @@ pub enum HookResult { impl From for HookResult { fn from(v: git2_hooks::HookResult) -> Self { match v { - git2_hooks::HookResult::Ok => Self::Ok, - git2_hooks::HookResult::NotOk { stdout, stderr } => { - Self::NotOk(format!("{stdout}{stderr}")) - } + git2_hooks::HookResult::Ok { .. } + | git2_hooks::HookResult::NoHookFound => Self::Ok, + git2_hooks::HookResult::RunNotSuccessful { + stdout, + stderr, + .. + } => Self::NotOk(format!("{stdout}{stderr}")), } } } @@ -34,7 +37,7 @@ pub fn hooks_commit_msg( let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_commit_msg(&repo, msg)?.into()) + Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into()) } /// this hook is documented here @@ -44,7 +47,7 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_pre_commit(&repo)?.into()) + Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into()) } /// @@ -53,7 +56,7 @@ pub fn hooks_post_commit(repo_path: &RepoPath) -> Result { let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_post_commit(&repo)?.into()) + Ok(git2_hooks::hooks_post_commit(&repo, None)?.into()) } #[cfg(test)] diff --git a/git2-hooks/Cargo.toml b/git2-hooks/Cargo.toml index c51ef57b..c85f4cc9 100644 --- a/git2-hooks/Cargo.toml +++ b/git2-hooks/Cargo.toml @@ -1,11 +1,12 @@ [package] name = "git2-hooks" -version = "0.2.0" +version = "0.3.0" authors = ["extrawurst "] edition = "2021" description = "adds git hooks support based on git2-rs" homepage = "https://github.com/extrawurst/gitui" repository = "https://github.com/extrawurst/gitui" +documentation = "https://docs.rs/git2-hooks/" readme = "README.md" license = "MIT" categories = ["development-tools"] diff --git a/git2-hooks/README.md b/git2-hooks/README.md index 732a62a3..a3ce5fff 100644 --- a/git2-hooks/README.md +++ b/git2-hooks/README.md @@ -2,3 +2,7 @@ adds git hook functionality on top of git2-rs +## todo + +- [ ] unittest coverage symlinks from `.git/hooks/` -> `X` +- [ ] unittest coverage `~` expansion inside `core.hooksPath` \ No newline at end of file diff --git a/git2-hooks/src/error.rs b/git2-hooks/src/error.rs index e8bce462..e088dfd4 100644 --- a/git2-hooks/src/error.rs +++ b/git2-hooks/src/error.rs @@ -3,6 +3,10 @@ use thiserror::Error; /// #[derive(Error, Debug)] pub enum HooksError { + /// + #[error("git error:{0}")] + Git(#[from] git2::Error), + /// #[error("io error:{0}")] Io(#[from] std::io::Error), diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index 38f32c4e..ffd23e70 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -12,56 +12,106 @@ pub struct HookPaths { pub pwd: PathBuf, } +const CONFIG_HOOKS_PATH: &str = "core.hooksPath"; +const DEFAULT_HOOKS_PATH: &str = "hooks"; + impl HookPaths { - pub fn new(repo: &Repository, hook: &str) -> Result { + /// `core.hooksPath` always takes precendence. + /// If its defined and there is no hook `hook` this is not considered + /// an error or a reason to search in other paths. + /// If the config is not set we go into search mode and + /// first check standard `.git/hooks` folder and any sub path provided in `other_paths`. + /// + /// Note: we try to model as closely as possible what git shell is doing. + pub fn new( + repo: &Repository, + other_paths: Option<&[&str]>, + hook: &str, + ) -> Result { let pwd = repo .workdir() .unwrap_or_else(|| repo.path()) .to_path_buf(); let git_dir = repo.path().to_path_buf(); - let hooks_path = repo - .config() - .and_then(|config| config.get_string("core.hooksPath")) - .map_or_else( - |e| { - log::error!("hookspath error: {}", e); - repo.path().to_path_buf().join("hooks/") - }, - PathBuf::from, - ); - let hook = hooks_path.join(hook); + if let Some(config_path) = Self::config_hook_path(repo)? { + let hooks_path = PathBuf::from(config_path); - let hook = shellexpand::full( - hook.as_os_str() - .to_str() - .ok_or(HooksError::PathToString)?, - )?; + let hook = hooks_path.join(hook); - let hook = PathBuf::from_str(hook.as_ref()) - .map_err(|_| HooksError::PathToString)?; + let hook = shellexpand::full( + hook.as_os_str() + .to_str() + .ok_or(HooksError::PathToString)?, + )?; + + let hook = PathBuf::from_str(hook.as_ref()) + .map_err(|_| HooksError::PathToString)?; + + return Ok(Self { + git: git_dir, + hook, + pwd, + }); + } Ok(Self { git: git_dir, - hook, + hook: Self::find_hook(repo, other_paths, hook), pwd, }) } - pub fn is_executable(&self) -> bool { + fn config_hook_path(repo: &Repository) -> Result> { + Ok(repo.config()?.get_string(CONFIG_HOOKS_PATH).ok()) + } + + /// check default hook path first and then followed by `other_paths`. + /// if no hook is found we return the default hook path + fn find_hook( + repo: &Repository, + other_paths: Option<&[&str]>, + hook: &str, + ) -> PathBuf { + let mut paths = vec![DEFAULT_HOOKS_PATH.to_string()]; + if let Some(others) = other_paths { + paths.extend( + others + .iter() + .map(|p| p.trim_end_matches('/').to_string()), + ); + } + + for p in paths { + let p = repo.path().to_path_buf().join(p).join(hook); + if p.exists() { + return p; + } + } + + repo.path() + .to_path_buf() + .join(DEFAULT_HOOKS_PATH) + .join(hook) + } + + /// was a hook file found and is it executable + pub fn found(&self) -> bool { self.hook.exists() && is_executable(&self.hook) } /// this function calls hook scripts based on conventions documented here /// see pub fn run_hook(&self, args: &[&str]) -> Result { - let arg_str = format!("{:?} {}", self.hook, args.join(" ")); + let hook = self.hook.clone(); + + let arg_str = format!("{:?} {}", hook, args.join(" ")); // Use -l to avoid "command not found" on Windows. let bash_args = vec!["-l".to_string(), "-c".to_string(), arg_str]; - log::trace!("run hook '{:?}' in '{:?}'", self.hook, self.pwd); + log::trace!("run hook '{:?}' in '{:?}'", hook, self.pwd); let git_bash = find_bash_executable() .unwrap_or_else(|| PathBuf::from("bash")); @@ -78,19 +128,24 @@ impl HookPaths { .output()?; if output.status.success() { - Ok(HookResult::Ok) + Ok(HookResult::Ok { hook }) } else { let stderr = String::from_utf8_lossy(&output.stderr).to_string(); let stdout = String::from_utf8_lossy(&output.stdout).to_string(); - Ok(HookResult::NotOk { stdout, stderr }) + Ok(HookResult::RunNotSuccessful { + code: output.status.code(), + stdout, + stderr, + hook, + }) } } } -#[cfg(not(windows))] +#[cfg(unix)] fn is_executable(path: &Path) -> bool { use std::os::unix::fs::PermissionsExt; diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 4cf4c0bc..0cf5ac8d 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -1,6 +1,11 @@ //! git2-rs addon supporting git hooks //! -//! most basic hook is: [`hooks_pre_commit`]. see also other `hooks_*` functions +//! we look for hooks in the following locations: +//! * whatever `config.hooksPath` points to +//! * `.git/hooks/` +//! * whatever list of paths provided as `other_paths` (in order) +//! +//! most basic hook is: [`hooks_pre_commit`]. see also other `hooks_*` functions. //! //! [`create_hook`] is useful to create git hooks from code (unittest make heavy usage of it) mod error; @@ -28,10 +33,36 @@ const HOOK_COMMIT_MSG_TEMP_FILE: &str = "COMMIT_EDITMSG"; /// #[derive(Debug, PartialEq, Eq)] pub enum HookResult { - /// Everything went fine - Ok, - /// Hook returned error - NotOk { stdout: String, stderr: String }, + /// No hook found + NoHookFound, + /// Hook executed with non error return code + Ok { + /// path of the hook that was run + hook: PathBuf, + }, + /// Hook executed and returned an error code + RunNotSuccessful { + /// exit code as reported back from process calling the hook + code: Option, + /// stderr output emitted by hook + stdout: String, + /// stderr output emitted by hook + stderr: String, + /// path of the hook that was run + hook: PathBuf, + }, +} + +impl HookResult { + /// helper to check if result is ok + pub fn is_ok(&self) -> bool { + matches!(self, HookResult::Ok { .. }) + } + + /// helper to check if result was run and not rejected + pub fn is_not_successful(&self) -> bool { + matches!(self, HookResult::RunNotSuccessful { .. }) + } } /// helper method to create git hooks programmatically (heavy used in unittests) @@ -40,7 +71,7 @@ pub fn create_hook( hook: &str, hook_script: &[u8], ) -> PathBuf { - let hook = HookPaths::new(r, hook).unwrap(); + let hook = HookPaths::new(r, None, hook).unwrap(); let path = hook.hook.clone(); @@ -52,7 +83,7 @@ pub fn create_hook( fn create_hook_in_path(path: &Path, hook_script: &[u8]) { File::create(path).unwrap().write_all(hook_script).unwrap(); - #[cfg(not(windows))] + #[cfg(unix)] { Command::new("chmod") .arg("+x") @@ -69,50 +100,56 @@ fn create_hook_in_path(path: &Path, hook_script: &[u8]) { /// parameter to the hook script. pub fn hooks_commit_msg( repo: &Repository, + other_paths: Option<&[&str]>, msg: &mut String, ) -> Result { - let hooks_path = HookPaths::new(repo, HOOK_COMMIT_MSG)?; + let hook = HookPaths::new(repo, other_paths, HOOK_COMMIT_MSG)?; - if hooks_path.is_executable() { - let temp_file = - hooks_path.git.join(HOOK_COMMIT_MSG_TEMP_FILE); - File::create(&temp_file)?.write_all(msg.as_bytes())?; - - let res = hooks_path.run_hook(&[temp_file - .as_os_str() - .to_string_lossy() - .as_ref()])?; - - // load possibly altered msg - msg.clear(); - File::open(temp_file)?.read_to_string(msg)?; - - Ok(res) - } else { - Ok(HookResult::Ok) + if !hook.found() { + return Ok(HookResult::NoHookFound); } + + let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE); + File::create(&temp_file)?.write_all(msg.as_bytes())?; + + let res = hook.run_hook(&[temp_file + .as_os_str() + .to_string_lossy() + .as_ref()])?; + + // load possibly altered msg + msg.clear(); + File::open(temp_file)?.read_to_string(msg)?; + + Ok(res) } /// this hook is documented here -pub fn hooks_pre_commit(repo: &Repository) -> Result { - let hook = HookPaths::new(repo, HOOK_PRE_COMMIT)?; +pub fn hooks_pre_commit( + repo: &Repository, + other_paths: Option<&[&str]>, +) -> Result { + let hook = HookPaths::new(repo, other_paths, HOOK_PRE_COMMIT)?; - if hook.is_executable() { - Ok(hook.run_hook(&[])?) - } else { - Ok(HookResult::Ok) + if !hook.found() { + return Ok(HookResult::NoHookFound); } + + hook.run_hook(&[]) } /// this hook is documented here -pub fn hooks_post_commit(repo: &Repository) -> Result { - let hook = HookPaths::new(repo, HOOK_POST_COMMIT)?; +pub fn hooks_post_commit( + repo: &Repository, + other_paths: Option<&[&str]>, +) -> Result { + let hook = HookPaths::new(repo, other_paths, HOOK_POST_COMMIT)?; - if hook.is_executable() { - Ok(hook.run_hook(&[])?) - } else { - Ok(HookResult::Ok) + if !hook.found() { + return Ok(HookResult::NoHookFound); } + + hook.run_hook(&[]) } #[cfg(test)] @@ -127,13 +164,19 @@ mod tests { let (_td, repo) = repo_init(); let mut msg = String::from("test"); - let res = hooks_commit_msg(&repo, &mut msg).unwrap(); + let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - assert_eq!(res, HookResult::Ok); + assert_eq!(res, HookResult::NoHookFound); - let res = hooks_post_commit(&repo).unwrap(); + let hook = b"#!/bin/sh +exit 0 + "; - assert_eq!(res, HookResult::Ok); + create_hook(&repo, HOOK_POST_COMMIT, hook); + + let res = hooks_post_commit(&repo, None).unwrap(); + + assert!(res.is_ok()); } #[test] @@ -147,9 +190,9 @@ exit 0 create_hook(&repo, HOOK_COMMIT_MSG, hook); let mut msg = String::from("test"); - let res = hooks_commit_msg(&repo, &mut msg).unwrap(); + let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - assert_eq!(res, HookResult::Ok); + assert!(res.is_ok()); assert_eq!(msg, String::from("test")); } @@ -167,9 +210,9 @@ exit 0 create_hook(&repo, HOOK_COMMIT_MSG, hook); let mut msg = String::from("test_sth"); - let res = hooks_commit_msg(&repo, &mut msg).unwrap(); + let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - assert_eq!(res, HookResult::Ok); + assert!(res.is_ok()); assert_eq!(msg, String::from("test_shell_command")); } @@ -183,8 +226,71 @@ exit 0 "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); - assert_eq!(res, HookResult::Ok); + let res = hooks_pre_commit(&repo, None).unwrap(); + assert!(res.is_ok()); + } + + #[test] + fn test_no_hook_found() { + let (_td, repo) = repo_init(); + + let res = hooks_pre_commit(&repo, None).unwrap(); + assert_eq!(res, HookResult::NoHookFound); + } + + #[test] + fn test_other_path() { + let (td, repo) = repo_init(); + + let hook = b"#!/bin/sh +exit 0 + "; + + let custom_hooks_path = td.path().join(".myhooks"); + + std::fs::create_dir(dbg!(&custom_hooks_path)).unwrap(); + create_hook_in_path( + dbg!(custom_hooks_path.join(HOOK_PRE_COMMIT).as_path()), + hook, + ); + + let res = + hooks_pre_commit(&repo, Some(&["../.myhooks"])).unwrap(); + + assert!(res.is_ok()); + } + + #[test] + fn test_other_path_precendence() { + let (td, repo) = repo_init(); + + { + let hook = b"#!/bin/sh +exit 0 + "; + + create_hook(&repo, HOOK_PRE_COMMIT, hook); + } + + { + let reject_hook = b"#!/bin/sh +exit 1 + "; + + let custom_hooks_path = td.path().join(".myhooks"); + std::fs::create_dir(dbg!(&custom_hooks_path)).unwrap(); + create_hook_in_path( + dbg!(custom_hooks_path + .join(HOOK_PRE_COMMIT) + .as_path()), + reject_hook, + ); + } + + let res = + hooks_pre_commit(&repo, Some(&["../.myhooks"])).unwrap(); + + assert!(res.is_ok()); } #[test] @@ -197,8 +303,8 @@ exit 1 "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); - assert!(res != HookResult::Ok); + let res = hooks_pre_commit(&repo, None).unwrap(); + assert!(res.is_not_successful()); } #[test] @@ -211,9 +317,9 @@ exit 1 "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); + let res = hooks_pre_commit(&repo, None).unwrap(); - let HookResult::NotOk { stdout, .. } = res else { + let HookResult::RunNotSuccessful { stdout, .. } = res else { unreachable!() }; @@ -242,15 +348,15 @@ exit 1 ) .unwrap(); - let res = hooks_pre_commit(&repo).unwrap(); + let res = hooks_pre_commit(&repo, None).unwrap(); - assert_eq!( - res, - HookResult::NotOk { - stdout: String::from("rejected\n"), - stderr: String::new() - } - ); + let HookResult::RunNotSuccessful { code, stdout, .. } = res + else { + unreachable!() + }; + + assert_eq!(code.unwrap(), 1); + assert_eq!(&stdout, "rejected\n"); } #[test] @@ -263,8 +369,8 @@ exit 1 "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); - assert!(res != HookResult::Ok); + let res = hooks_pre_commit(&repo, None).unwrap(); + assert!(res.is_not_successful()); } #[test] @@ -284,8 +390,8 @@ sys.exit(0) "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); - assert_eq!(res, HookResult::Ok); + let res = hooks_pre_commit(&repo, None).unwrap(); + assert!(res.is_ok()); } #[test] @@ -305,8 +411,8 @@ sys.exit(1) "; create_hook(&repo, HOOK_PRE_COMMIT, hook); - let res = hooks_pre_commit(&repo).unwrap(); - assert!(res != HookResult::Ok); + let res = hooks_pre_commit(&repo, None).unwrap(); + assert!(res.is_not_successful()); } #[test] @@ -322,15 +428,15 @@ exit 1 create_hook(&repo, HOOK_COMMIT_MSG, hook); let mut msg = String::from("test"); - let res = hooks_commit_msg(&repo, &mut msg).unwrap(); + let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - assert_eq!( - res, - HookResult::NotOk { - stdout: String::from("rejected\n"), - stderr: String::new() - } - ); + let HookResult::RunNotSuccessful { code, stdout, .. } = res + else { + unreachable!() + }; + + assert_eq!(code.unwrap(), 1); + assert_eq!(&stdout, "rejected\n"); assert_eq!(msg, String::from("msg\n")); } @@ -347,9 +453,9 @@ exit 0 create_hook(&repo, HOOK_COMMIT_MSG, hook); let mut msg = String::from("test"); - let res = hooks_commit_msg(&repo, &mut msg).unwrap(); + let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - assert_eq!(res, HookResult::Ok); + assert!(res.is_ok()); assert_eq!(msg, String::from("msg\n")); } @@ -358,7 +464,8 @@ exit 0 let (_td, repo) = repo_init_bare(); let git_root = repo.path().to_path_buf(); - let hook = HookPaths::new(&repo, HOOK_POST_COMMIT).unwrap(); + let hook = + HookPaths::new(&repo, None, HOOK_POST_COMMIT).unwrap(); assert_eq!(hook.pwd, git_root); } @@ -368,7 +475,8 @@ exit 0 let (_td, repo) = repo_init(); let git_root = repo.path().to_path_buf(); - let hook = HookPaths::new(&repo, HOOK_POST_COMMIT).unwrap(); + let hook = + HookPaths::new(&repo, None, HOOK_POST_COMMIT).unwrap(); assert_eq!(hook.pwd, git_root.parent().unwrap()); }