Add timeouts for commit hooks #2547

DaRacci · 2025-03-05T12:39:24Z

This Pull Request closes #2546.

It changes the following:

Adds timeouts to git hooks

I followed the checklist:

I added unittests
I ran make check without errors
I tested the overall application
I added an appropriate item to the changelog

DaRacci · 2025-03-05T12:40:35Z

Opening early as a draft.

Initial changes are working and have tests added for them just need to figure out how we are going to implement a timeout value, if thats user configuration or a default impl.

DaRacci · 2025-03-06T08:03:18Z

Was wanting to get some feedback on this before progressing further, is this the correct place to implement the timeout logic? @extrawurst

extrawurst · 2025-03-11T10:24:30Z

This would impact other users like Gitbutler. @Byron what do you think about this change?

cruessler

From an implementation standpoint, this looks good. The only thing I would add is tests for non-zero timeouts (maybe that’s something you already planned or maybe I missed something).

asyncgit/src/sync/hooks.rs

DaRacci · 2025-03-16T02:47:59Z

Also worth mentioning as its done in this PR, I've changed the test shell shebang to use /usr/bin/env sh as this is a more widely accepted and allows distributions like NixOS to run the tests.

DaRacci · 2025-03-16T02:59:20Z

I've also noticed that while running tests that since i use the non POSIX Shell (nushell) that i need to prefix the command with SHELL=bash, i was wondering if it may be worth testing if the $SHELL is a known non compliant shell and trying to look for a default POSIX shell in the path?

DaRacci · 2025-03-16T03:20:36Z

Now that the implementation is solid how do i go about with setting the timeout, how do we expose configurable value for the user and should there be a default timeout?

Byron

Thanks for letting me know, @extrawurst.

The changes seem to not apply to git2-hooks, so wouldn't affect GitButler in its current form. However, that would change if timeouts would move into git2-hooks as I seem to be proposing :).

PS: I unsubscribed as this PR is relevant to me only once git2-hooks is involved. You can always ping me to change that.

asyncgit/src/sync/hooks.rs

…meouts 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

DaRacci · 2025-03-16T07:43:14Z

@Byron I've updated the implementation and it is now inside git2-hooks, I've added new functions with suffixed _with_timeout for each hook but the run_hooks function has a breaking change having the timeout parameter added to it.

DaRacci · 2025-03-16T07:46:19Z

The last remaining thing to do is move the tests i had written over to the git2_hooks crate, even so the current tests in asyncgit should be able to cover most cases i think.

DaRacci · 2025-03-16T07:58:03Z

I was also thinking about the initial issue where the problem was an infinite wait because the hook was waiting for input, maybe a solution could be when a hook is running a new window is shown with the stdio from the hook and also allowing stdin, not sure if that would work how im thinking though.

Byron

Thanks a lot for the quick turnaround!

The new implementation definitely addresses the previous issue, and now the timeout has an actual effect on the hook.

When looking at git2-hooks exclusively, I am not sure I saw a test for this though.

Byron · 2025-03-16T22:48:04Z

git2-hooks/src/hookspath.rs

@@ -130,7 +136,28 @@ impl HookPaths {
 				"DUMMY_ENV_TO_FIX_WINDOWS_CMD_RUNS",
 				"FixPathHandlingOnWindows",
 			)
-			.output()?;
+			.stdout(Stdio::piped())
+			.stderr(Stdio::piped())


This changes the semantics we had before, where stdin was null (see the docs of output).
Doing so should also make it impossible for a hook to hang waiting on stdin. Hooks which still hang are probably buggy in another way.

Byron · 2025-03-16T22:48:57Z

git2-hooks/src/hookspath.rs

+				}
+
+				std::thread::yield_now();
+				std::thread::sleep(Duration::from_millis(10));


Doesn't sleep() imply a yield, among other things?

Byron · 2025-03-16T22:50:04Z

git2-hooks/src/lib.rs

@@ -66,6 +67,10 @@ pub enum HookResult {
 		/// path of the hook that was run
 		hook: PathBuf,
 	},
+	TimedOut {


Docs are missing.
Probably the crate could use a deny(missing_docs) as well.

Byron · 2025-03-16T22:52:39Z

git2-hooks/src/hookspath.rs

@@ -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(


I still think this doesn't have to be a breaking change unless one really wants to tell folks to deal with the timeout. That timeouts are implemented now probably means most people didn't have issues with this (or solved them by fixing the hooks), so I'd argue that run_hook_with_timeout() should be favored.

Byron · 2025-03-16T22:55:01Z

git2-hooks/src/hookspath.rs

+				}
+
+				std::thread::yield_now();
+				std::thread::sleep(Duration::from_millis(10));


One could think about a quadratic backoff (or a custom sequence) here where the first few milliseconds are checked more often, and as it takes longer, one checks less often.
This might also be overthinking it as 10ms seems fine.

Depending on how this is done, I think one should mention in the docs that the timeout may prolong the actual runtimes, right now by up to 10ms.

Byron · 2025-03-16T22:55:18Z

git2-hooks/src/hookspath.rs

@@ -106,7 +108,11 @@ impl HookPaths {

 	/// this function calls hook scripts based on conventions documented here
 	/// see <https://git-scm.com/docs/githooks>


Needs a documentation update.

feat(asyncgit): add timeouts for hooks

cb52f7b

extrawurst requested a review from Byron March 11, 2025 10:24

Merge branch 'master' into master

50f55f4

extrawurst requested a review from cruessler March 15, 2025 09:09

cruessler reviewed Mar 15, 2025

View reviewed changes

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

fix(asyncgit/test): use correct hook path and add non zero timeout test

cb53532

DaRacci force-pushed the master branch from 215febe to cb53532 Compare March 16, 2025 03:22

Byron reviewed Mar 16, 2025

View reviewed changes

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

asyncgit/src/sync/hooks.rs Outdated Show resolved Hide resolved

DaRacci added 4 commits March 16, 2025 16:59

chore: fix minor spelling mistakes

7703743

fix(test): relax time a bit

63acc53

feat: implement option for hook timeout and abort the commit

b932754

DaRacci marked this pull request as ready for review March 16, 2025 11:02

Byron reviewed Mar 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add timeouts for commit hooks #2547

Add timeouts for commit hooks #2547

DaRacci commented Mar 5, 2025 •

edited

Loading

DaRacci commented Mar 5, 2025

DaRacci commented Mar 6, 2025 •

edited

Loading

extrawurst commented Mar 11, 2025

cruessler left a comment

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment •

edited

Loading

DaRacci commented Mar 16, 2025 •

edited

Loading

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment

Byron Mar 16, 2025

Byron Mar 16, 2025

Byron Mar 16, 2025

Byron Mar 16, 2025

Byron Mar 16, 2025

Byron Mar 16, 2025

		@@ -106,7 +108,11 @@ impl HookPaths {

		/// this function calls hook scripts based on conventions documented here
		/// see <https://git-scm.com/docs/githooks>

Add timeouts for commit hooks #2547

Are you sure you want to change the base?

Add timeouts for commit hooks #2547

Conversation

DaRacci commented Mar 5, 2025 • edited Loading

DaRacci commented Mar 5, 2025

DaRacci commented Mar 6, 2025 • edited Loading

extrawurst commented Mar 11, 2025

cruessler left a comment

Choose a reason for hiding this comment

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment • edited Loading

Choose a reason for hiding this comment

DaRacci commented Mar 16, 2025 • edited Loading

DaRacci commented Mar 16, 2025

DaRacci commented Mar 16, 2025

Byron left a comment

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

Byron Mar 16, 2025

Choose a reason for hiding this comment

DaRacci commented Mar 5, 2025 •

edited

Loading

DaRacci commented Mar 6, 2025 •

edited

Loading

Byron left a comment •

edited

Loading

DaRacci commented Mar 16, 2025 •

edited

Loading