Git hooks shell fix.merged

CHANGELOG.md

@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
 ## Unreleased
+* execute git-hooks without shell (on *nix) [[@Joshix](https://github.com/Joshix-1)] ([#2483](https://github.com/extrawurst/gitui/pull/2483))
 
 ### Added
 * support loading custom syntax highlighting themes from a file [[@acuteenvy](https://github.com/acuteenvy)] ([#2565](https://github.com/gitui-org/gitui/pull/2565))
@@ -24,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * set the terminal title to `gitui ({repo_path})` [[@acuteenvy](https://github.com/acuteenvy)] ([#2462](https://github.com/gitui-org/gitui/issues/2462))
 * 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))
+
 ## [0.27.0] - 2024-01-14
 
 **new: manage remotes**

asyncgit/src/sync/hooks.rs

@@ -74,13 +74,37 @@ pub fn hooks_prepare_commit_msg(
 
 #[cfg(test)]
 mod tests {
+	use git2::Repository;
+	use tempfile::TempDir;
+
 	use super::*;
-	use crate::sync::tests::repo_init;
+	use crate::sync::tests::repo_init_with_prefix;
+
+	fn repo_init() -> Result<(TempDir, Repository)> {
+		repo_init_with_prefix("gitui $#  ")
+	}
+	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,14 +137,14 @@ 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 =
 			crate::sync::utils::repo_work_dir(repo_path).unwrap();
 
 		let hook = b"#!/bin/sh
-	echo $(pwd)
+	echo \"$(pwd)\"
 	exit 1
 	        ";
 
@@ -143,10 +167,10 @@ 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
+	echo 'msg' > \"$1\"
 	echo 'rejected'
 	exit 1
 	        ";
@@ -174,4 +198,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_with_prefix("bla '").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"));
+	}
 }

asyncgit/src/sync/mod.rs

@@ -144,11 +144,19 @@ pub mod tests {
 
 	///
 	pub fn repo_init() -> Result<(TempDir, Repository)> {
+		repo_init_with_prefix("gitui")
+	}
+
+	///
+	#[inline]
+	pub fn repo_init_with_prefix(
+		prefix: &'static str,
+	) -> Result<(TempDir, Repository)> {
 		init_log();
 
 		sandbox_config_files();
 
-		let td = TempDir::new()?;
+		let td = TempDir::with_prefix(prefix)?;
 		let repo = Repository::init(td.path())?;
 		{
 			let mut config = repo.config()?;

git2-hooks/src/hookspath.rs

@@ -3,7 +3,7 @@ use git2::Repository;
 use crate::{error::Result, HookResult, HooksError};
 
 use std::{
-	env,
+	ffi::OsStr,
 	path::{Path, PathBuf},
 	process::Command,
 	str::FromStr,
@@ -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())
 	}
@@ -108,29 +135,45 @@ impl HookPaths {
 	/// see <https://git-scm.com/docs/githooks>
 	pub fn run_hook(&self, args: &[&str]) -> Result<HookResult> {
 		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 '{:?}'", hook, self.pwd);
 
-		let git_shell = find_bash_executable()
-			.or_else(find_default_unix_shell)
-			.unwrap_or_else(|| "bash".into());
-		let output = Command::new(git_shell)
-			.args(bash_args)
-			.with_no_window()
-			.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",
+		let run_command = |command: &mut Command| {
+			command.current_dir(&self.pwd).with_no_window().output()
+		};
+		let output = if cfg!(windows) {
+			let command = {
+				let mut os_str = std::ffi::OsString::new();
+				os_str.push("'");
+				os_str.push(hook.as_os_str()); // TODO: this doesn't work if `hook` contains single-quotes
+				os_str.push("'");
+				os_str.push(" \"$@\"");
+				os_str
+			};
+			let shell_args = {
+				let mut shell_args = vec![
+					OsStr::new("-l"), // Use -l to avoid "command not found"
+					OsStr::new("-c"),
+					command.as_os_str(),
+					hook.as_os_str(),
+				];
+				shell_args.extend(args.iter().map(OsStr::new));
+				shell_args
+			};
+
+			run_command(
+				Command::new(sh_on_windows())
+					.args(shell_args)
+					// 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()?;
+		} else {
+			run_command(Command::new(&hook).args(args))
+		}?;
 
 		if output.status.success() {
 			Ok(HookResult::Ok { hook })
@@ -150,6 +193,30 @@ impl HookPaths {
 	}
 }
 
+/// Get the path to the sh.exe bundled with Git for Windows
+fn sh_on_windows() -> PathBuf {
+	if cfg!(windows) {
+		Command::new("where.exe")
+			.arg("git")
+			.output()
+			.ok()
+			.map(|out| {
+				PathBuf::from(Into::<String>::into(
+					String::from_utf8_lossy(&out.stdout),
+				))
+			})
+			.as_deref()
+			.and_then(Path::parent)
+			.and_then(Path::parent)
+			.map(|p| p.join("usr/bin/sh.exe"))
+			.filter(|p| p.exists())
+			.unwrap_or_else(|| "sh".into())
+	} else {
+		debug_assert!(false, "should only be called on windows");
+		"sh".into()
+	}
+}
+
 #[cfg(unix)]
 fn is_executable(path: &Path) -> bool {
 	use std::os::unix::fs::PermissionsExt;
@@ -168,40 +235,12 @@ fn is_executable(path: &Path) -> bool {
 }
 
 #[cfg(windows)]
-/// windows does not consider bash scripts to be executable so we consider everything
+/// windows does not consider shell scripts to be executable so we consider everything
 /// to be executable (which is not far from the truth for windows platform.)
 const fn is_executable(_: &Path) -> bool {
 	true
 }
 
-// Find bash.exe, and avoid finding wsl's bash.exe on Windows.
-// None for non-Windows.
-fn find_bash_executable() -> Option<PathBuf> {
-	if cfg!(windows) {
-		Command::new("where.exe")
-			.arg("git")
-			.output()
-			.ok()
-			.map(|out| {
-				PathBuf::from(Into::<String>::into(
-					String::from_utf8_lossy(&out.stdout),
-				))
-			})
-			.as_deref()
-			.and_then(Path::parent)
-			.and_then(Path::parent)
-			.map(|p| p.join("usr/bin/bash.exe"))
-			.filter(|p| p.exists())
-	} else {
-		None
-	}
-}
-
-// Find default shell on Unix-like OS.
-fn find_default_unix_shell() -> Option<PathBuf> {
-	env::var_os("SHELL").map(PathBuf::from)
-}
-
 trait CommandExt {
 	/// The process is a console application that is being run without a
 	/// console window. Therefore, the console handle for the application is
@@ -232,3 +271,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
+		);
+	}
+}