Add timeouts for commit hooks

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<git2_hooks::HookResult> for HookResult {
@@ -22,6 +25,7 @@ impl From<git2_hooks::HookResult> 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<HookResult> {
+	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<HookResult> {
 	scope_time!("hooks_pre_commit");
@@ -47,6 +67,22 @@ pub fn hooks_pre_commit(repo_path: &RepoPath) -> Result<HookResult> {
 	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<HookResult> {
+	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<HookResult> {
 	scope_time!("hooks_post_commit");
@@ -56,6 +92,22 @@ pub fn hooks_post_commit(repo_path: &RepoPath) -> Result<HookResult> {
 	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<HookResult> {
+	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<HookResult> {
+	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;
 
@@ -82,7 +154,7 @@ mod tests {
 		let (_td, repo) = repo_init().unwrap();
 		let root = repo.path().parent().unwrap();
 
-		let hook = b"#!/bin/sh
+		let hook = b"#!/usr/bin/env sh
 	echo 'rejected'
 	exit 1
 	        ";
@@ -119,7 +191,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
 	        ";
@@ -145,7 +217,7 @@ mod tests {
 		let (_td, repo) = repo_init().unwrap();
 		let root = repo.path().parent().unwrap();
 
-		let hook = b"#!/bin/sh
+		let hook = b"#!/usr/bin/env sh
 	echo 'msg' > $1
 	echo 'rejected'
 	exit 1
@@ -174,4 +246,107 @@ 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());
+	}
 }

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;

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 {
@@ -106,7 +108,11 @@ impl HookPaths {
 
 	/// this function calls hook scripts based on conventions documented here
 	/// see <https://git-scm.com/docs/githooks>
-	pub fn run_hook(&self, args: &[&str]) -> Result<HookResult> {
+	pub fn run_hook(
+		&self,
+		args: &[&str],
+		timeout: Duration,
+	) -> Result<HookResult> {
 		let hook = self.hook.clone();
 
 		let arg_str = format!("{:?} {}", hook, args.join(" "));
@@ -119,7 +125,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)
@@ -130,7 +136,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 })