diff --git a/CHANGELOG.md b/CHANGELOG.md index c7caec869d..47231d74b1 100644 --- a/CHANGELOG.md +++ b/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)) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 6fc672134f..b78f25d9e9 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/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(); diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index c52a556aad..09d4e6ef53 100644 --- a/asyncgit/src/sync/mod.rs +++ b/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, + ) -> 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()?; diff --git a/asyncgit/src/sync/repository.rs b/asyncgit/src/sync/repository.rs index 2a0af47dbd..ea251c5e46 100644 --- a/asyncgit/src/sync/repository.rs +++ b/asyncgit/src/sync/repository.rs @@ -42,6 +42,12 @@ impl RepoPath { } } +impl From 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)) diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index af4f8af0a7..1518bfd76f 100644 --- a/git2-hooks/src/hookspath.rs +++ b/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 pub fn run_hook(&self, args: &[&str]) -> Result { - 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]; + self.run_hook_os_str(args) + } + /// this function calls hook scripts based on conventions documented here + /// see + pub fn run_hook_os_str(&self, args: I) -> Result + where + I: IntoIterator + Copy, + S: AsRef, + { + let hook = self.hook.clone(); 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,33 +224,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(); + 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", + ); - permissions.mode() & 0o111 != 0 -} + // 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 { +/// 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") @@ -217,16 +259,36 @@ fn find_bash_executable() -> Option { .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 { - 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 { diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 2a458856d7..4c949a1e46 100644 --- a/git2-hooks/src/lib.rs +++ b/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 ";