From 10b47046626e34f2e03b38d8b2f757a8c21589f5 Mon Sep 17 00:00:00 2001 From: Stephan Dilly Date: Fri, 17 Dec 2021 00:46:08 +0100 Subject: [PATCH] run hooks in correct pwd (#1049) see #1046 --- asyncgit/src/sync/hooks.rs | 254 ++++++++++++++++++++++++------------- 1 file changed, 169 insertions(+), 85 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index ca94fa65..fcb30559 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/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 { + 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 + pub fn run_hook(&self, args: &[&str]) -> Result { + 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 /// 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 { scope_time!("hooks_commit_msg"); - let git_dir = git_dir_as_string(repo_path)?; - let git_dir = git_dir.as_path(); + let hooks_path = HookPaths::new(repo_path, HOOK_COMMIT_MSG)?; - if hook_runable(git_dir, HOOK_COMMIT_MSG) { - let temp_file = git_dir.join(HOOK_COMMIT_MSG_TEMP_FILE); + 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 = run_hook( - git_dir, - HOOK_COMMIT_MSG, - &[HOOK_COMMIT_MSG_TEMP_FILE], - )?; + let res = hooks_path.run_hook(&[temp_file + .as_os_str() + .to_string_lossy() + .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 { scope_time!("hooks_pre_commit"); - let git_dir = git_dir_as_string(repo_path)?; - let git_dir = git_dir.as_path(); + let hook = HookPaths::new(repo_path, HOOK_PRE_COMMIT)?; - if hook_runable(git_dir, HOOK_PRE_COMMIT) { - Ok(run_hook(git_dir, HOOK_PRE_COMMIT, &[])?) + if hook.is_executable() { + Ok(hook.run_hook(&[])?) } else { Ok(HookResult::Ok) } @@ -64,26 +120,15 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { pub fn hooks_post_commit(repo_path: &RepoPath) -> Result { scope_time!("hooks_post_commit"); - let git_dir = git_dir_as_string(repo_path)?; - let git_dir = git_dir.as_path(); + let hook = HookPaths::new(repo_path, HOOK_POST_COMMIT)?; - if hook_runable(git_dir, HOOK_POST_COMMIT) { - Ok(run_hook(git_dir, HOOK_POST_COMMIT, &[])?) + if hook.is_executable() { + Ok(hook.run_hook(&[])?) } else { Ok(HookResult::Ok) } } -fn git_dir_as_string(repo_path: &RepoPath) -> Result { - 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 -fn run_hook( - path: &Path, - hook_script: &str, - args: &[&str], -) -> Result { - 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]) { - let path = git_dir_as_string( - &path.as_os_str().to_str().unwrap().into(), - ) - .unwrap(); - let path = path.as_path(); - dbg!(&path); + fn create_hook(path: &RepoPath, hook: &str, hook_script: &[u8]) { + let hook = HookPaths::new(path, hook).unwrap(); - 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]) - .current_dir(path) + .arg("+x") + .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(), - workdir: workdir.into_path(), + gitdir: dbg!(git_root.to_path_buf()), + 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()); + } }