git2-hooks: allows customizing what places to look for hooks (#1975)

* allows customizing what places to look for hooks

Cargo.lock

@@ -714,7 +714,7 @@ dependencies = [
 
 [[package]]
 name = "git2-hooks"
-version = "0.2.0"
+version = "0.3.0"
 dependencies = [
  "git2",
  "git2-testing",

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"]}

asyncgit/src/sync/hooks.rs

@@ -14,10 +14,13 @@ pub enum HookResult {
 impl From<git2_hooks::HookResult> for HookResult {
 	fn from(v: git2_hooks::HookResult) -> Self {
 		match v {
-			git2_hooks::HookResult::Ok => Self::Ok,
+			git2_hooks::HookResult::Ok { .. }
-			git2_hooks::HookResult::NotOk { stdout, stderr } => {
+			| git2_hooks::HookResult::NoHookFound => Self::Ok,
-				Self::NotOk(format!("{stdout}{stderr}"))
+			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 <https://git-scm.com/docs/githooks#_pre_commit>
@@ -44,7 +47,7 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
 
 	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<HookResult> {
 
 	let repo = repo(repo_path)?;
 
-	Ok(git2_hooks::hooks_post_commit(&repo)?.into())
+	Ok(git2_hooks::hooks_post_commit(&repo, None)?.into())
 }
 
 #[cfg(test)]

git2-hooks/Cargo.toml

@@ -1,11 +1,12 @@
 [package]
 name = "git2-hooks"
-version = "0.2.0"
+version = "0.3.0"
 authors = ["extrawurst <mail@rusticorn.com>"]
 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"]

git2-hooks/README.md

@@ -2,3 +2,7 @@
 
 adds git hook functionality on top of git2-rs
 
+## todo
+
+- [ ] unittest coverage symlinks from `.git/hooks/<hook>` -> `X`
+- [ ] unittest coverage `~` expansion inside `core.hooksPath`

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),

git2-hooks/src/hookspath.rs

@@ -12,24 +12,31 @@ 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<Self> {
+	/// `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<Self> {
 		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()
+		if let Some(config_path) = Self::config_hook_path(repo)? {
-			.and_then(|config| config.get_string("core.hooksPath"))
+			let hooks_path = PathBuf::from(config_path);
-			.map_or_else(
-				|e| {
-					log::error!("hookspath error: {}", e);
-					repo.path().to_path_buf().join("hooks/")
-				},
-				PathBuf::from,
-			);
 
 			let hook = hooks_path.join(hook);
 
@@ -42,26 +49,69 @@ impl HookPaths {
 			let hook = PathBuf::from_str(hook.as_ref())
 				.map_err(|_| HooksError::PathToString)?;
 
-		Ok(Self {
+			return Ok(Self {
 				git: git_dir,
 				hook,
 				pwd,
+			});
+		}
+
+		Ok(Self {
+			git: git_dir,
+			hook: Self::find_hook(repo, other_paths, hook),
+			pwd,
 		})
 	}
 
-	pub fn is_executable(&self) -> bool {
+	fn config_hook_path(repo: &Repository) -> Result<Option<String>> {
+		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 <https://git-scm.com/docs/githooks>
 	pub fn run_hook(&self, args: &[&str]) -> Result<HookResult> {
-		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;
 

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
+	/// No hook found
-	Ok,
+	NoHookFound,
-	/// Hook returned error
+	/// Hook executed with non error return code
-	NotOk { stdout: String, stderr: String },
+	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<i32>,
+		/// 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,16 +100,19 @@ 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<HookResult> {
-	let hooks_path = HookPaths::new(repo, HOOK_COMMIT_MSG)?;
+	let hook = HookPaths::new(repo, other_paths, HOOK_COMMIT_MSG)?;
 
-	if hooks_path.is_executable() {
+	if !hook.found() {
-		let temp_file =
+		return Ok(HookResult::NoHookFound);
-			hooks_path.git.join(HOOK_COMMIT_MSG_TEMP_FILE);
+	}
+
+	let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE);
 	File::create(&temp_file)?.write_all(msg.as_bytes())?;
 
-		let res = hooks_path.run_hook(&[temp_file
+	let res = hook.run_hook(&[temp_file
 		.as_os_str()
 		.to_string_lossy()
 		.as_ref()])?;
@@ -88,31 +122,34 @@ pub fn hooks_commit_msg(
 	File::open(temp_file)?.read_to_string(msg)?;
 
 	Ok(res)
-	} else {
-		Ok(HookResult::Ok)
-	}
 }
 
 /// this hook is documented here <https://git-scm.com/docs/githooks#_pre_commit>
-pub fn hooks_pre_commit(repo: &Repository) -> Result<HookResult> {
+pub fn hooks_pre_commit(
-	let hook = HookPaths::new(repo, HOOK_PRE_COMMIT)?;
+	repo: &Repository,
+	other_paths: Option<&[&str]>,
+) -> Result<HookResult> {
+	let hook = HookPaths::new(repo, other_paths, HOOK_PRE_COMMIT)?;
 
-	if hook.is_executable() {
+	if !hook.found() {
-		Ok(hook.run_hook(&[])?)
+		return Ok(HookResult::NoHookFound);
-	} else {
-		Ok(HookResult::Ok)
 	}
+
+	hook.run_hook(&[])
 }
 
 /// this hook is documented here <https://git-scm.com/docs/githooks#_post_commit>
-pub fn hooks_post_commit(repo: &Repository) -> Result<HookResult> {
+pub fn hooks_post_commit(
-	let hook = HookPaths::new(repo, HOOK_POST_COMMIT)?;
+	repo: &Repository,
+	other_paths: Option<&[&str]>,
+) -> Result<HookResult> {
+	let hook = HookPaths::new(repo, other_paths, HOOK_POST_COMMIT)?;
 
-	if hook.is_executable() {
+	if !hook.found() {
-		Ok(hook.run_hook(&[])?)
+		return Ok(HookResult::NoHookFound);
-	} else {
-		Ok(HookResult::Ok)
 	}
+
+	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();
+		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert_eq!(res, HookResult::Ok);
+		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();
+		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert!(res != HookResult::Ok);
+		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!(
+		let HookResult::RunNotSuccessful { code, stdout, .. } = res
-			res,
+		else {
-			HookResult::NotOk {
+			unreachable!()
-				stdout: String::from("rejected\n"),
+		};
-				stderr: String::new()
+
-			}
+		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();
+		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert!(res != HookResult::Ok);
+		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();
+		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert_eq!(res, HookResult::Ok);
+		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();
+		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert!(res != HookResult::Ok);
+		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!(
+		let HookResult::RunNotSuccessful { code, stdout, .. } = res
-			res,
+		else {
-			HookResult::NotOk {
+			unreachable!()
-				stdout: String::from("rejected\n"),
+		};
-				stderr: String::new()
+
-			}
+		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());
 	}