Resolve core.hooksPath relative to GIT_WORK_TREE (#2571)

* Resolve core.hooksPath relative to GIT_WORK_TREE

git supports relative values in core.hooksPath.

`man git-config`:

> A relative path is taken as relative to the directory where the hooks are
> run (see the "DESCRIPTION" section of githooks[5]).

`man githooks`:

> Before Git invokes a hook, it changes its working directory to either
> $GIT_DIR in a bare repository or the root of the working tree in a >
> non-bare repository.

I.e. relative paths in core.hooksPath in non-bare repositories are always
relative to GIT_WORK_TREE.

There is a further exception; I believe this is not considered for path
resolution:

> An exception are hooks triggered during a push (pre-receive, update,
> post-receive, post-update, push-to-checkout) which are always executed
> in $GIT_DIR.

* Favor Repository::workdir() over path().parent()

This more clearly errors in case of bare repositories instead of using
the parent directory of the bare repository.

---------

Co-authored-by: Naseschwarz <[email protected]>
Co-authored-by: extrawurst <[email protected]>

Author: 3 people

CHANGELOG.md

@@ -25,6 +25,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * respect `.mailmap` [[@acuteenvy](https://github.com/acuteenvy)] ([#2406](https://github.com/gitui-org/gitui/issues/2406))
 
 ### Fixes
+* resolve `core.hooksPath` relative to `GIT_WORK_TREE` [[@naseschwarz](https://github.com/naseschwarz)] ([#2571](https://github.com/gitui-org/gitui/issues/2571))
 * yanking commit ranges no longer generates incorrect dotted range notations, but lists each individual commit [[@naseschwarz](https://github.com/naseschwarz)] (https://github.com/gitui-org/gitui/issues/2576)
 
 ## [0.27.0] - 2024-01-14

asyncgit/src/sync/hooks.rs

@@ -76,11 +76,28 @@ pub fn hooks_prepare_commit_msg(
 mod tests {
 	use super::*;
 	use crate::sync::tests::repo_init;
+	use std::fs::File;
+	use std::io::Write;
+	use std::path::Path;
+
+	fn create_hook_in_path(path: &Path, hook_script: &[u8]) {
+		File::create(path).unwrap().write_all(hook_script).unwrap();
+
+		#[cfg(unix)]
+		{
+			std::process::Command::new("chmod")
+				.arg("+x")
+				.arg(path)
+				// .current_dir(path)
+				.output()
+				.unwrap();
+		}
+	}
 
 	#[test]
 	fn test_post_commit_hook_reject_in_subfolder() {
 		let (_td, repo) = repo_init().unwrap();
-		let root = repo.path().parent().unwrap();
+		let root = repo.workdir().unwrap();
 
 		let hook = b"#!/bin/sh
 	echo 'rejected'
@@ -113,7 +130,7 @@ mod tests {
 	#[cfg(unix)]
 	fn test_pre_commit_workdir() {
 		let (_td, repo) = repo_init().unwrap();
-		let root = repo.path().parent().unwrap();
+		let root = repo.workdir().unwrap();
 		let repo_path: &RepoPath =
 			&root.as_os_str().to_str().unwrap().into();
 		let workdir =
@@ -143,7 +160,7 @@ mod tests {
 	#[test]
 	fn test_hooks_commit_msg_reject_in_subfolder() {
 		let (_td, repo) = repo_init().unwrap();
-		let root = repo.path().parent().unwrap();
+		let root = repo.workdir().unwrap();
 
 		let hook = b"#!/bin/sh
 	echo 'msg' > $1
@@ -174,4 +191,37 @@ mod tests {
 
 		assert_eq!(msg, String::from("msg\n"));
 	}
+
+	#[test]
+	fn test_hooks_commit_msg_reject_in_hooks_folder_githooks_moved_absolute(
+	) {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.workdir().unwrap();
+		let mut config = repo.config().unwrap();
+
+		const HOOKS_DIR: &'static str = "my_hooks";
+		config.set_str("core.hooksPath", HOOKS_DIR).unwrap();
+
+		let hook = b"#!/bin/sh
+	echo 'msg' > \"$1\"
+	echo 'rejected'
+	exit 1
+	        ";
+		let hooks_folder = root.join(HOOKS_DIR);
+		std::fs::create_dir_all(&hooks_folder).unwrap();
+		create_hook_in_path(&hooks_folder.join("commit-msg"), hook);
+
+		let mut msg = String::from("test");
+		let res = hooks_commit_msg(
+			&hooks_folder.to_str().unwrap().into(),
+			&mut msg,
+		)
+		.unwrap();
+		assert_eq!(
+			res,
+			HookResult::NotOk(String::from("rejected\n"))
+		);
+
+		assert_eq!(msg, String::from("msg\n"));
+	}
 }

git2-hooks/src/hookspath.rs

@@ -41,16 +41,8 @@ impl HookPaths {
 		if let Some(config_path) = Self::config_hook_path(repo)? {
 			let hooks_path = PathBuf::from(config_path);
 
-			let hook = hooks_path.join(hook);
-
-			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)?;
+			let hook =
+				Self::expand_path(&hooks_path.join(hook), &pwd)?;
 
 			return Ok(Self {
 				git: git_dir,
@@ -66,6 +58,41 @@ impl HookPaths {
 		})
 	}
 
+	/// Expand path according to the rule of githooks and config
+	/// core.hooksPath
+	fn expand_path(path: &Path, pwd: &Path) -> Result<PathBuf> {
+		let hook_expanded = shellexpand::full(
+			path.as_os_str()
+				.to_str()
+				.ok_or(HooksError::PathToString)?,
+		)?;
+		let hook_expanded = PathBuf::from_str(hook_expanded.as_ref())
+			.map_err(|_| HooksError::PathToString)?;
+
+		// `man git-config`:
+		//
+		// > A relative path is taken as relative to the
+		// > directory where the hooks are run (see the
+		// > "DESCRIPTION" section of githooks[5]).
+		//
+		// `man githooks`:
+		//
+		// > Before Git invokes a hook, it changes its
+		// > working directory to either $GIT_DIR in a bare
+		// > repository or the root of the working tree in a
+		// > non-bare repository.
+		//
+		// I.e. relative paths in core.hooksPath in non-bare
+		// repositories are always relative to GIT_WORK_TREE.
+		Ok({
+			if hook_expanded.is_absolute() {
+				hook_expanded
+			} else {
+				pwd.join(hook_expanded)
+			}
+		})
+	}
+
 	fn config_hook_path(repo: &Repository) -> Result<Option<String>> {
 		Ok(repo.config()?.get_string(CONFIG_HOOKS_PATH).ok())
 	}
@@ -232,3 +259,35 @@ impl CommandExt for Command {
 		self
 	}
 }
+
+#[cfg(test)]
+mod test {
+	use super::HookPaths;
+	use std::path::Path;
+
+	#[test]
+	fn test_hookspath_relative() {
+		assert_eq!(
+			HookPaths::expand_path(
+				&Path::new("pre-commit"),
+				&Path::new("example_git_root"),
+			)
+			.unwrap(),
+			Path::new("example_git_root").join("pre-commit")
+		);
+	}
+
+	#[test]
+	fn test_hookspath_absolute() {
+		let absolute_hook =
+			std::env::current_dir().unwrap().join("pre-commit");
+		assert_eq!(
+			HookPaths::expand_path(
+				&absolute_hook,
+				&Path::new("example_git_root"),
+			)
+			.unwrap(),
+			absolute_hook
+		);
+	}
+}