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

Author: DaRacci

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<git2_hooks::HookResult> for HookResult {
@@ -26,156 +25,127 @@ impl From<git2_hooks::HookResult> for HookResult {
 				stderr,
 				..
 			} => Self::NotOk(format!("{stdout}{stderr}")),
+			git2_hooks::HookResult::TimedOut { .. } => Self::TimedOut,
 		}
 	}
 }
 
-fn run_with_timeout<F>(
-	f: F,
-	timeout: Duration,
-) -> Result<(HookResult, Option<String>)>
-where
-	F: FnOnce() -> Result<(HookResult, Option<String>)>
-		+ 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<HookResult> {
+	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<HookResult> {
-	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<HookResult> {
+	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<HookResult> {
 	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<HookResult> {
+	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<HookResult> {
 	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`
 pub fn hooks_prepare_commit_msg(
 	repo_path: &RepoPath,
 	source: PrepareCommitMsgSource,
 	msg: &mut String,
+) -> Result<HookResult> {
+	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<HookResult> {
 	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,12 +360,43 @@ mod tests {
 			hook,
 		);
 
-		let res = hooks_post_commit(
+		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;