Run git-hooks more correctly

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 directly if possible (on *nix) else use sh instead of bash (without reading SHELL variable) [[@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))

asyncgit/src/sync/hooks.rs

@@ -74,8 +74,15 @@ 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 $# ' ")
+	}
 
 	#[test]
 	fn test_post_commit_hook_reject_in_subfolder() {
@@ -85,7 +92,7 @@ mod tests {
 		let hook = b"#!/bin/sh
 	echo 'rejected'
 	exit 1
-	        ";
+			";
 
 		git2_hooks::create_hook(
 			&repo,
@@ -120,10 +127,9 @@ mod tests {
 			crate::sync::utils::repo_work_dir(repo_path).unwrap();
 
 		let hook = b"#!/bin/sh
-	echo $(pwd)
+	echo \"$(pwd)\"
 	exit 1
-	        ";
-
+		";
 		git2_hooks::create_hook(
 			&repo,
 			git2_hooks::HOOK_PRE_COMMIT,
@@ -146,10 +152,10 @@ mod tests {
 		let root = repo.path().parent().unwrap();
 
 		let hook = b"#!/bin/sh
-	echo 'msg' > $1
+	echo 'msg' > \"$1\"
 	echo 'rejected'
 	exit 1
-	        ";
+		";
 
 		git2_hooks::create_hook(
 			&repo,

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,6 @@ use git2::Repository;
 use crate::{error::Result, HookResult, HooksError};
 
 use std::{
-	env,
 	path::{Path, PathBuf},
 	process::Command,
 	str::FromStr,
@@ -17,6 +16,7 @@ pub struct HookPaths {
 
 const CONFIG_HOOKS_PATH: &str = "core.hooksPath";
 const DEFAULT_HOOKS_PATH: &str = "hooks";
+const ENOEXEC: i32 = 8;
 
 impl HookPaths {
 	/// `core.hooksPath` always takes precedence.
@@ -108,29 +108,51 @@ 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
+				.args(args)
+				.current_dir(&self.pwd)
+				.with_no_window()
+				.output()
+		};
+
+		let output = if cfg!(windows) {
+			// execute hook in shell
+			let command = {
+				let mut os_str = std::ffi::OsString::new();
+				os_str.push("'");
+				if let Some(hook) = hook.to_str() {
+					// SEE: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02_02
+					// Enclosing characters in single-quotes ( '' ) shall preserve the literal value of each character within the single-quotes.
+					// A single-quote cannot occur within single-quotes.
+					const REPLACEMENT: &str = concat!(
+						"'",   // closing single-quote
+						"\\'", // one escaped single-quote (outside of single-quotes)
+						"'",   // new single-quote
+					);
+					os_str.push(hook.replace('\'', REPLACEMENT));
+				} else {
+					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
+			};
+			run_command(
+				sh_command().arg("-c").arg(command).arg(&hook),
 			)
-			.output()?;
+		} else {
+			// execute hook directly
+			match run_command(&mut Command::new(&hook)) {
+				Err(err) if err.raw_os_error() == Some(ENOEXEC) => {
+					run_command(sh_command().arg(&hook))
+				}
+				result => result,
+			}
+		}?;
 
 		if output.status.success() {
 			Ok(HookResult::Ok { hook })
@@ -150,33 +172,28 @@ impl HookPaths {
 	}
 }
 
-#[cfg(unix)]
-fn is_executable(path: &Path) -> bool {
-	use std::os::unix::fs::PermissionsExt;
+fn sh_command() -> Command {
+	let mut command = Command::new(sh_path());
 
-	let metadata = match path.metadata() {
-		Ok(metadata) => metadata,
-		Err(e) => {
-			log::error!("metadata error: {}", e);
-			return false;
-		}
-	};
-
-	let permissions = metadata.permissions();
-
-	permissions.mode() & 0o111 != 0
-}
+	if cfg!(windows) {
+		// 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
+		command.env(
+			"DUMMY_ENV_TO_FIX_WINDOWS_CMD_RUNS",
+			"FixPathHandlingOnWindows",
+		);
+
+		// Use -l to avoid "command not found"
+		command.arg("-l");
+	}
 
-#[cfg(windows)]
-/// windows does not consider bash 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
+	command
 }
 
-// Find bash.exe, and avoid finding wsl's bash.exe on Windows.
-// None for non-Windows.
-fn find_bash_executable() -> Option<PathBuf> {
+/// Get the path to the sh executable.
+/// On Windows get the sh.exe bundled with Git for Windows
+pub fn sh_path() -> PathBuf {
 	if cfg!(windows) {
 		Command::new("where.exe")
 			.arg("git")
@@ -190,16 +207,36 @@ fn find_bash_executable() -> Option<PathBuf> {
 			.as_deref()
 			.and_then(Path::parent)
 			.and_then(Path::parent)
-			.map(|p| p.join("usr/bin/bash.exe"))
+			.map(|p| p.join("usr/bin/sh.exe"))
 			.filter(|p| p.exists())
+			.unwrap_or_else(|| "sh".into())
 	} else {
-		None
+		"sh".into()
 	}
 }
 
-// Find default shell on Unix-like OS.
-fn find_default_unix_shell() -> Option<PathBuf> {
-	env::var_os("SHELL").map(PathBuf::from)
+#[cfg(unix)]
+fn is_executable(path: &Path) -> bool {
+	use std::os::unix::fs::PermissionsExt;
+
+	let metadata = match path.metadata() {
+		Ok(metadata) => metadata,
+		Err(e) => {
+			log::error!("metadata error: {}", e);
+			return false;
+		}
+	};
+
+	let permissions = metadata.permissions();
+
+	permissions.mode() & 0o111 != 0
+}
+
+#[cfg(windows)]
+/// 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
 }
 
 trait CommandExt {

git2-hooks/src/lib.rs

@@ -282,7 +282,7 @@ exit 0
 
 		let hook = br#"#!/bin/sh
 COMMIT_MSG="$(cat "$1")"
-printf "$COMMIT_MSG" | sed 's/sth/shell_command/g' >"$1"
+printf "$COMMIT_MSG" | sed 's/sth/shell_command/g' > "$1"
 exit 0
         "#;
 
@@ -309,6 +309,41 @@ exit 0
 		assert!(res.is_ok());
 	}
 
+	#[test]
+	fn test_hook_with_missing_shebang() {
+		const TEXT: &str = "Hello, world!";
+
+		let (_td, repo) = repo_init();
+
+		let hook = b"echo \"$@\"\nexit 42";
+
+		create_hook(&repo, HOOK_PRE_COMMIT, hook);
+
+		let hook =
+			HookPaths::new(&repo, None, HOOK_PRE_COMMIT).unwrap();
+
+		assert!(hook.found());
+
+		let result = hook.run_hook(&[TEXT]).unwrap();
+
+		let HookResult::RunNotSuccessful {
+			code,
+			stdout,
+			stderr,
+			hook: h,
+		} = result
+		else {
+			unreachable!("run_hook should've failed");
+		};
+
+		let stdout = stdout.as_str().trim_ascii_end();
+
+		assert_eq!(code, Some(42));
+		assert_eq!(h, hook.hook);
+		assert_eq!(stdout, TEXT, "{:?} != {TEXT:?}", stdout);
+		assert!(stderr.is_empty());
+	}
+
 	#[test]
 	fn test_no_hook_found() {
 		let (_td, repo) = repo_init();
@@ -388,6 +423,8 @@ exit 1
 
 	#[test]
 	fn test_env_containing_path() {
+		const PATH_EXPORT: &str = "export PATH";
+
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
@@ -402,9 +439,12 @@ exit 1
 			unreachable!()
 		};
 
-		assert!(stdout
-			.lines()
-			.any(|line| line.starts_with("export PATH")));
+		assert!(
+			stdout
+				.lines()
+				.any(|line| line.starts_with(PATH_EXPORT)),
+			"Could not find line starting with {PATH_EXPORT:?} in: {stdout:?}"
+		);
 	}
 
 	#[test]
@@ -470,7 +510,7 @@ sys.exit(0)
 
 		create_hook(&repo, HOOK_PRE_COMMIT, hook);
 		let res = hooks_pre_commit(&repo, None).unwrap();
-		assert!(res.is_ok());
+		assert!(res.is_ok(), "{res:?}");
 	}
 
 	#[test]
@@ -499,9 +539,9 @@ sys.exit(1)
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
-echo 'msg' > $1
-echo 'rejected'
-exit 1
+	echo 'msg' > \"$1\"
+	echo 'rejected'
+	exit 1
         ";
 
 		create_hook(&repo, HOOK_COMMIT_MSG, hook);
@@ -525,7 +565,7 @@ exit 1
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
-echo 'msg' > $1
+echo 'msg' > \"$1\"
 exit 0
         ";
 
@@ -565,7 +605,7 @@ exit 0
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
-echo msg:$2 > $1
+echo \"msg:$2\" > \"$1\"
 exit 0
         ";
 
@@ -589,7 +629,7 @@ exit 0
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
-echo $2,$3 > $1
+echo \"$2,$3\" > \"$1\"
 echo 'rejected'
 exit 2
         ";