From c25b809ad6e94cd133bbd04cf9056280f950e249 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Wed, 5 Mar 2025 23:30:27 +1100 Subject: [PATCH] feat(asyncgit): add timeouts for hooks --- CHANGELOG.md | 1 + Cargo.lock | 23 ++- asyncgit/src/sync/hooks.rs | 223 ++++++++++++++++++++- asyncgit/src/sync/mod.rs | 7 +- git2-hooks/Cargo.toml | 1 + git2-hooks/src/hookspath.rs | 385 +++++++++++++++++++++++++++++------- git2-hooks/src/lib.rs | 176 ++++++++++++++--- src/app.rs | 1 + src/options.rs | 13 ++ src/popups/commit.rs | 117 ++++++++--- src/popups/options.rs | 49 ++++- 11 files changed, 861 insertions(+), 135 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e60180cdc5..57e2fb7a1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,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 error messages [[@acuteenvy](https://github.com/acuteenvy)] ([#2617](https://github.com/gitui-org/gitui/pull/2617)) diff --git a/Cargo.lock b/Cargo.lock index ecf304a412..f7c0b2e5aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -397,6 +397,12 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[package]] +name = "cfg_aliases" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "613afe47fcd5fac7ccf1db93babcb082c5994d996f20b8b159f2ad1658eb5724" + [[package]] name = "chacha20" version = "0.9.1" @@ -1140,6 +1146,7 @@ dependencies = [ "git2-testing", "gix-path", "log", + "nix", "pretty_assertions", "shellexpand", "tempfile", @@ -2188,9 +2195,9 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.169" +version = "0.2.172" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b5aba8db14291edd000dfcc4d620c7ebfb122c613afb886ca8803fa4e128a20a" +checksum = "d750af042f7ef4f724306de029d18836c26c1765a54a6a3f094cbd23a7267ffa" [[package]] name = "libgit2-sys" @@ -2343,6 +2350,18 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "nix" +version = "0.30.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "74523f3a35e05aba87a1d978330aef40f67b0304ac79c1c00b294c9830543db6" +dependencies = [ + "bitflags 2.9.1", + "cfg-if", + "cfg_aliases", + "libc", +] + [[package]] name = "notify" version = "8.0.0" diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 111b426259..6dad0d37c3 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,13 @@ pub enum HookResult { Ok, /// Hook returned error NotOk(String), + /// Hook timed out + TimedOut { + /// Stdout + stdout: String, + /// Stderr + stderr: String, + }, } impl From for HookResult { @@ -22,6 +30,11 @@ impl From for HookResult { stderr, .. } => Self::NotOk(format!("{stdout}{stderr}")), + git2_hooks::HookResult::TimedOut { + stdout, + stderr, + .. + } => Self::TimedOut { stdout, stderr }, } } } @@ -30,30 +43,66 @@ impl From for HookResult { pub fn hooks_commit_msg( repo_path: &RepoPath, msg: &mut String, +) -> Result { + hooks_commit_msg_with_timeout(repo_path, msg, None) +} + +/// see `git2_hooks::hooks_commit_msg` +#[allow(unused)] +pub fn hooks_commit_msg_with_timeout( + repo_path: &RepoPath, + msg: &mut String, + timeout: Option, ) -> Result { scope_time!("hooks_commit_msg"); let repo = repo(repo_path)?; - - Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into()) + 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 { + hooks_pre_commit_with_timeout(repo_path, None) +} + +/// see `git2_hooks::hooks_pre_commit` +#[allow(unused)] +pub fn hooks_pre_commit_with_timeout( + repo_path: &RepoPath, + timeout: Option, +) -> Result { scope_time!("hooks_pre_commit"); let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into()) + 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 { + hooks_post_commit_with_timeout(repo_path, None) +} + +/// see `git2_hooks::hooks_post_commit` +#[allow(unused)] +pub fn hooks_post_commit_with_timeout( + repo_path: &RepoPath, + timeout: Option, +) -> Result { scope_time!("hooks_post_commit"); let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_post_commit(&repo, None)?.into()) + Ok(git2_hooks::hooks_post_commit_with_timeout( + &repo, None, timeout, + )? + .into()) } /// see `git2_hooks::hooks_prepare_commit_msg` @@ -66,8 +115,26 @@ pub fn hooks_prepare_commit_msg( let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_prepare_commit_msg( - &repo, None, source, msg, + Ok(git2_hooks::hooks_prepare_commit_msg_with_timeout( + &repo, None, source, msg, None, + )? + .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: Option, +) -> 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()) } @@ -77,7 +144,7 @@ mod tests { use std::{ffi::OsString, io::Write as _, path::Path}; use git2::Repository; - use tempfile::TempDir; + use tempfile::{tempdir, TempDir}; use super::*; use crate::sync::tests::repo_init_with_prefix; @@ -125,7 +192,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 "; @@ -239,4 +306,144 @@ 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(), + Some(Duration::from_millis(200)), + ) + .unwrap(); + + assert_eq!( + res, + HookResult::TimedOut { + stdout: String::new(), + stderr: String::new() + } + ); + } + + #[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(), + Some(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(), + Some(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 5 +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(), + Some(Duration::from_millis(100)), + ); + + assert!(res.is_ok()); + assert!(!file.exists()); + } + + #[test] + #[cfg(unix)] + fn test_ensure_group_kill_works() { + let (_td, repo) = repo_init().unwrap(); + let root = repo.path().parent().unwrap(); + + let hook = b"#!/usr/bin/env sh +sleep 30 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_COMMIT, + hook, + ); + + let time_start = std::time::Instant::now(); + let res = hooks_pre_commit_with_timeout( + &root.to_str().unwrap().into(), + Some(Duration::from_millis(150)), + ) + .unwrap(); + let time_end = std::time::Instant::now(); + let elapsed = time_end.duration_since(time_start); + + log::info!("elapsed: {:?}", elapsed); + // If the children didn't get killed this would + // have taken the full 30 seconds. + assert!(elapsed.as_secs() < 15); + assert!(matches!(res, HookResult::TimedOut { .. })) + } } diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index 09d4e6ef53..f608026241 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/Cargo.toml b/git2-hooks/Cargo.toml index 6bdd0497a9..717702fdd1 100644 --- a/git2-hooks/Cargo.toml +++ b/git2-hooks/Cargo.toml @@ -18,6 +18,7 @@ gix-path = "0.10" log = "0.4" shellexpand = "3.1" thiserror = "2.0" +nix = { version = "0.30.1", features = ["process", "signal"], default-features = false } [dev-dependencies] git2-testing = { path = "../git2-testing" } diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index fc4db5c87a..fe923358e4 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -4,9 +4,21 @@ use crate::{error::Result, HookResult, HooksError}; use std::{ ffi::{OsStr, OsString}, + io::Read, path::{Path, PathBuf}, - process::Command, + process::{Child, Command, Stdio}, str::FromStr, + thread, + time::Duration, +}; + +#[cfg(unix)] +use { + nix::{ + sys::signal::{killpg, SIGKILL}, + unistd::Pid, + }, + std::os::unix::process::CommandExt as _, }; pub struct HookPaths { @@ -134,96 +146,282 @@ impl HookPaths { /// this function calls hook scripts based on conventions documented here /// see + #[inline] + #[allow(unused)] pub fn run_hook(&self, args: &[&str]) -> Result { self.run_hook_os_str(args) } /// this function calls hook scripts based on conventions documented here /// see + #[inline] pub fn run_hook_os_str(&self, args: I) -> Result + where + I: IntoIterator + Copy, + S: AsRef, + { + self.run_hook_with_timeout_os_str(args, None) + } + + /// 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 + #[inline] + pub fn run_hook_with_timeout( + &self, + args: &[&str], + timeout: Option, + ) -> Result { + self.run_hook_with_timeout_os_str(args, timeout) + } + + /// 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_os_str( + &self, + args: I, + timeout: Option, + ) -> Result where I: IntoIterator + Copy, S: AsRef, { let hook = self.hook.clone(); - log::trace!("run hook '{:?}' in '{:?}'", hook, self.pwd); - - let run_command = |command: &mut Command| { - command - .args(args) - .current_dir(&self.pwd) - .with_no_window() - .output() - }; + let mut child = spawn_hook_process(&self.pwd, &hook, args)?; - 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); + let output = if timeout.is_none() + || timeout.is_some_and(|t| t.is_zero()) + { + child.wait_with_output()? + } else { + let timeout = timeout.unwrap(); + if !timeout_with_quadratic_backoff(timeout, || { + Ok(child.try_wait()?.is_some()) + })? { + if cfg!(unix) { + match i32::try_from(child.id()) { + Ok(pid) => { + killpg(Pid::from_raw(pid), SIGKILL) + .expect("killpg failed"); } + Err(_) => child.kill()?, } - - os_str.push(hook.as_os_str()); + } else { + child.kill()?; } - os_str.push("'"); - os_str.push(" \"$@\""); - os_str - }; - run_command( - sh_command().arg("-c").arg(command).arg(&hook), - ) - } 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)) + let mut stdout = String::new(); + let mut stderr = String::new(); + if let Some(mut pipe) = child.stdout { + pipe.read_to_string(&mut stdout)?; + } + if let Some(mut pipe) = child.stderr { + pipe.read_to_string(&mut stderr)?; } - result => result, + + return Ok(HookResult::TimedOut { + hook, + stdout, + stderr, + }); } - }?; - if output.status.success() { - Ok(HookResult::Ok { hook }) - } 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, - }) + child.wait_with_output()? + }; + + Ok(hook_result_from_output(hook, &output)) + } +} + +/// This will loop, sleeping with quadratically increasing time until completion or timeout has been reached. +/// +/// Formula: +/// Base Duration: `TIMESCALE` is set to 1 millisecond. +/// Max Sleep Duration: `MAX_SLEEP` is set to 50 milliseconds. +/// Quadratic Calculation: Sleep time = (attempt^2) * `TIMESCALE`, capped by `MAX_SLEEP`. +/// +/// 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` +/// 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 TIMESCALE: Duration = Duration::from_millis(1); + const MAX_SLEEP: Duration = Duration::from_millis(50); + + let timer = std::time::Instant::now(); + let mut attempt: u32 = 1; + + while !is_complete()? { + let Some(remaining_time) = + timeout.checked_sub(timer.elapsed()) + else { + return Ok(false); + }; + + let sleep_time = TIMESCALE + .saturating_mul(attempt.saturating_pow(2)) + .min(MAX_SLEEP) + .min(remaining_time); + + thread::sleep(sleep_time); + attempt += 1; + } + + Ok(true) +} + +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: I, +) -> Result +where + I: IntoIterator + Copy, + S: AsRef, +{ + log::trace!("run hook '{:?}' in '{:?}'", hook, directory); + + let spawn_command = |command: &mut Command| { + if cfg!(unix) { + command.process_group(0); + } + + command + .args(args) + .current_dir(directory) + .with_no_window() + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::piped()) + .spawn() + }; + + let child = 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 + }; + spawn_command(sh_command().arg("-c").arg(command).arg(hook)) + } else { + // execute hook directly + match spawn_command(&mut Command::new(hook)) { + Err(err) if err.raw_os_error() == Some(ENOEXEC) => { + spawn_command(sh_command().arg(hook)) + } + result => result, + } + }?; + + Ok(child) +} + fn sh_command() -> Command { let mut command = Command::new(gix_path::env::shell()); @@ -300,7 +498,8 @@ impl CommandExt for Command { #[cfg(test)] mod test { - use super::HookPaths; + use super::*; + use pretty_assertions::assert_eq; use std::path::Path; #[test] @@ -328,4 +527,54 @@ 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(); + // Attempt 3 = 14ms so we want to ensure we didn't pass it. + assert!(elapsed.as_millis() < 13); + } + + /// 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 4c949a1e46..4e6e510437 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; @@ -59,7 +60,16 @@ pub enum HookResult { RunNotSuccessful { /// exit code as reported back from process calling the hook code: Option, + /// stdout output emitted by hook + stdout: String, /// stderr output emitted by hook + stderr: String, + /// path of the hook that was run + hook: PathBuf, + }, + /// Hook took too long to execute and was killed + TimedOut { + /// stdout output emitted by hook stdout: String, /// stderr output emitted by hook stderr: String, @@ -78,6 +88,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 +127,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,16 +148,25 @@ pub fn hooks_commit_msg( other_paths: Option<&[&str]>, msg: &mut String, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_COMMIT_MSG)?; + hooks_commit_msg_with_timeout(repo, other_paths, msg, None) +} - if !hook.found() { - return Ok(HookResult::NoHookFound); - } +/// 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: Option, +) -> 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_os_str([&temp_file])?; + let res = + hook.run_hook_with_timeout_os_str(&temp_file, timeout)?; // load possibly altered msg msg.clear(); @@ -146,13 +180,18 @@ pub fn hooks_pre_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_PRE_COMMIT)?; + hooks_pre_commit_with_timeout(repo, other_paths, None) +} - if !hook.found() { - return Ok(HookResult::NoHookFound); - } +/// this hook is documented here +pub fn hooks_pre_commit_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + timeout: Option, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); - hook.run_hook(&[]) + hook.run_hook_with_timeout(&[], timeout) } /// this hook is documented here @@ -160,15 +199,21 @@ pub fn hooks_post_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_POST_COMMIT)?; + hooks_post_commit_with_timeout(repo, other_paths, None) +} - if !hook.found() { - return Ok(HookResult::NoHookFound); - } +/// this hook is documented here +pub fn hooks_post_commit_with_timeout( + repo: &Repository, + other_paths: Option<&[&str]>, + timeout: Option, +) -> Result { + let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); - hook.run_hook(&[]) + hook.run_hook_with_timeout(&[], timeout) } +#[derive(Clone, Copy)] pub enum PrepareCommitMsgSource { Message, Template, @@ -185,12 +230,24 @@ pub fn hooks_prepare_commit_msg( source: PrepareCommitMsgSource, msg: &mut String, ) -> Result { - let hook = - HookPaths::new(repo, other_paths, HOOK_PREPARE_COMMIT_MSG)?; + hooks_prepare_commit_msg_with_timeout( + repo, + other_paths, + source, + msg, + None, + ) +} - if !hook.found() { - return Ok(HookResult::NoHookFound); - } +/// 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: Option, +) -> 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())?; @@ -219,7 +276,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(); @@ -233,7 +290,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() { @@ -372,7 +429,7 @@ exit 0 } #[test] - fn test_other_path_precendence() { + fn test_other_path_precedence() { let (td, repo) = repo_init(); { @@ -493,7 +550,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 @@ -514,7 +571,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 @@ -657,4 +714,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, + Some(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, + Some(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, + Some(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 0fafe64151..6991f09cce 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..3ce23d52bb 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,32 @@ 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 { stdout, stderr } => { + log::error!("pre-commit hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "pre-commit hook timed out after {} seconds, see output below.\n{}\n{}", + self.get_hook_timeout() + .unwrap_or(Duration::ZERO) + .as_secs(), + stdout, + stderr + ), + )); + return Ok(CommitResult::Aborted); + } + HookResult::Ok => {} } } @@ -253,25 +272,61 @@ 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 { stdout, stderr } => { + log::error!("commit-msg hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "commit-msg hook timed out after {} seconds, see output below.\n{}\n{}", + self.get_hook_timeout() + .unwrap_or(Duration::ZERO) + .as_secs(), + stdout, + stderr + ), + )); + 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 { stdout, stderr } => { + log::error!("post-commit hook timed out"); + self.queue.push(InternalEvent::ShowErrorMsg( + format!( + "post-commit hook timed out after {} seconds, see output below.\n{}\n{}", + self.get_hook_timeout() + .unwrap_or(Duration::ZERO) + .as_secs(), + stdout, + stderr + ), + )); + } + HookResult::Ok => {} } Ok(CommitResult::CommitDone) @@ -441,11 +496,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 +534,10 @@ impl CommitPopup { Ok(msg) } + + fn get_hook_timeout(&self) -> Option { + self.options.borrow().hook_timeout() + } } 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);