diff --git a/CHANGELOG.md b/CHANGELOG.md index c7caec869d..e2f1b2b8a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * new command-line option to override the default log file path (`--logfile`) [[@acuteenvy](https://github.com/acuteenvy)] ([#2539](https://github.com/gitui-org/gitui/pull/2539)) * dx: `make check` checks Cargo.toml dependency ordering using `cargo sort` [[@naseschwarz](https://github.com/naseschwarz)] * add `use_selection_fg` to theme file to allow customizing selection foreground color [[@Upsylonbare](https://github.com/Upsylonbare)] ([#2515](https://github.com/gitui-org/gitui/pull/2515)) +* add timeouts for hooks [[@DaRacci](https://github.com/DaRacci)] ([#2547](https://github.com/gitui-org/gitui/pull/2547)) ### Changed * improve syntax highlighting file detection [[@acuteenvy](https://github.com/acuteenvy)] ([#2524](https://github.com/extrawurst/gitui/pull/2524)) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 6fc672134f..7c273c621a 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -2,6 +2,7 @@ use super::{repository::repo, RepoPath}; use crate::error::Result; pub use git2_hooks::PrepareCommitMsgSource; use scopetime::scope_time; +use std::time::Duration; /// #[derive(Debug, PartialEq, Eq)] @@ -10,6 +11,8 @@ pub enum HookResult { Ok, /// Hook returned error NotOk(String), + /// Hook timed out + TimedOut, } impl From for HookResult { @@ -22,6 +25,7 @@ impl From for HookResult { stderr, .. } => Self::NotOk(format!("{stdout}{stderr}")), + git2_hooks::HookResult::TimedOut { .. } => Self::TimedOut, } } } @@ -38,6 +42,22 @@ pub fn hooks_commit_msg( Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into()) } +/// see `git2_hooks::hooks_prepare_commit_msg` +#[allow(unused)] +pub fn hooks_commit_msg_with_timeout( + repo_path: &RepoPath, + msg: &mut String, + timeout: Duration, +) -> Result { + scope_time!("hooks_prepare_commit_msg"); + + let repo = repo(repo_path)?; + Ok(git2_hooks::hooks_commit_msg_with_timeout( + &repo, None, msg, timeout, + )? + .into()) +} + /// see `git2_hooks::hooks_pre_commit` pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { scope_time!("hooks_pre_commit"); @@ -47,6 +67,22 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into()) } +/// see `git2_hooks::hooks_pre_commit` +#[allow(unused)] +pub fn hooks_pre_commit_with_timeout( + repo_path: &RepoPath, + timeout: Duration, +) -> Result { + scope_time!("hooks_pre_commit"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_pre_commit_with_timeout( + &repo, None, timeout, + )? + .into()) +} + /// see `git2_hooks::hooks_post_commit` pub fn hooks_post_commit(repo_path: &RepoPath) -> Result { scope_time!("hooks_post_commit"); @@ -56,6 +92,22 @@ pub fn hooks_post_commit(repo_path: &RepoPath) -> Result { Ok(git2_hooks::hooks_post_commit(&repo, None)?.into()) } +/// see `git2_hooks::hooks_post_commit` +#[allow(unused)] +pub fn hooks_post_commit_with_timeout( + repo_path: &RepoPath, + timeout: Duration, +) -> Result { + scope_time!("hooks_post_commit"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_post_commit_with_timeout( + &repo, None, timeout, + )? + .into()) +} + /// see `git2_hooks::hooks_prepare_commit_msg` pub fn hooks_prepare_commit_msg( repo_path: &RepoPath, @@ -72,8 +124,28 @@ pub fn hooks_prepare_commit_msg( .into()) } +/// see `git2_hooks::hooks_prepare_commit_msg` +#[allow(unused)] +pub fn hooks_prepare_commit_msg_with_timeout( + repo_path: &RepoPath, + source: PrepareCommitMsgSource, + msg: &mut String, + timeout: Duration, +) -> Result { + scope_time!("hooks_prepare_commit_msg"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_prepare_commit_msg_with_timeout( + &repo, None, source, msg, timeout, + )? + .into()) +} + #[cfg(test)] mod tests { + use tempfile::tempdir; + use super::*; use crate::sync::tests::repo_init; use std::fs::File; @@ -99,7 +171,7 @@ mod tests { let (_td, repo) = repo_init().unwrap(); let root = repo.workdir().unwrap(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'rejected' exit 1 "; @@ -136,7 +208,7 @@ mod tests { let workdir = crate::sync::utils::repo_work_dir(repo_path).unwrap(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo $(pwd) exit 1 "; @@ -162,7 +234,7 @@ mod tests { let (_td, repo) = repo_init().unwrap(); let root = repo.workdir().unwrap(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'msg' > $1 echo 'rejected' exit 1 @@ -224,4 +296,106 @@ mod tests { assert_eq!(msg, String::from("msg\n")); } + + #[test] + fn test_hooks_respect_timeout() { + let (_td, repo) = repo_init().unwrap(); + let root = repo.path().parent().unwrap(); + + let hook = b"#!/usr/bin/env sh + sleep 0.250 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_COMMIT, + hook, + ); + + let res = hooks_pre_commit_with_timeout( + &root.to_str().unwrap().into(), + Duration::from_millis(200), + ) + .unwrap(); + + assert_eq!(res, HookResult::TimedOut); + } + + #[test] + fn test_hooks_faster_than_timeout() { + let (_td, repo) = repo_init().unwrap(); + let root = repo.path().parent().unwrap(); + + let hook = b"#!/usr/bin/env sh + sleep 0.1 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_COMMIT, + hook, + ); + + let res = hooks_pre_commit_with_timeout( + &root.to_str().unwrap().into(), + Duration::from_millis(150), + ) + .unwrap(); + + assert_eq!(res, HookResult::Ok); + } + + #[test] + fn test_hooks_timeout_zero() { + let (_td, repo) = repo_init().unwrap(); + let root = repo.path().parent().unwrap(); + + let hook = b"#!/usr/bin/env sh + sleep 1 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_POST_COMMIT, + hook, + ); + + let res = hooks_post_commit_with_timeout( + &root.to_str().unwrap().into(), + Duration::ZERO, + ) + .unwrap(); + + assert_eq!(res, HookResult::Ok); + } + + #[test] + fn test_run_with_timeout_kills() { + let (_td, repo) = repo_init().unwrap(); + let root = repo.path().parent().unwrap(); + + let temp_dir = tempdir().expect("temp dir"); + let file = temp_dir.path().join("test"); + let hook = format!( + "#!/usr/bin/env sh +sleep 1 +echo 'after sleep' > {} + ", + file.as_path().to_str().unwrap() + ); + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_COMMIT, + hook.as_bytes(), + ); + + let res = hooks_pre_commit_with_timeout( + &root.to_str().unwrap().into(), + Duration::from_millis(100), + ); + + assert!(res.is_ok()); + assert!(!file.exists()); + } } diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index c52a556aad..e78984f945 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -66,8 +66,11 @@ pub use config::{ pub use diff::get_diff_commit; pub use git2::BranchType; pub use hooks::{ - hooks_commit_msg, hooks_post_commit, hooks_pre_commit, - hooks_prepare_commit_msg, HookResult, PrepareCommitMsgSource, + hooks_commit_msg, hooks_commit_msg_with_timeout, + hooks_post_commit, hooks_post_commit_with_timeout, + hooks_pre_commit, hooks_pre_commit_with_timeout, + hooks_prepare_commit_msg, hooks_prepare_commit_msg_with_timeout, + HookResult, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index af4f8af0a7..cc48e5f822 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -1,12 +1,15 @@ use git2::Repository; +use log::debug; use crate::{error::Result, HookResult, HooksError}; use std::{ env, path::{Path, PathBuf}, - process::Command, + process::{Child, Command, Stdio}, str::FromStr, + thread, + time::Duration, }; pub struct HookPaths { @@ -135,48 +138,186 @@ impl HookPaths { /// see pub fn run_hook(&self, args: &[&str]) -> Result { let hook = self.hook.clone(); + let output = spawn_hook_process(&self.pwd, &hook, args)? + .wait_with_output()?; - 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", - ) - .output()?; + Ok(hook_result_from_output(hook, &output)) + } - if output.status.success() { - Ok(HookResult::Ok { hook }) + /// this function calls hook scripts based on conventions documented here + /// see + /// + /// With the addition of a timeout for the execution of the script. + /// If the script takes longer than the specified timeout it will be killed. + /// + /// This will add an additional 1ms at a minimum, up to a maximum of 50ms. + /// see `timeout_with_quadratic_backoff` for more information + pub fn run_hook_with_timeout( + &self, + args: &[&str], + timeout: Duration, + ) -> Result { + let hook = self.hook.clone(); + let mut child = spawn_hook_process(&self.pwd, &hook, args)?; + + let output = if timeout.is_zero() { + child.wait_with_output()? } else { - let stderr = - String::from_utf8_lossy(&output.stderr).to_string(); - let stdout = - String::from_utf8_lossy(&output.stdout).to_string(); - - Ok(HookResult::RunNotSuccessful { - code: output.status.code(), - stdout, - stderr, - hook, - }) + if !timeout_with_quadratic_backoff(timeout, || { + Ok(child.try_wait()?.is_some()) + })? { + debug!("killing hook process"); + child.kill()?; + return Ok(HookResult::TimedOut { hook }); + } + + child.wait_with_output()? + }; + + Ok(hook_result_from_output(hook, &output)) + } +} + +/// This will loop, sleeping with exponentially increasing time until completion or timeout has been reached. +/// +/// Formula: +/// Base Duration: `BASE_MILLIS` is set to 1 millisecond. +/// Max Sleep Duration: `MAX_SLEEP_MILLIS` is set to 50 milliseconds. +/// Quadratic Calculation: Sleep time = (attempt^2) * `BASE_MILLIS`, capped by `MAX_SLEEP_MILLIS`. +/// +/// The timing for each attempt up to the cap is as follows. +/// +/// Attempt 1: +/// Sleep Time=(1^2)×1=1 +/// Actual Sleep: 1 millisecond +/// Total Sleep: 1 millisecond +/// +/// Attempt 2: +/// Sleep Time=(2^2)×1=4 +/// Actual Sleep: 4 milliseconds +/// Total Sleep: 5 milliseconds +/// +/// Attempt 3: +/// Sleep Time=(3^2)×1=9 +/// Actual Sleep: 9 milliseconds +/// Total Sleep: 14 milliseconds +/// +/// Attempt 4: +/// Sleep Time=(4^2)×1=16 +/// Actual Sleep: 16 milliseconds +/// Total Sleep: 30 milliseconds +/// +/// Attempt 5: +/// Sleep Time=(5^2)×1=25 +/// Actual Sleep: 25 milliseconds +/// Total Sleep: 55 milliseconds +/// +/// Attempt 6: +/// Sleep Time=(6^2)×1=36 +/// Actual Sleep: 36 milliseconds +/// Total Sleep: 91 milliseconds +/// +/// Attempt 7: +/// Sleep Time=(7^2)×1=49 +/// Actual Sleep: 49 milliseconds +/// Total Sleep: 140 milliseconds +/// +/// Attempt 8: +// Sleep Time=(8^2)×1=64, capped by `MAX_SLEEP_MILLIS` of 50 +// Actual Sleep: 50 milliseconds +// Total Sleep: 190 milliseconds +fn timeout_with_quadratic_backoff( + timeout: Duration, + mut is_complete: F, +) -> Result +where + F: FnMut() -> Result, +{ + const BASE_MILLIS: u64 = 1; + const MAX_SLEEP_MILLIS: u64 = 50; + + let timer = std::time::Instant::now(); + let mut attempt: i32 = 1; + + loop { + if is_complete()? { + return Ok(true); + } + + if timer.elapsed() > timeout { + return Ok(false); + } + + let mut sleep_time = Duration::from_millis( + (attempt.pow(2) as u64) + .saturating_mul(BASE_MILLIS) + .min(MAX_SLEEP_MILLIS), + ); + + // Ensure we do not sleep more than the remaining time + let remaining_time = timeout - timer.elapsed(); + if remaining_time < sleep_time { + sleep_time = remaining_time; + } + + thread::sleep(sleep_time); + attempt += 1; + } +} + +fn hook_result_from_output( + hook: PathBuf, + output: &std::process::Output, +) -> HookResult { + if output.status.success() { + HookResult::Ok { hook } + } else { + let stderr = + String::from_utf8_lossy(&output.stderr).to_string(); + let stdout = + String::from_utf8_lossy(&output.stdout).to_string(); + + HookResult::RunNotSuccessful { + code: output.status.code(), + stdout, + stderr, + hook, } } } +fn spawn_hook_process( + directory: &PathBuf, + hook: &PathBuf, + args: &[&str], +) -> Result { + 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, directory); + + let git_shell = find_bash_executable() + .or_else(find_default_unix_shell) + .unwrap_or_else(|| "bash".into()); + let child = Command::new(git_shell) + .args(bash_args) + .with_no_window() + .current_dir(directory) + // 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", + ) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + + Ok(child) +} + #[cfg(unix)] fn is_executable(path: &Path) -> bool { use std::os::unix::fs::PermissionsExt; @@ -262,7 +403,8 @@ impl CommandExt for Command { #[cfg(test)] mod test { - use super::HookPaths; + use super::*; + use pretty_assertions::assert_eq; use std::path::Path; #[test] @@ -290,4 +432,53 @@ mod test { absolute_hook ); } + + /// Ensures that the `timeout_with_quadratic_backoff` function + /// does not cause the total execution time does not grealy increase the total execution time. + #[test] + fn test_timeout_with_quadratic_backoff_cost() { + let timeout = Duration::from_millis(100); + let start = std::time::Instant::now(); + let result = + timeout_with_quadratic_backoff(timeout, || Ok(false)); + let elapsed = start.elapsed(); + + assert_eq!(result.unwrap(), false); + assert!(elapsed < timeout + Duration::from_millis(10)); + } + + /// Ensures that the `timeout_with_quadratic_backoff` function + /// does not cause the execution time wait for much longer than the reason we are waiting. + #[test] + fn test_timeout_with_quadratic_backoff_timeout() { + let timeout = Duration::from_millis(100); + let wait_time = Duration::from_millis(5); // Attempt 1 + 2 = 5 ms + + let start = std::time::Instant::now(); + let _ = timeout_with_quadratic_backoff(timeout, || { + Ok(start.elapsed() > wait_time) + }); + + let elapsed = start.elapsed(); + assert_eq!(5, elapsed.as_millis()); + } + + /// Ensures that the overhead of the `timeout_with_quadratic_backoff` function + /// does not exceed 15 microseconds per attempt. + /// + /// This will obviously vary depending on the system, but this is a rough estimate. + /// The overhead on an AMD 5900x is roughly 1 - 1.5 microseconds per attempt. + #[test] + fn test_timeout_with_quadratic_backoff_overhead() { + // A timeout of 50 milliseconds should take 8 attempts to reach the timeout. + const TARGET_ATTEMPTS: u128 = 8; + const TIMEOUT: Duration = Duration::from_millis(190); + + let start = std::time::Instant::now(); + let _ = timeout_with_quadratic_backoff(TIMEOUT, || Ok(false)); + let elapsed = start.elapsed(); + + let overhead = (elapsed - TIMEOUT).as_micros(); + assert!(overhead < TARGET_ATTEMPTS * 15); + } } diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 2a458856d7..9db91416d1 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -31,6 +31,7 @@ use std::{ fs::File, io::{Read, Write}, path::{Path, PathBuf}, + time::Duration, }; pub use error::HooksError; @@ -66,6 +67,11 @@ pub enum HookResult { /// path of the hook that was run hook: PathBuf, }, + /// Hook took too long to execute and was killed + TimedOut { + /// path of the hook that was run + hook: PathBuf, + }, } impl HookResult { @@ -78,6 +84,11 @@ impl HookResult { pub const fn is_not_successful(&self) -> bool { matches!(self, Self::RunNotSuccessful { .. }) } + + /// helper to check if result was a timeout + pub const fn is_timeout(&self) -> bool { + matches!(self, Self::TimedOut { .. }) + } } /// helper method to create git hooks programmatically (heavy used in unittests) @@ -112,6 +123,16 @@ fn create_hook_in_path(path: &Path, hook_script: &[u8]) { } } +macro_rules! find_hook { + ($repo:expr, $other_paths:expr, $hook_type:expr) => {{ + let hook = HookPaths::new($repo, $other_paths, $hook_type)?; + if !hook.found() { + return Ok(HookResult::NoHookFound); + } + hook + }}; +} + /// Git hook: `commit_msg` /// /// This hook is documented here . @@ -123,11 +144,7 @@ pub fn hooks_commit_msg( other_paths: Option<&[&str]>, msg: &mut String, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_COMMIT_MSG)?; - - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + let hook = find_hook!(repo, other_paths, HOOK_COMMIT_MSG); let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE); File::create(&temp_file)?.write_all(msg.as_bytes())?; @@ -144,34 +161,75 @@ pub fn hooks_commit_msg( Ok(res) } +/// Git hook: `commit_msg` +/// +/// See [`hooks_commit_msg`] for more details. +pub fn hooks_commit_msg_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + msg: &mut String, + timeout: Duration, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_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_with_timeout( + &[temp_file.as_os_str().to_string_lossy().as_ref()], + timeout, + )?; + + // load possibly altered msg + msg.clear(); + File::open(temp_file)?.read_to_string(msg)?; + + Ok(res) +} + /// this hook is documented here pub fn hooks_pre_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_PRE_COMMIT)?; - - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); hook.run_hook(&[]) } +/// this hook is documented here +pub fn hooks_pre_commit_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + timeout: Duration, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); + + hook.run_hook_with_timeout(&[], timeout) +} + /// this hook is documented here pub fn hooks_post_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_POST_COMMIT)?; - - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); hook.run_hook(&[]) } +/// this hook is documented here +pub fn hooks_post_commit_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + timeout: Duration, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); + + hook.run_hook_with_timeout(&[], timeout) +} + +#[derive(Clone, Copy)] pub enum PrepareCommitMsgSource { Message, Template, @@ -188,13 +246,54 @@ pub fn hooks_prepare_commit_msg( source: PrepareCommitMsgSource, msg: &mut String, ) -> Result { - let hook = - HookPaths::new(repo, other_paths, HOOK_PREPARE_COMMIT_MSG)?; + let hook = find_hook!(repo, other_paths, HOOK_PREPARE_COMMIT_MSG); + + let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE); + File::create(&temp_file)?.write_all(msg.as_bytes())?; + + let temp_file_path = temp_file.as_os_str().to_string_lossy(); + + let vec = vec![ + temp_file_path.as_ref(), + match source { + PrepareCommitMsgSource::Message => "message", + PrepareCommitMsgSource::Template => "template", + PrepareCommitMsgSource::Merge => "merge", + PrepareCommitMsgSource::Squash => "squash", + PrepareCommitMsgSource::Commit(_) => "commit", + }, + ]; + let mut args = vec; + + let id = if let PrepareCommitMsgSource::Commit(id) = &source { + Some(id.to_string()) + } else { + None + }; - if !hook.found() { - return Ok(HookResult::NoHookFound); + if let Some(id) = &id { + args.push(id); } + let res = hook.run_hook(args.as_slice())?; + + // load possibly altered msg + msg.clear(); + File::open(temp_file)?.read_to_string(msg)?; + + Ok(res) +} + +/// this hook is documented here +pub fn hooks_prepare_commit_msg_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + source: PrepareCommitMsgSource, + msg: &mut String, + timeout: Duration, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_PREPARE_COMMIT_MSG); + let temp_file = hook.git.join(HOOK_COMMIT_MSG_TEMP_FILE); File::create(&temp_file)?.write_all(msg.as_bytes())?; @@ -222,7 +321,7 @@ pub fn hooks_prepare_commit_msg( args.push(id); } - let res = hook.run_hook(args.as_slice())?; + let res = hook.run_hook_with_timeout(args.as_slice(), timeout)?; // load possibly altered msg msg.clear(); @@ -236,7 +335,7 @@ mod tests { use super::*; use git2_testing::{repo_init, repo_init_bare}; use pretty_assertions::assert_eq; - use tempfile::TempDir; + use tempfile::{tempdir, TempDir}; #[test] fn test_smoke() { @@ -247,7 +346,7 @@ mod tests { assert_eq!(res, HookResult::NoHookFound); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -262,7 +361,7 @@ exit 0 fn test_hooks_commit_msg_ok() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -280,7 +379,7 @@ exit 0 fn test_hooks_commit_msg_with_shell_command_ok() { let (_td, repo) = repo_init(); - let hook = br#"#!/bin/sh + let hook = br#"#!/usr/bin/env sh COMMIT_MSG="$(cat "$1")" printf "$COMMIT_MSG" | sed 's/sth/shell_command/g' >"$1" exit 0 @@ -300,7 +399,7 @@ exit 0 fn test_pre_commit_sh() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -321,7 +420,7 @@ exit 0 fn test_other_path() { let (td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -340,11 +439,11 @@ exit 0 } #[test] - fn test_other_path_precendence() { + fn test_other_path_precedence() { let (td, repo) = repo_init(); { - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -352,7 +451,7 @@ exit 0 } { - let reject_hook = b"#!/bin/sh + let reject_hook = b"#!/usr/bin/env sh exit 1 "; @@ -376,7 +475,7 @@ exit 1 fn test_pre_commit_fail_sh() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'rejected' exit 1 "; @@ -390,7 +489,7 @@ exit 1 fn test_env_containing_path() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh export exit 1 "; @@ -412,7 +511,7 @@ exit 1 let (_td, repo) = repo_init(); let hooks = TempDir::new().unwrap(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'rejected' exit 1 "; @@ -442,7 +541,7 @@ exit 1 fn test_pre_commit_fail_bare() { let (_td, repo) = repo_init_bare(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'rejected' exit 1 "; @@ -456,7 +555,7 @@ exit 1 fn test_pre_commit_py() { let (_td, repo) = repo_init(); - // mirror how python pre-commmit sets itself up + // mirror how python pre-commit sets itself up #[cfg(not(windows))] let hook = b"#!/usr/bin/env python import sys @@ -477,7 +576,7 @@ sys.exit(0) fn test_pre_commit_fail_py() { let (_td, repo) = repo_init(); - // mirror how python pre-commmit sets itself up + // mirror how python pre-commit sets itself up #[cfg(not(windows))] let hook = b"#!/usr/bin/env python import sys @@ -498,7 +597,7 @@ sys.exit(1) fn test_hooks_commit_msg_reject() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'msg' > $1 echo 'rejected' exit 1 @@ -524,7 +623,7 @@ exit 1 fn test_commit_msg_no_block_but_alter() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo 'msg' > $1 exit 0 "; @@ -564,7 +663,7 @@ exit 0 fn test_hooks_prep_commit_msg_success() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo msg:$2 > $1 exit 0 "; @@ -588,7 +687,7 @@ exit 0 fn test_hooks_prep_commit_msg_reject() { let (_td, repo) = repo_init(); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh echo $2,$3 > $1 echo 'rejected' exit 2 @@ -620,4 +719,73 @@ exit 2 ) ); } + + #[test] + fn test_hooks_timeout_kills() { + let (_td, repo) = repo_init(); + + let hook = b"#!/usr/bin/env sh +sleep 0.25 + "; + + create_hook(&repo, HOOK_PRE_COMMIT, hook); + + let res = hooks_pre_commit_with_timeout( + &repo, + None, + Duration::from_millis(200), + ) + .unwrap(); + + assert!(res.is_timeout()); + } + + #[test] + fn test_hooks_timeout_with_zero() { + let (_td, repo) = repo_init(); + + let hook = b"#!/usr/bin/env sh +sleep 0.25 + "; + + create_hook(&repo, HOOK_PRE_COMMIT, hook); + + let res = hooks_pre_commit_with_timeout( + &repo, + None, + Duration::ZERO, + ); + + assert!(res.is_ok()); + } + + #[test] + fn test_hooks_timeout_faster_than_timeout() { + let (_td, repo) = repo_init(); + + let temp_dir = tempdir().expect("temp dir"); + let file = temp_dir.path().join("test"); + let hook = format!( + "#!/usr/bin/env sh +sleep 0.1 +echo 'after sleep' > {} + ", + file.to_str().unwrap() + ); + + create_hook(&repo, HOOK_PRE_COMMIT, hook.as_bytes()); + + let res = hooks_pre_commit_with_timeout( + &repo, + None, + Duration::from_millis(150), + ); + + assert!(res.is_ok()); + assert!(file.exists()); + + let mut str = String::new(); + File::open(file).unwrap().read_to_string(&mut str).unwrap(); + assert!(str == "after sleep\n"); + } } diff --git a/src/app.rs b/src/app.rs index 68d2987c6f..36cc834ada 100644 --- a/src/app.rs +++ b/src/app.rs @@ -846,6 +846,7 @@ impl App { | AppOption::DiffInterhunkLines => { self.status_tab.update_diff()?; } + AppOption::HookTimeout => {} } flags.insert(NeedsUpdate::ALL); diff --git a/src/options.rs b/src/options.rs index db04802092..133df1b256 100644 --- a/src/options.rs +++ b/src/options.rs @@ -14,6 +14,7 @@ use std::{ io::{Read, Write}, path::PathBuf, rc::Rc, + time::Duration, }; #[derive(Default, Clone, Serialize, Deserialize)] @@ -22,6 +23,7 @@ struct OptionsData { pub diff: DiffOptions, pub status_show_untracked: Option, pub commit_msgs: Vec, + pub hook_timeout: Option, } const COMMIT_MSG_HISTORY_LENGTH: usize = 20; @@ -66,6 +68,11 @@ impl Options { self.data.diff } + #[allow(unused)] + pub const fn hook_timeout(&self) -> Option { + self.data.hook_timeout + } + pub const fn status_show_untracked( &self, ) -> Option { @@ -137,6 +144,12 @@ impl Options { } } + #[allow(unused)] + pub fn set_hook_timeout(&mut self, timeout: Option) { + self.data.hook_timeout = timeout; + self.save(); + } + fn save(&self) { if let Err(e) = self.save_failable() { log::error!("options save error: {}", e); diff --git a/src/popups/commit.rs b/src/popups/commit.rs index 008fc6f8a7..2b0e6d1970 100644 --- a/src/popups/commit.rs +++ b/src/popups/commit.rs @@ -28,6 +28,7 @@ use ratatui::{ Frame, }; +use std::time::Duration; use std::{ fmt::Write as _, fs::{read_to_string, File}, @@ -237,14 +238,28 @@ impl CommitPopup { if verify { // run pre commit hook - can reject commit - if let HookResult::NotOk(e) = - sync::hooks_pre_commit(&self.repo.borrow())? - { - log::error!("pre-commit hook error: {}", e); - self.queue.push(InternalEvent::ShowErrorMsg( - format!("pre-commit hook error:\n{e}"), - )); - return Ok(CommitResult::Aborted); + match sync::hooks_pre_commit_with_timeout( + &self.repo.borrow(), + self.get_hook_timeout(), + )? { + HookResult::NotOk(e) => { + log::error!("pre-commit hook error: {}", e); + self.queue.push(InternalEvent::ShowErrorMsg( + format!("pre-commit hook error:\n{e}"), + )); + return Ok(CommitResult::Aborted); + } + HookResult::TimedOut => { + log::error!("pre-commit hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "pre-commit hook timed out after {} seconds", + self.get_hook_timeout().as_secs() + ), + )); + return Ok(CommitResult::Aborted); + } + HookResult::Ok => {} } } @@ -253,25 +268,53 @@ impl CommitPopup { if verify { // run commit message check hook - can reject commit - if let HookResult::NotOk(e) = - sync::hooks_commit_msg(&self.repo.borrow(), &mut msg)? - { - log::error!("commit-msg hook error: {}", e); - self.queue.push(InternalEvent::ShowErrorMsg( - format!("commit-msg hook error:\n{e}"), - )); - return Ok(CommitResult::Aborted); + match sync::hooks_commit_msg_with_timeout( + &self.repo.borrow(), + &mut msg, + self.get_hook_timeout(), + )? { + HookResult::NotOk(e) => { + log::error!("commit-msg hook error: {}", e); + self.queue.push(InternalEvent::ShowErrorMsg( + format!("commit-msg hook error:\n{e}"), + )); + return Ok(CommitResult::Aborted); + } + HookResult::TimedOut => { + log::error!("commit-msg hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "commit-msg hook timed out after {} seconds", + self.get_hook_timeout().as_secs() + ), + )); + return Ok(CommitResult::Aborted); + } + HookResult::Ok => {} } } self.do_commit(&msg)?; - if let HookResult::NotOk(e) = - sync::hooks_post_commit(&self.repo.borrow())? - { - log::error!("post-commit hook error: {}", e); - self.queue.push(InternalEvent::ShowErrorMsg(format!( - "post-commit hook error:\n{e}" - ))); + match sync::hooks_post_commit_with_timeout( + &self.repo.borrow(), + self.get_hook_timeout(), + )? { + HookResult::NotOk(e) => { + log::error!("post-commit hook error: {}", e); + self.queue.push(InternalEvent::ShowErrorMsg( + format!("post-commit hook error:\n{e}"), + )); + } + HookResult::TimedOut => { + log::error!("post-commit hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "post-commit hook timed out after {} seconds", + self.get_hook_timeout().as_secs() + ), + )); + } + HookResult::Ok => {} } Ok(CommitResult::CommitDone) @@ -441,11 +484,13 @@ impl CommitPopup { self.mode = mode; let mut msg = self.input.get_text().to_string(); - if let HookResult::NotOk(e) = sync::hooks_prepare_commit_msg( - &self.repo.borrow(), - msg_source, - &mut msg, - )? { + if let HookResult::NotOk(e) = + sync::hooks_prepare_commit_msg_with_timeout( + &self.repo.borrow(), + msg_source, + &mut msg, + self.get_hook_timeout(), + )? { log::error!("prepare-commit-msg hook rejection: {e}",); } self.input.set_text(msg); @@ -477,6 +522,13 @@ impl CommitPopup { Ok(msg) } + + fn get_hook_timeout(&self) -> Duration { + self.options + .borrow() + .hook_timeout() + .unwrap_or(Duration::ZERO) + } } impl DrawableComponent for CommitPopup { diff --git a/src/popups/options.rs b/src/popups/options.rs index e74e8bdc0b..c2e76fadc4 100644 --- a/src/popups/options.rs +++ b/src/popups/options.rs @@ -1,3 +1,5 @@ +use std::time::Duration; + use crate::{ app::Environment, components::{ @@ -27,6 +29,7 @@ pub enum AppOption { DiffIgnoreWhitespaces, DiffContextLines, DiffInterhunkLines, + HookTimeout, } pub struct OptionsPopup { @@ -99,6 +102,19 @@ impl OptionsPopup { &diff.interhunk_lines.to_string(), self.is_select(AppOption::DiffInterhunkLines), ); + Self::add_header(txt, ""); + + Self::add_header(txt, "Hooks"); + self.add_entry( + txt, + width, + "Timeout", + &self.options.borrow().hook_timeout().map_or_else( + || "None".to_string(), + |d| format!("{d:?}"), + ), + self.is_select(AppOption::HookTimeout), + ); } fn is_select(&self, kind: AppOption) -> bool { @@ -138,7 +154,7 @@ impl OptionsPopup { if up { self.selection = match self.selection { AppOption::StatusShowUntracked => { - AppOption::DiffInterhunkLines + AppOption::HookTimeout } AppOption::DiffIgnoreWhitespaces => { AppOption::StatusShowUntracked @@ -149,6 +165,9 @@ impl OptionsPopup { AppOption::DiffInterhunkLines => { AppOption::DiffContextLines } + AppOption::HookTimeout => { + AppOption::DiffInterhunkLines + } }; } else { self.selection = match self.selection { @@ -162,6 +181,9 @@ impl OptionsPopup { AppOption::DiffInterhunkLines } AppOption::DiffInterhunkLines => { + AppOption::HookTimeout + } + AppOption::HookTimeout => { AppOption::StatusShowUntracked } }; @@ -207,6 +229,14 @@ impl OptionsPopup { .borrow_mut() .diff_hunk_lines_change(true); } + AppOption::HookTimeout => { + let current = + self.options.borrow().hook_timeout(); + let inc = Duration::from_secs(1); + let new = current.map(|d| d + inc).or(Some(inc)); + + self.options.borrow_mut().set_hook_timeout(new); + } } } else { match self.selection { @@ -246,6 +276,21 @@ impl OptionsPopup { .borrow_mut() .diff_hunk_lines_change(false); } + AppOption::HookTimeout => { + let current = + self.options.borrow().hook_timeout(); + let dec = Duration::from_secs(1); + + let new = current.and_then(|d| { + if d > dec { + Some(d - dec) + } else { + None + } + }); + + self.options.borrow_mut().set_hook_timeout(new); + } } } @@ -257,7 +302,7 @@ impl OptionsPopup { impl DrawableComponent for OptionsPopup { fn draw(&self, f: &mut Frame, area: Rect) -> Result<()> { if self.is_visible() { - const SIZE: (u16, u16) = (50, 10); + const SIZE: (u16, u16) = (50, 12); let area = ui::centered_rect_absolute(SIZE.0, SIZE.1, area);