run hooks in correct pwd (#1049)

see #1046

asyncgit/src/sync/hooks.rs

@@ -13,6 +13,64 @@ const HOOK_PRE_COMMIT: &str = "hooks/pre-commit";
 const HOOK_COMMIT_MSG: &str = "hooks/commit-msg";
 const HOOK_COMMIT_MSG_TEMP_FILE: &str = "COMMIT_EDITMSG";
 
+struct HookPaths {
+	git: PathBuf,
+	hook: PathBuf,
+	pwd: PathBuf,
+}
+
+impl HookPaths {
+	pub fn new(repo_path: &RepoPath, hook: &str) -> Result<Self> {
+		let repo = repo(repo_path)?;
+		let pwd = repo
+			.workdir()
+			.unwrap_or_else(|| repo.path())
+			.to_path_buf();
+		let git_dir = repo.path().to_path_buf();
+		let hook = git_dir.join(hook);
+		Ok(Self {
+			git: git_dir,
+			hook,
+			pwd,
+		})
+	}
+
+	pub fn is_executable(&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 bash_args = vec!["-c".to_string(), arg_str];
+
+		log::trace!("run hook '{:?}' in '{:?}'", self.hook, self.pwd);
+
+		let output = Command::new("bash")
+			.args(bash_args)
+			.current_dir(&self.pwd)
+			// This call forces Command to handle the Path environment correctly on windows,
+			// the specific env set here does not matter
+			// see https://github.com/rust-lang/rust/issues/37519
+			.env(
+				"DUMMY_ENV_TO_FIX_WINDOWS_CMD_RUNS",
+				"FixPathHandlingOnWindows",
+			)
+			.output()?;
+
+		if output.status.success() {
+			Ok(HookResult::Ok)
+		} else {
+			let err = String::from_utf8_lossy(&output.stderr);
+			let out = String::from_utf8_lossy(&output.stdout);
+			let formatted = format!("{}{}", out, err);
+
+			Ok(HookResult::NotOk(formatted))
+		}
+	}
+}
+
 /// this hook is documented here <https://git-scm.com/docs/githooks#_commit_msg>
 /// we use the same convention as other git clients to create a temp file containing
 /// the commit message at `<.git|hooksPath>/COMMIT_EDITMSG` and pass it's relative path as the only
@@ -23,18 +81,17 @@ pub fn hooks_commit_msg(
 ) -> Result<HookResult> {
 	scope_time!("hooks_commit_msg");
 
-	let git_dir = git_dir_as_string(repo_path)?;
+	let hooks_path = HookPaths::new(repo_path, HOOK_COMMIT_MSG)?;
-	let git_dir = git_dir.as_path();
 
-	if hook_runable(git_dir, HOOK_COMMIT_MSG) {
+	if hooks_path.is_executable() {
-		let temp_file = git_dir.join(HOOK_COMMIT_MSG_TEMP_FILE);
+		let temp_file =
+			hooks_path.git.join(HOOK_COMMIT_MSG_TEMP_FILE);
 		File::create(&temp_file)?.write_all(msg.as_bytes())?;
 
-		let res = run_hook(
+		let res = hooks_path.run_hook(&[temp_file
-			git_dir,
+			.as_os_str()
-			HOOK_COMMIT_MSG,
+			.to_string_lossy()
-			&[HOOK_COMMIT_MSG_TEMP_FILE],
+			.as_ref()])?;
-		)?;
 
 		// load possibly altered msg
 		msg.clear();
@@ -51,11 +108,10 @@ pub fn hooks_commit_msg(
 pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
 	scope_time!("hooks_pre_commit");
 
-	let git_dir = git_dir_as_string(repo_path)?;
+	let hook = HookPaths::new(repo_path, HOOK_PRE_COMMIT)?;
-	let git_dir = git_dir.as_path();
 
-	if hook_runable(git_dir, HOOK_PRE_COMMIT) {
+	if hook.is_executable() {
-		Ok(run_hook(git_dir, HOOK_PRE_COMMIT, &[])?)
+		Ok(hook.run_hook(&[])?)
 	} else {
 		Ok(HookResult::Ok)
 	}
@@ -64,26 +120,15 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
 pub fn hooks_post_commit(repo_path: &RepoPath) -> Result<HookResult> {
 	scope_time!("hooks_post_commit");
 
-	let git_dir = git_dir_as_string(repo_path)?;
+	let hook = HookPaths::new(repo_path, HOOK_POST_COMMIT)?;
-	let git_dir = git_dir.as_path();
 
-	if hook_runable(git_dir, HOOK_POST_COMMIT) {
+	if hook.is_executable() {
-		Ok(run_hook(git_dir, HOOK_POST_COMMIT, &[])?)
+		Ok(hook.run_hook(&[])?)
 	} else {
 		Ok(HookResult::Ok)
 	}
 }
 
-fn git_dir_as_string(repo_path: &RepoPath) -> Result<PathBuf> {
-	let repo = repo(repo_path)?;
-	Ok(repo.path().to_path_buf())
-}
-
-fn hook_runable(path: &Path, hook: &str) -> bool {
-	let path = path.join(hook);
-	path.exists() && is_executable(&path)
-}
-
 ///
 #[derive(Debug, PartialEq)]
 pub enum HookResult {
@@ -93,39 +138,6 @@ pub enum HookResult {
 	NotOk(String),
 }
 
-/// this function calls hook scripts based on conventions documented here
-/// see <https://git-scm.com/docs/githooks>
-fn run_hook(
-	path: &Path,
-	hook_script: &str,
-	args: &[&str],
-) -> Result<HookResult> {
-	let arg_str = format!("{} {}", hook_script, args.join(" "));
-	let bash_args = vec!["-c".to_string(), arg_str];
-
-	let output = Command::new("bash")
-		.args(bash_args)
-		.current_dir(path)
-		// This call forces Command to handle the Path environment correctly on windows,
-		// the specific env set here does not matter
-		// see https://github.com/rust-lang/rust/issues/37519
-		.env(
-			"DUMMY_ENV_TO_FIX_WINDOWS_CMD_RUNS",
-			"FixPathHandlingOnWindows",
-		)
-		.output()?;
-
-	if output.status.success() {
-		Ok(HookResult::Ok)
-	} else {
-		let err = String::from_utf8_lossy(&output.stderr);
-		let out = String::from_utf8_lossy(&output.stdout);
-		let formatted = format!("{}{}", out, err);
-
-		Ok(HookResult::NotOk(formatted))
-	}
-}
-
 #[cfg(not(windows))]
 fn is_executable(path: &Path) -> bool {
 	use std::os::unix::fs::PermissionsExt;
@@ -147,11 +159,10 @@ const fn is_executable(_: &Path) -> bool {
 
 #[cfg(test)]
 mod tests {
-	use tempfile::TempDir;
-
 	use super::*;
 	use crate::sync::tests::{repo_init, repo_init_bare};
 	use std::fs::{self, File};
+	use tempfile::TempDir;
 
 	#[test]
 	fn test_smoke() {
@@ -170,24 +181,21 @@ mod tests {
 		assert_eq!(res, HookResult::Ok);
 	}
 
-	fn create_hook(path: &Path, hook_path: &str, hook_script: &[u8]) {
+	fn create_hook(path: &RepoPath, hook: &str, hook_script: &[u8]) {
-		let path = git_dir_as_string(
+		let hook = HookPaths::new(path, hook).unwrap();
-			&path.as_os_str().to_str().unwrap().into(),
-		)
-		.unwrap();
-		let path = path.as_path();
-		dbg!(&path);
 
-		File::create(&path.join(hook_path))
+		File::create(&hook.hook)
 			.unwrap()
 			.write_all(hook_script)
 			.unwrap();
 
 		#[cfg(not(windows))]
 		{
+			let hook = hook.hook.as_os_str();
 			Command::new("chmod")
-				.args(&["+x", hook_path])
+				.arg("+x")
-				.current_dir(path)
+				.arg(hook)
+				// .current_dir(path)
 				.output()
 				.unwrap();
 		}
@@ -204,7 +212,7 @@ mod tests {
 exit 0
         ";
 
-		create_hook(root, HOOK_COMMIT_MSG, hook);
+		create_hook(repo_path, HOOK_COMMIT_MSG, hook);
 
 		let mut msg = String::from("test");
 		let res = hooks_commit_msg(repo_path, &mut msg).unwrap();
@@ -225,7 +233,7 @@ exit 0
 exit 0
         ";
 
-		create_hook(root, HOOK_PRE_COMMIT, hook);
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(repo_path).unwrap();
 		assert_eq!(res, HookResult::Ok);
 	}
@@ -242,7 +250,7 @@ echo 'rejected'
 exit 1
         ";
 
-		create_hook(root, HOOK_PRE_COMMIT, hook);
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(repo_path).unwrap();
 		assert!(res != HookResult::Ok);
 	}
@@ -253,8 +261,8 @@ exit 1
 		let workdir = TempDir::new().unwrap();
 		let git_root = git_root.into_path();
 		let repo_path = &RepoPath::Workdir {
-			gitdir: git_root.to_path_buf(),
+			gitdir: dbg!(git_root.to_path_buf()),
-			workdir: workdir.into_path(),
+			workdir: dbg!(workdir.into_path()),
 		};
 
 		let hook = b"#!/bin/sh
@@ -262,7 +270,7 @@ echo 'rejected'
 exit 1
         ";
 
-		create_hook(git_root.as_path(), "hooks/pre-commit", hook);
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(repo_path).unwrap();
 		assert!(res != HookResult::Ok);
 	}
@@ -286,7 +294,7 @@ import sys
 sys.exit(0)
         ";
 
-		create_hook(root, HOOK_PRE_COMMIT, hook);
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(repo_path).unwrap();
 		assert_eq!(res, HookResult::Ok);
 	}
@@ -310,7 +318,7 @@ import sys
 sys.exit(1)
         ";
 
-		create_hook(root, HOOK_PRE_COMMIT, hook);
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(repo_path).unwrap();
 		assert!(res != HookResult::Ok);
 	}
@@ -328,7 +336,7 @@ echo 'rejected'
 exit 1
         ";
 
-		create_hook(root, HOOK_COMMIT_MSG, hook);
+		create_hook(repo_path, HOOK_COMMIT_MSG, hook);
 
 		let mut msg = String::from("test");
 		let res = hooks_commit_msg(repo_path, &mut msg).unwrap();
@@ -345,7 +353,8 @@ exit 1
 	fn test_hooks_commit_msg_reject_in_subfolder() {
 		let (_td, repo) = repo_init().unwrap();
 		let root = repo.path().parent().unwrap();
-		// let repo_path = root.as_os_str().to_str().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
 
 		let hook = b"#!/bin/sh
 echo 'msg' > $1
@@ -353,7 +362,7 @@ echo 'rejected'
 exit 1
         ";
 
-		create_hook(root, HOOK_COMMIT_MSG, hook);
+		create_hook(repo_path, HOOK_COMMIT_MSG, hook);
 
 		let subfolder = root.join("foo/");
 		fs::create_dir_all(&subfolder).unwrap();
@@ -385,7 +394,7 @@ echo 'msg' > $1
 exit 0
         ";
 
-		create_hook(root, HOOK_COMMIT_MSG, hook);
+		create_hook(repo_path, HOOK_COMMIT_MSG, hook);
 
 		let mut msg = String::from("test");
 		let res = hooks_commit_msg(repo_path, &mut msg).unwrap();
@@ -398,13 +407,15 @@ exit 0
 	fn test_post_commit_hook_reject_in_subfolder() {
 		let (_td, repo) = repo_init().unwrap();
 		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
 
 		let hook = b"#!/bin/sh
 echo 'rejected'
 exit 1
         ";
 
-		create_hook(root, HOOK_POST_COMMIT, hook);
+		create_hook(repo_path, HOOK_POST_COMMIT, hook);
 
 		let subfolder = root.join("foo/");
 		fs::create_dir_all(&subfolder).unwrap();
@@ -418,4 +429,77 @@ exit 1
 			HookResult::NotOk(String::from("rejected\n"))
 		);
 	}
+
+	// make sure we run the hooks with the correct pwd.
+	// for non-bare repos this is the dir of the worktree
+	// unfortunately does not work on windows
+	#[test]
+	#[cfg(unix)]
+	fn test_pre_commit_workdir() {
+		let (_td, repo) = repo_init().unwrap();
+		let root = repo.path().parent().unwrap();
+		let repo_path: &RepoPath =
+			&root.as_os_str().to_str().unwrap().into();
+		let workdir =
+			crate::sync::utils::repo_work_dir(repo_path).unwrap();
+
+		let hook = b"#!/bin/sh
+echo $(pwd)
+exit 1
+        ";
+
+		create_hook(repo_path, HOOK_PRE_COMMIT, hook);
+		let res = hooks_pre_commit(repo_path).unwrap();
+		if let HookResult::NotOk(res) = res {
+			assert_eq!(
+				Path::new(res.trim_end()),
+				Path::new(&workdir)
+			);
+		} else {
+			assert!(false);
+		}
+	}
+
+	#[test]
+	fn test_hook_pwd_in_bare_without_workdir() {
+		let (_td, _repo) = repo_init_bare().unwrap();
+		let git_root = _repo.path().to_path_buf();
+		let repo_path = &RepoPath::Path(git_root.clone());
+
+		let hook =
+			HookPaths::new(repo_path, HOOK_POST_COMMIT).unwrap();
+
+		assert_eq!(hook.pwd, dbg!(git_root));
+	}
+
+	#[test]
+	fn test_hook_pwd_in_bare_with_workdir() {
+		let (git_root, _repo) = repo_init_bare().unwrap();
+		let workdir = TempDir::new().unwrap();
+		let git_root = git_root.into_path();
+		let repo_path = &RepoPath::Workdir {
+			gitdir: dbg!(git_root.to_path_buf()),
+			workdir: dbg!(workdir.path().to_path_buf()),
+		};
+
+		let hook =
+			HookPaths::new(repo_path, HOOK_POST_COMMIT).unwrap();
+
+		assert_eq!(
+			hook.pwd.canonicalize().unwrap(),
+			dbg!(workdir.path().canonicalize().unwrap())
+		);
+	}
+
+	#[test]
+	fn test_hook_pwd() {
+		let (_td, _repo) = repo_init().unwrap();
+		let git_root = _repo.path().to_path_buf();
+		let repo_path = &RepoPath::Path(git_root.clone());
+
+		let hook =
+			HookPaths::new(repo_path, HOOK_POST_COMMIT).unwrap();
+
+		assert_eq!(hook.pwd, git_root.parent().unwrap());
+	}
 }