From 08eaefe660079f5c6cad84459be1059211ec6890 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Wed, 5 Mar 2025 23:30:27 +1100 Subject: [PATCH 1/8] feat(asyncgit): add timeouts for hooks --- asyncgit/src/sync/hooks.rs | 199 +++++++++++++++++++++++++++++++++---- src/popups/commit.rs | 24 +++-- 2 files changed, 192 insertions(+), 31 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 6fc672134f..7fdd3a739e 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -2,6 +2,10 @@ use super::{repository::repo, RepoPath}; use crate::error::Result; pub use git2_hooks::PrepareCommitMsgSource; use scopetime::scope_time; +use std::{ + sync::mpsc::{channel, RecvTimeoutError}, + time::Duration, +}; /// #[derive(Debug, PartialEq, Eq)] @@ -26,34 +30,112 @@ impl From for HookResult { } } +fn run_with_timeout( + f: F, + timeout: Duration, +) -> Result<(HookResult, Option)> +where + F: FnOnce() -> Result<(HookResult, Option)> + + Send + + Sync + + 'static, +{ + if timeout.is_zero() { + return f(); // Don't bother with threads if we don't have a timeout + } + + let (tx, rx) = channel(); + let _ = std::thread::spawn(move || { + let result = f(); + tx.send(result) + }); + + match rx.recv_timeout(timeout) { + Ok(result) => result, + Err(RecvTimeoutError::Timeout) => Ok(( + HookResult::NotOk("hook timed out".to_string()), + None, + )), + Err(RecvTimeoutError::Disconnected) => { + unreachable!() + } + } +} + /// see `git2_hooks::hooks_commit_msg` pub fn hooks_commit_msg( repo_path: &RepoPath, msg: &mut String, + timeout: Duration, ) -> Result { scope_time!("hooks_commit_msg"); - let repo = repo(repo_path)?; + let repo_path = repo_path.clone(); + let mut msg_clone = msg.clone(); + let (result, msg_opt) = run_with_timeout( + move || { + let repo = repo(&repo_path)?; + Ok(( + git2_hooks::hooks_commit_msg( + &repo, + None, + &mut msg_clone, + )? + .into(), + Some(msg_clone), + )) + }, + timeout, + )?; + + if let Some(updated_msg) = msg_opt { + msg.clear(); + msg.push_str(&updated_msg); + } - Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into()) + Ok(result) } /// see `git2_hooks::hooks_pre_commit` -pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { +pub fn hooks_pre_commit( + repo_path: &RepoPath, + timeout: Duration, +) -> Result { scope_time!("hooks_pre_commit"); - let repo = repo(repo_path)?; - - Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into()) + let repo_path = repo_path.clone(); + run_with_timeout( + move || { + let repo = repo(&repo_path)?; + Ok(( + git2_hooks::hooks_pre_commit(&repo, None)?.into(), + None, + )) + }, + timeout, + ) + .map(|res| res.0) } /// see `git2_hooks::hooks_post_commit` -pub fn hooks_post_commit(repo_path: &RepoPath) -> Result { +pub fn hooks_post_commit( + repo_path: &RepoPath, + timeout: Duration, +) -> Result { scope_time!("hooks_post_commit"); - let repo = repo(repo_path)?; - - Ok(git2_hooks::hooks_post_commit(&repo, None)?.into()) + let repo_path = repo_path.clone(); + run_with_timeout( + move || { + let repo = repo(&repo_path)?; + Ok(( + git2_hooks::hooks_post_commit(&repo, None)?.into(), + None, + )) + }, + timeout, + ) + .map(|res| res.0) } /// see `git2_hooks::hooks_prepare_commit_msg` @@ -61,15 +143,35 @@ pub fn hooks_prepare_commit_msg( repo_path: &RepoPath, source: PrepareCommitMsgSource, msg: &mut String, + timeout: Duration, ) -> Result { scope_time!("hooks_prepare_commit_msg"); - let repo = repo(repo_path)?; + let repo_path = repo_path.clone(); + let mut msg_cloned = msg.clone(); + let (result, msg_opt) = run_with_timeout( + move || { + let repo = repo(&repo_path)?; + Ok(( + git2_hooks::hooks_prepare_commit_msg( + &repo, + None, + source, + &mut msg_cloned, + )? + .into(), + Some(msg_cloned), + )) + }, + timeout, + )?; + + if let Some(updated_msg) = msg_opt { + msg.clear(); + msg.push_str(&updated_msg); + } - Ok(git2_hooks::hooks_prepare_commit_msg( - &repo, None, source, msg, - )? - .into()) + Ok(result) } #[cfg(test)] @@ -99,7 +201,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 "; @@ -113,9 +215,11 @@ 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.to_str().unwrap().into(), + Duration::ZERO, + ) + .unwrap(); assert_eq!( res, @@ -136,7 +240,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 "; @@ -146,7 +250,8 @@ mod tests { git2_hooks::HOOK_PRE_COMMIT, hook, ); - let res = hooks_pre_commit(repo_path).unwrap(); + let res = + hooks_pre_commit(repo_path, Duration::ZERO).unwrap(); if let HookResult::NotOk(res) = res { assert_eq!( std::path::Path::new(res.trim_end()), @@ -162,7 +267,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 @@ -181,6 +286,7 @@ mod tests { let res = hooks_commit_msg( &subfolder.to_str().unwrap().into(), &mut msg, + Duration::ZERO, ) .unwrap(); @@ -224,4 +330,53 @@ 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 1 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_COMMIT_MSG, + hook, + ); + + let res = hooks_pre_commit( + &root.to_str().unwrap().into(), + Duration::ZERO, + ) + .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_COMMIT_MSG, + hook, + ); + + let res = hooks_commit_msg( + &root.to_str().unwrap().into(), + &mut String::new(), + Duration::ZERO, + ) + .unwrap(); + + assert_eq!(res, HookResult::Ok); + } } diff --git a/src/popups/commit.rs b/src/popups/commit.rs index 008fc6f8a7..ff91227df2 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,9 +238,10 @@ impl CommitPopup { if verify { // run pre commit hook - can reject commit - if let HookResult::NotOk(e) = - sync::hooks_pre_commit(&self.repo.borrow())? - { + if let HookResult::NotOk(e) = sync::hooks_pre_commit( + &self.repo.borrow(), + Duration::ZERO, + )? { log::error!("pre-commit hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg( format!("pre-commit hook error:\n{e}"), @@ -253,9 +255,11 @@ 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)? - { + if let HookResult::NotOk(e) = sync::hooks_commit_msg( + &self.repo.borrow(), + &mut msg, + Duration::ZERO, + )? { log::error!("commit-msg hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg( format!("commit-msg hook error:\n{e}"), @@ -265,9 +269,10 @@ impl CommitPopup { } self.do_commit(&msg)?; - if let HookResult::NotOk(e) = - sync::hooks_post_commit(&self.repo.borrow())? - { + if let HookResult::NotOk(e) = sync::hooks_post_commit( + &self.repo.borrow(), + Duration::ZERO, + )? { log::error!("post-commit hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg(format!( "post-commit hook error:\n{e}" @@ -445,6 +450,7 @@ impl CommitPopup { &self.repo.borrow(), msg_source, &mut msg, + Duration::ZERO, )? { log::error!("prepare-commit-msg hook rejection: {e}",); } From ae55b0949c86ea0255edc7ea6c2831323339f310 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 16 Mar 2025 14:14:55 +1100 Subject: [PATCH 2/8] fix(asyncgit/test): use correct hook path and add non zero timeout test --- asyncgit/src/sync/hooks.rs | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 7fdd3a739e..7e2cf30fb6 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -337,18 +337,45 @@ mod tests { let root = repo.path().parent().unwrap(); let hook = b"#!/usr/bin/env sh - sleep 1 + sleep 0.21 "; git2_hooks::create_hook( &repo, - git2_hooks::HOOK_COMMIT_MSG, + git2_hooks::HOOK_PRE_COMMIT, hook, ); let res = hooks_pre_commit( &root.to_str().unwrap().into(), - Duration::ZERO, + Duration::from_millis(200), + ) + .unwrap(); + + assert_eq!( + res, + HookResult::NotOk("hook timed out".to_string()) + ); + } + + #[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( + &root.to_str().unwrap().into(), + Duration::from_millis(110), ) .unwrap(); @@ -366,13 +393,12 @@ mod tests { git2_hooks::create_hook( &repo, - git2_hooks::HOOK_COMMIT_MSG, + git2_hooks::HOOK_POST_COMMIT, hook, ); - let res = hooks_commit_msg( + let res = hooks_post_commit( &root.to_str().unwrap().into(), - &mut String::new(), Duration::ZERO, ) .unwrap(); From f619e3efbb5442464b710dcf9d8862f8391097fa Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 16 Mar 2025 16:59:24 +1100 Subject: [PATCH 3/8] chore: fix minor spelling mistakes --- git2-hooks/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 2a458856d7..5cd3b5983a 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -340,7 +340,7 @@ exit 0 } #[test] - fn test_other_path_precendence() { + fn test_other_path_precedence() { let (td, repo) = repo_init(); { @@ -456,7 +456,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 +477,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 From 64979e574cbbc792c66ce5f8813fc5ef99b832df Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 16 Mar 2025 17:28:23 +1100 Subject: [PATCH 4/8] refactor: move timeout logic to git2-hooks & add new functions for timeouts All hooks functions now have their own with_timeout variant inside git2-hooks and asyncgit. The previous implementation of using a new thread with a timeout has been ditched in favour of a child command which we can now poll and kill to prevent the hook from continueing. I've added a hard coded default 5 second timeout but this will be handled by the Options struct later --- asyncgit/src/sync/hooks.rs | 242 ++++++++++++++++++------------------ asyncgit/src/sync/mod.rs | 7 +- git2-hooks/src/hookspath.rs | 35 +++++- git2-hooks/src/lib.rs | 144 +++++++++++++++++---- src/popups/commit.rs | 48 ++++--- 5 files changed, 304 insertions(+), 172 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 7e2cf30fb6..ef8306509e 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -2,10 +2,7 @@ use super::{repository::repo, RepoPath}; use crate::error::Result; pub use git2_hooks::PrepareCommitMsgSource; use scopetime::scope_time; -use std::{ - sync::mpsc::{channel, RecvTimeoutError}, - time::Duration, -}; +use std::time::Duration; /// #[derive(Debug, PartialEq, Eq)] @@ -14,6 +11,8 @@ pub enum HookResult { Ok, /// Hook returned error NotOk(String), + /// Hook timed out + TimedOut, } impl From for HookResult { @@ -26,116 +25,87 @@ impl From for HookResult { stderr, .. } => Self::NotOk(format!("{stdout}{stderr}")), + git2_hooks::HookResult::TimedOut { .. } => Self::TimedOut, } } } -fn run_with_timeout( - f: F, - timeout: Duration, -) -> Result<(HookResult, Option)> -where - F: FnOnce() -> Result<(HookResult, Option)> - + Send - + Sync - + 'static, -{ - if timeout.is_zero() { - return f(); // Don't bother with threads if we don't have a timeout - } +/// see `git2_hooks::hooks_commit_msg` +pub fn hooks_commit_msg( + repo_path: &RepoPath, + msg: &mut String, +) -> Result { + scope_time!("hooks_commit_msg"); - let (tx, rx) = channel(); - let _ = std::thread::spawn(move || { - let result = f(); - tx.send(result) - }); - - match rx.recv_timeout(timeout) { - Ok(result) => result, - Err(RecvTimeoutError::Timeout) => Ok(( - HookResult::NotOk("hook timed out".to_string()), - None, - )), - Err(RecvTimeoutError::Disconnected) => { - unreachable!() - } - } + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_commit_msg(&repo, None, msg)?.into()) } -/// see `git2_hooks::hooks_commit_msg` -pub fn hooks_commit_msg( +/// 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_commit_msg"); + scope_time!("hooks_prepare_commit_msg"); - let repo_path = repo_path.clone(); - let mut msg_clone = msg.clone(); - let (result, msg_opt) = run_with_timeout( - move || { - let repo = repo(&repo_path)?; - Ok(( - git2_hooks::hooks_commit_msg( - &repo, - None, - &mut msg_clone, - )? - .into(), - Some(msg_clone), - )) - }, - timeout, - )?; - - if let Some(updated_msg) = msg_opt { - msg.clear(); - msg.push_str(&updated_msg); - } + let repo = repo(repo_path)?; + Ok(git2_hooks::hooks_commit_msg_with_timeout( + &repo, None, msg, timeout, + )? + .into()) +} - Ok(result) +/// see `git2_hooks::hooks_pre_commit` +pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result { + scope_time!("hooks_pre_commit"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_pre_commit(&repo, None)?.into()) } /// see `git2_hooks::hooks_pre_commit` -pub fn 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_path = repo_path.clone(); - run_with_timeout( - move || { - let repo = repo(&repo_path)?; - Ok(( - git2_hooks::hooks_pre_commit(&repo, None)?.into(), - None, - )) - }, - timeout, - ) - .map(|res| res.0) + 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"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_post_commit(&repo, None)?.into()) } /// see `git2_hooks::hooks_post_commit` -pub fn 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_path = repo_path.clone(); - run_with_timeout( - move || { - let repo = repo(&repo_path)?; - Ok(( - git2_hooks::hooks_post_commit(&repo, None)?.into(), - None, - )) - }, - timeout, - ) - .map(|res| res.0) + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_post_commit_with_timeout( + &repo, None, timeout, + )? + .into()) } /// see `git2_hooks::hooks_prepare_commit_msg` @@ -143,39 +113,39 @@ pub fn hooks_prepare_commit_msg( repo_path: &RepoPath, source: PrepareCommitMsgSource, msg: &mut String, +) -> Result { + scope_time!("hooks_prepare_commit_msg"); + + let repo = repo(repo_path)?; + + Ok(git2_hooks::hooks_prepare_commit_msg( + &repo, None, source, 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_path = repo_path.clone(); - let mut msg_cloned = msg.clone(); - let (result, msg_opt) = run_with_timeout( - move || { - let repo = repo(&repo_path)?; - Ok(( - git2_hooks::hooks_prepare_commit_msg( - &repo, - None, - source, - &mut msg_cloned, - )? - .into(), - Some(msg_cloned), - )) - }, - timeout, - )?; - - if let Some(updated_msg) = msg_opt { - msg.clear(); - msg.push_str(&updated_msg); - } + let repo = repo(repo_path)?; - Ok(result) + 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; @@ -215,11 +185,9 @@ mod tests { let subfolder = root.join("foo/"); std::fs::create_dir_all(&subfolder).unwrap(); - let res = hooks_post_commit( - &subfolder.to_str().unwrap().into(), - Duration::ZERO, - ) - .unwrap(); + let res = + hooks_post_commit(&subfolder.to_str().unwrap().into()) + .unwrap(); assert_eq!( res, @@ -250,8 +218,7 @@ mod tests { git2_hooks::HOOK_PRE_COMMIT, hook, ); - let res = - hooks_pre_commit(repo_path, Duration::ZERO).unwrap(); + let res = hooks_pre_commit(repo_path).unwrap(); if let HookResult::NotOk(res) = res { assert_eq!( std::path::Path::new(res.trim_end()), @@ -286,7 +253,6 @@ mod tests { let res = hooks_commit_msg( &subfolder.to_str().unwrap().into(), &mut msg, - Duration::ZERO, ) .unwrap(); @@ -346,16 +312,13 @@ mod tests { hook, ); - let res = hooks_pre_commit( + let res = hooks_pre_commit_with_timeout( &root.to_str().unwrap().into(), Duration::from_millis(200), ) .unwrap(); - assert_eq!( - res, - HookResult::NotOk("hook timed out".to_string()) - ); + assert_eq!(res, HookResult::TimedOut); } #[test] @@ -373,7 +336,7 @@ mod tests { hook, ); - let res = hooks_pre_commit( + let res = hooks_pre_commit_with_timeout( &root.to_str().unwrap().into(), Duration::from_millis(110), ) @@ -397,7 +360,7 @@ mod tests { hook, ); - let res = hooks_post_commit( + let res = hooks_post_commit_with_timeout( &root.to_str().unwrap().into(), Duration::ZERO, ) @@ -405,4 +368,35 @@ mod tests { 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..e907810e6f 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -1,12 +1,14 @@ use git2::Repository; +use log::debug; use crate::{error::Result, HookResult, HooksError}; use std::{ env, path::{Path, PathBuf}, - process::Command, + process::{Command, Stdio}, str::FromStr, + time::Duration, }; pub struct HookPaths { @@ -133,7 +135,11 @@ impl HookPaths { /// this function calls hook scripts based on conventions documented here /// see - pub fn run_hook(&self, args: &[&str]) -> Result { + pub fn run_hook( + &self, + args: &[&str], + timeout: Duration, + ) -> Result { let hook = self.hook.clone(); let arg_str = format!("{:?} {}", hook, args.join(" ")); @@ -146,7 +152,7 @@ impl HookPaths { let git_shell = find_bash_executable() .or_else(find_default_unix_shell) .unwrap_or_else(|| "bash".into()); - let output = Command::new(git_shell) + let mut child = Command::new(git_shell) .args(bash_args) .with_no_window() .current_dir(&self.pwd) @@ -157,7 +163,28 @@ impl HookPaths { "DUMMY_ENV_TO_FIX_WINDOWS_CMD_RUNS", "FixPathHandlingOnWindows", ) - .output()?; + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .stdin(Stdio::piped()) + .spawn()?; + + let output = if timeout.is_zero() { + child.wait_with_output()? + } else { + let timer = std::time::Instant::now(); + while child.try_wait()?.is_none() { + if timer.elapsed() > timeout { + debug!("killing hook process"); + child.kill()?; + return Ok(HookResult::TimedOut { hook }); + } + + std::thread::yield_now(); + std::thread::sleep(Duration::from_millis(10)); + } + + child.wait_with_output()? + }; if output.status.success() { Ok(HookResult::Ok { hook }) diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 5cd3b5983a..db5084eeb0 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,10 @@ pub enum HookResult { /// path of the hook that was run hook: PathBuf, }, + TimedOut { + /// path of the hook that was run + hook: PathBuf, + }, } impl HookResult { @@ -78,6 +83,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 +122,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,19 +143,41 @@ pub fn hooks_commit_msg( other_paths: Option<&[&str]>, msg: &mut String, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_COMMIT_MSG)?; + let hook = find_hook!(repo, other_paths, HOOK_COMMIT_MSG); - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + 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()], + Duration::ZERO, + )?; + + // load possibly altered msg + msg.clear(); + File::open(temp_file)?.read_to_string(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(&[temp_file - .as_os_str() - .to_string_lossy() - .as_ref()])?; + let res = hook.run_hook( + &[temp_file.as_os_str().to_string_lossy().as_ref()], + timeout, + )?; // load possibly altered msg msg.clear(); @@ -149,13 +191,20 @@ pub fn hooks_pre_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_PRE_COMMIT)?; + let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + hook.run_hook(&[], Duration::ZERO) +} + +/// 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(&[]) + hook.run_hook(&[], timeout) } /// this hook is documented here @@ -163,15 +212,23 @@ pub fn hooks_post_commit( repo: &Repository, other_paths: Option<&[&str]>, ) -> Result { - let hook = HookPaths::new(repo, other_paths, HOOK_POST_COMMIT)?; + let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); - if !hook.found() { - return Ok(HookResult::NoHookFound); - } + hook.run_hook(&[], Duration::ZERO) +} + +/// 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(&[]) + hook.run_hook(&[], timeout) } +#[derive(Clone, Copy)] pub enum PrepareCommitMsgSource { Message, Template, @@ -188,13 +245,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); - if !hook.found() { - return Ok(HookResult::NoHookFound); + 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 let Some(id) = &id { + args.push(id); } + let res = hook.run_hook(args.as_slice(), Duration::ZERO)?; + + // 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 +320,7 @@ pub fn hooks_prepare_commit_msg( args.push(id); } - let res = hook.run_hook(args.as_slice())?; + let res = hook.run_hook(args.as_slice(), timeout)?; // load possibly altered msg msg.clear(); diff --git a/src/popups/commit.rs b/src/popups/commit.rs index ff91227df2..4dba9f8f23 100644 --- a/src/popups/commit.rs +++ b/src/popups/commit.rs @@ -238,10 +238,11 @@ impl CommitPopup { if verify { // run pre commit hook - can reject commit - if let HookResult::NotOk(e) = sync::hooks_pre_commit( - &self.repo.borrow(), - Duration::ZERO, - )? { + if let HookResult::NotOk(e) = + sync::hooks_pre_commit_with_timeout( + &self.repo.borrow(), + self.get_hook_timeout(), + )? { log::error!("pre-commit hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg( format!("pre-commit hook error:\n{e}"), @@ -255,11 +256,12 @@ 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, - Duration::ZERO, - )? { + if let HookResult::NotOk(e) = + sync::hooks_commit_msg_with_timeout( + &self.repo.borrow(), + &mut msg, + self.get_hook_timeout(), + )? { log::error!("commit-msg hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg( format!("commit-msg hook error:\n{e}"), @@ -269,10 +271,11 @@ impl CommitPopup { } self.do_commit(&msg)?; - if let HookResult::NotOk(e) = sync::hooks_post_commit( - &self.repo.borrow(), - Duration::ZERO, - )? { + if let HookResult::NotOk(e) = + sync::hooks_post_commit_with_timeout( + &self.repo.borrow(), + self.get_hook_timeout(), + )? { log::error!("post-commit hook error: {}", e); self.queue.push(InternalEvent::ShowErrorMsg(format!( "post-commit hook error:\n{e}" @@ -446,12 +449,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, - Duration::ZERO, - )? { + 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); @@ -483,6 +487,12 @@ impl CommitPopup { Ok(msg) } + + // TODO - Configurable timeout + #[allow(clippy::unused_self, clippy::missing_const_for_fn)] + fn get_hook_timeout(&self) -> Duration { + Duration::from_secs(5) + } } impl DrawableComponent for CommitPopup { From 4f8b5bab11ae1d38a80be7fef7fb9e3ca680a59c Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 16 Mar 2025 18:16:41 +1100 Subject: [PATCH 5/8] fix(test): relax time a bit --- asyncgit/src/sync/hooks.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index ef8306509e..b5310ab3be 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -303,7 +303,7 @@ mod tests { let root = repo.path().parent().unwrap(); let hook = b"#!/usr/bin/env sh - sleep 0.21 + sleep 0.250 "; git2_hooks::create_hook( @@ -338,7 +338,7 @@ mod tests { let res = hooks_pre_commit_with_timeout( &root.to_str().unwrap().into(), - Duration::from_millis(110), + Duration::from_millis(150), ) .unwrap(); From 2fe5f28ed74851ce7e0ba92c2e68926c3d721b22 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 16 Mar 2025 18:40:56 +1100 Subject: [PATCH 6/8] feat: implement option for hook timeout and abort the commit --- src/app.rs | 1 + src/options.rs | 13 ++++++ src/popups/commit.rs | 102 ++++++++++++++++++++++++++++-------------- src/popups/options.rs | 49 +++++++++++++++++++- 4 files changed, 130 insertions(+), 35 deletions(-) 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 4dba9f8f23..2b0e6d1970 100644 --- a/src/popups/commit.rs +++ b/src/popups/commit.rs @@ -238,16 +238,28 @@ impl CommitPopup { if verify { // run pre commit hook - can reject commit - if let HookResult::NotOk(e) = - sync::hooks_pre_commit_with_timeout( - &self.repo.borrow(), - self.get_hook_timeout(), - )? { - 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 => {} } } @@ -256,30 +268,53 @@ impl CommitPopup { if verify { // run commit message check hook - can reject commit - if let HookResult::NotOk(e) = - sync::hooks_commit_msg_with_timeout( - &self.repo.borrow(), - &mut msg, - self.get_hook_timeout(), - )? { - 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_with_timeout( - &self.repo.borrow(), - self.get_hook_timeout(), - )? { - 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) @@ -488,10 +523,11 @@ impl CommitPopup { Ok(msg) } - // TODO - Configurable timeout - #[allow(clippy::unused_self, clippy::missing_const_for_fn)] fn get_hook_timeout(&self) -> Duration { - Duration::from_secs(5) + self.options + .borrow() + .hook_timeout() + .unwrap_or(Duration::ZERO) } } 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); From 883c4250aa3956de552b17d394ac45ddced35590 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 23 Mar 2025 21:02:23 +1100 Subject: [PATCH 7/8] refactor: implement exponentional backoff for sleep duration. Also adds tests for this function and the usage of timeouts in git2-hooks --- asyncgit/src/sync/hooks.rs | 5 +- git2-hooks/src/hookspath.rs | 270 +++++++++++++++++++++++++++++------- git2-hooks/src/lib.rs | 124 +++++++++++++---- 3 files changed, 316 insertions(+), 83 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index b5310ab3be..7c273c621a 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -378,9 +378,8 @@ mod tests { let file = temp_dir.path().join("test"); let hook = format!( "#!/usr/bin/env sh - sleep 1 - - echo 'after sleep' > {} +sleep 1 +echo 'after sleep' > {} ", file.as_path().to_str().unwrap() ); diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index e907810e6f..cc48e5f822 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -6,8 +6,9 @@ use crate::{error::Result, HookResult, HooksError}; use std::{ env, path::{Path, PathBuf}, - process::{Command, Stdio}, + process::{Child, Command, Stdio}, str::FromStr, + thread, time::Duration, }; @@ -135,75 +136,188 @@ impl HookPaths { /// this function calls hook scripts based on conventions documented here /// see - pub fn run_hook( + 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()?; + + Ok(hook_result_from_output(hook, &output)) + } + + /// 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 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 mut child = 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", - ) - .stdout(Stdio::piped()) - .stderr(Stdio::piped()) - .stdin(Stdio::piped()) - .spawn()?; + let mut child = spawn_hook_process(&self.pwd, &hook, args)?; let output = if timeout.is_zero() { child.wait_with_output()? } else { - let timer = std::time::Instant::now(); - while child.try_wait()?.is_none() { - if timer.elapsed() > timeout { - debug!("killing hook process"); - child.kill()?; - return Ok(HookResult::TimedOut { hook }); - } - - std::thread::yield_now(); - std::thread::sleep(Duration::from_millis(10)); + 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()? }; - 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, - }) + 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; @@ -289,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] @@ -317,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 db5084eeb0..9db91416d1 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -67,6 +67,7 @@ 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, @@ -148,10 +149,10 @@ 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()], - Duration::ZERO, - )?; + let res = hook.run_hook(&[temp_file + .as_os_str() + .to_string_lossy() + .as_ref()])?; // load possibly altered msg msg.clear(); @@ -174,7 +175,7 @@ pub fn hooks_commit_msg_with_timeout( 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( + let res = hook.run_hook_with_timeout( &[temp_file.as_os_str().to_string_lossy().as_ref()], timeout, )?; @@ -193,7 +194,7 @@ pub fn hooks_pre_commit( ) -> Result { let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); - hook.run_hook(&[], Duration::ZERO) + hook.run_hook(&[]) } /// this hook is documented here @@ -204,7 +205,7 @@ pub fn hooks_pre_commit_with_timeout( ) -> Result { let hook = find_hook!(repo, other_paths, HOOK_PRE_COMMIT); - hook.run_hook(&[], timeout) + hook.run_hook_with_timeout(&[], timeout) } /// this hook is documented here @@ -214,7 +215,7 @@ pub fn hooks_post_commit( ) -> Result { let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); - hook.run_hook(&[], Duration::ZERO) + hook.run_hook(&[]) } /// this hook is documented here @@ -225,7 +226,7 @@ pub fn hooks_post_commit_with_timeout( ) -> Result { let hook = find_hook!(repo, other_paths, HOOK_POST_COMMIT); - hook.run_hook(&[], timeout) + hook.run_hook_with_timeout(&[], timeout) } #[derive(Clone, Copy)] @@ -274,7 +275,7 @@ pub fn hooks_prepare_commit_msg( args.push(id); } - let res = hook.run_hook(args.as_slice(), Duration::ZERO)?; + let res = hook.run_hook(args.as_slice())?; // load possibly altered msg msg.clear(); @@ -320,7 +321,7 @@ pub fn hooks_prepare_commit_msg_with_timeout( args.push(id); } - let res = hook.run_hook(args.as_slice(), timeout)?; + let res = hook.run_hook_with_timeout(args.as_slice(), timeout)?; // load possibly altered msg msg.clear(); @@ -334,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() { @@ -345,7 +346,7 @@ mod tests { assert_eq!(res, HookResult::NoHookFound); - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -360,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 "; @@ -378,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 @@ -398,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 "; @@ -419,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 "; @@ -442,7 +443,7 @@ exit 0 let (td, repo) = repo_init(); { - let hook = b"#!/bin/sh + let hook = b"#!/usr/bin/env sh exit 0 "; @@ -450,7 +451,7 @@ exit 0 } { - let reject_hook = b"#!/bin/sh + let reject_hook = b"#!/usr/bin/env sh exit 1 "; @@ -474,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 "; @@ -488,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 "; @@ -510,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 "; @@ -540,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 "; @@ -596,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 @@ -622,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 "; @@ -662,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 "; @@ -686,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 @@ -718,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"); + } } From 4d0b022651624be2c71367a2deb87f82ba967039 Mon Sep 17 00:00:00 2001 From: DaRacci Date: Sun, 23 Mar 2025 21:17:36 +1100 Subject: [PATCH 8/8] chore: Add entry to CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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))