Run git-hooks more correctly (#2483)

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,14 +74,40 @@ pub fn hooks_prepare_commit_msg(
 
 #[cfg(test)]
 mod tests {
+	use std::{ffi::OsString, io::Write as _, path::Path};
+
+	use git2::Repository;
+	use tempfile::TempDir;
+
 	use super::*;
-	use crate::sync::tests::repo_init;
-	use std::fs::File;
-	use std::io::Write;
-	use std::path::Path;
+	use crate::sync::tests::repo_init_with_prefix;
+
+	fn repo_init() -> Result<(TempDir, Repository)> {
+		let mut os_string: OsString = OsString::new();
+
+		os_string.push("gitui $# ' ");
+
+		#[cfg(target_os = "linux")]
+		{
+			use std::os::unix::ffi::OsStrExt;
+
+			const INVALID_UTF8: &[u8] = b"\xED\xA0\x80";
+
+			os_string.push(std::ffi::OsStr::from_bytes(INVALID_UTF8));
+
+			assert!(os_string.to_str().is_none());
+		}
+
+		os_string.push(" ");
+
+		repo_init_with_prefix(os_string)
+	}
 
 	fn create_hook_in_path(path: &Path, hook_script: &[u8]) {
-		File::create(path).unwrap().write_all(hook_script).unwrap();
+		std::fs::File::create(path)
+			.unwrap()
+			.write_all(hook_script)
+			.unwrap();
 
 		#[cfg(unix)]
 		{
@@ -102,7 +128,7 @@ mod tests {
 		let hook = b"#!/bin/sh
 	echo 'rejected'
 	exit 1
-	        ";
+			";
 
 		git2_hooks::create_hook(
 			&repo,
@@ -113,9 +139,7 @@ mod tests {
 		let subfolder = root.join("foo/");
 		std::fs::create_dir_all(&subfolder).unwrap();
 
-		let res =
-			hooks_post_commit(&subfolder.to_str().unwrap().into())
-				.unwrap();
+		let res = hooks_post_commit(&subfolder.into()).unwrap();
 
 		assert_eq!(
 			res,
@@ -131,16 +155,12 @@ mod tests {
 	fn test_pre_commit_workdir() {
 		let (_td, repo) = repo_init().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 repo_path: &RepoPath = &root.to_path_buf().into();
 
 		let hook = b"#!/bin/sh
-	echo $(pwd)
+	echo \"$(pwd)\"
 	exit 1
-	        ";
-
+		";
 		git2_hooks::create_hook(
 			&repo,
 			git2_hooks::HOOK_PRE_COMMIT,
@@ -149,8 +169,9 @@ mod tests {
 		let res = hooks_pre_commit(repo_path).unwrap();
 		if let HookResult::NotOk(res) = res {
 			assert_eq!(
-				std::path::Path::new(res.trim_end()),
-				std::path::Path::new(&workdir)
+				res.trim_end().trim_end_matches('/'),
+				// TODO: fix if output isn't utf8.
+				root.to_string_lossy().trim_end_matches('/'),
 			);
 		} else {
 			assert!(false);
@@ -163,10 +184,10 @@ mod tests {
 		let root = repo.workdir().unwrap();
 
 		let hook = b"#!/bin/sh
-	echo 'msg' > $1
+	echo 'msg' > \"$1\"
 	echo 'rejected'
 	exit 1
-	        ";
+		";
 
 		git2_hooks::create_hook(
 			&repo,
@@ -178,11 +199,8 @@ mod tests {
 		std::fs::create_dir_all(&subfolder).unwrap();
 
 		let mut msg = String::from("test");
-		let res = hooks_commit_msg(
-			&subfolder.to_str().unwrap().into(),
-			&mut msg,
-		)
-		.unwrap();
+		let res =
+			hooks_commit_msg(&subfolder.into(), &mut msg).unwrap();
 
 		assert_eq!(
 			res,
@@ -199,7 +217,7 @@ mod tests {
 		let root = repo.workdir().unwrap();
 		let mut config = repo.config().unwrap();
 
-		const HOOKS_DIR: &'static str = "my_hooks";
+		const HOOKS_DIR: &str = "my_hooks";
 		config.set_str("core.hooksPath", HOOKS_DIR).unwrap();
 
 		let hook = b"#!/bin/sh
@@ -213,7 +231,7 @@ mod tests {
 
 		let mut msg = String::from("test");
 		let res = hooks_commit_msg(
-			&hooks_folder.to_str().unwrap().into(),
+			&hooks_folder.to_path_buf().into(),
 			&mut msg,
 		)
 		.unwrap();

asyncgit/src/sync/mod.rs

@@ -123,7 +123,7 @@ pub mod tests {
 	};
 	use crate::error::Result;
 	use git2::Repository;
-	use std::{path::Path, process::Command};
+	use std::{ffi::OsStr, path::Path, process::Command};
 	use tempfile::TempDir;
 
 	///
@@ -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: impl AsRef<OsStr>,
+	) -> 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()?;

asyncgit/src/sync/repository.rs

@@ -42,6 +42,12 @@ impl RepoPath {
 	}
 }
 
+impl From<PathBuf> for RepoPath {
+	fn from(value: PathBuf) -> Self {
+		Self::Path(value)
+	}
+}
+
 impl From<&str> for RepoPath {
 	fn from(p: &str) -> Self {
 		Self::Path(PathBuf::from(p))

git2-hooks/src/hookspath.rs

@@ -3,7 +3,7 @@ use git2::Repository;
 use crate::{error::Result, HookResult, HooksError};
 
 use std::{
-	env,
+	ffi::{OsStr, OsString},
 	path::{Path, PathBuf},
 	process::Command,
 	str::FromStr,
@@ -17,6 +17,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.
@@ -134,30 +135,76 @@ impl HookPaths {
 	/// 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> {
+		self.run_hook_os_str(args)
+	}
+
+	/// this function calls hook scripts based on conventions documented here
+	/// see <https://git-scm.com/docs/githooks>
+	pub fn run_hook_os_str<I, S>(&self, args: I) -> Result<HookResult>
+	where
+		I: IntoIterator<Item = S> + Copy,
+		S: AsRef<OsStr>,
+	{
 		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 = {
+				// 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
+				);
+
+				let mut os_str = OsString::new();
+				os_str.push("'");
+				if let Some(hook) = hook.to_str() {
+					os_str.push(hook.replace('\'', REPLACEMENT));
+				} else {
+					#[cfg(windows)]
+					{
+						use std::os::windows::ffi::OsStrExt;
+						if hook
+							.as_os_str()
+							.encode_wide()
+							.any(|x| x == u16::from(b'\''))
+						{
+							// TODO: escape single quotes instead of failing
+							return Err(HooksError::PathToString);
+						}
+					}
+
+					os_str.push(hook.as_os_str());
+				}
+				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 })
@@ -177,6 +224,49 @@ impl HookPaths {
 	}
 }
 
+fn sh_command() -> Command {
+	let mut command = Command::new(sh_path());
+
+	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");
+	}
+
+	command
+}
+
+/// 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")
+			.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 {
+		"sh".into()
+	}
+}
+
 #[cfg(unix)]
 fn is_executable(path: &Path) -> bool {
 	use std::os::unix::fs::PermissionsExt;
@@ -195,40 +285,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

git2-hooks/src/lib.rs

@@ -132,10 +132,7 @@ pub fn hooks_commit_msg(
 	let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE);
 	File::create(&temp_file)?.write_all(msg.as_bytes())?;
 
-	let res = hook.run_hook(&[temp_file
-		.as_os_str()
-		.to_string_lossy()
-		.as_ref()])?;
+	let res = hook.run_hook_os_str([&temp_file])?;
 
 	// load possibly altered msg
 	msg.clear();
@@ -282,7 +279,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 +306,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 +420,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 +436,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 +507,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 +536,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 +562,7 @@ exit 1
 		let (_td, repo) = repo_init();
 
 		let hook = b"#!/bin/sh
-echo 'msg' > $1
+echo 'msg' > \"$1\"
 exit 0
         ";
 
@@ -565,7 +602,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 +626,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
         ";