proper pre-push hook implementation #2811

extrawurst · 2025-12-17T13:55:20Z

argenkiwi · 2025-12-17T19:37:29Z

I can confirm the issue appears to be gone when building from this branch.

I am not that familiar with Rust development, so let me share the steps I followed to ensure I am not missing anything:

Uninstalled gitui
Cloned this repo
Checked out the pre-push-with-lfs branch
Executed cargo build
Switched to the folder of the repo that presented the original issue
Executed cargo build --manifest-path path/to/gitui/repo/Cargo.toml
Successfully performed a push by pressing P

extrawurst · 2025-12-17T19:40:36Z

I can confirm the issue appears to be gone when building from this branch.

I am not that familiar with Rust development, so let me share the steps I followed to ensure I am not missing anything:

The way you did it works, if you wanna use this custom build in a few more places to test, simply run make install and it will install a custom build gitui in your system that you can invoke just like your prior installed gitui version

argenkiwi · 2025-12-17T19:44:54Z

I can confirm the issue appears to be gone when building from this branch.
I am not that familiar with Rust development, so let me share the steps I followed to ensure I am not missing anything:

The way you did it works, if you wanna use this custom build in a few more places to test, simply run make install and it will install a custom build gitui in your system that you can invoke just like your prior installed gitui version

Excellent, that will get me through the last work day of the year. Thank you for solving the issue so quickly. Have a great end of the year.

Branch case now uses the same destination ref logic as the push path: it honors `push.default=upstream` (when not deleting) via `branch_push_destination_ref`, so the `<remote-ref>` field matches the refspec Git would use.

cruessler · 2025-12-18T06:57:30Z

@extrawurst I’ll try and have a look later this week! (I want to set up a proper repository with git lfs hooks for testing.)

cruessler · 2025-12-21T17:26:13Z

I started reviewing this PR, but since it’s a lot of new code and I’m not deeply familiar with hooks, it might take a while. :-) I can confirm, though, that the error message mentioned in the bug report is gone. Instead, git lfs pre-push … is called as far as I can tell.

cruessler

As far as I can tell, things are looking good! My understanding is that this PR adds proper passing of arguments and stdin to pre-push hook calls, to be more in line with what git does.

The changes in hooks.rs look good, but I haven’t been able to review them as detailed as the ones in lib.rs as this is an area I’m less familiar with.

Some of my comments are repetitive, e. g. ones to the effect of “Can this comment be removed”. I hope that’s more helpful than just a general “Some of the comments could potentially be removed”.

cruessler · 2025-12-27T14:05:45Z

git2-hooks/src/lib.rs

 	pub const fn is_ok(&self) -> bool {
-		matches!(self, Self::Ok { .. })
+		match self {
+			Self::Run(response) => response.code == 0,


What about calling response.is_successful() and !response.is_successful() below? Otherwise, can HookRunResponse::is_successful be deleted?

cruessler · 2025-12-27T14:07:07Z

git2-hooks/src/lib.rs

+}
+
+impl PrePushRef {
+	#[must_use]


Why is this #[must_use]?

cruessler · 2025-12-27T14:07:52Z

git2-hooks/src/lib.rs

+		oid.map_or_else(|| "0".repeat(40), |id| id.to_string())
+	}
+
+	#[must_use]


Why is this #[must_use]?

cruessler · 2025-12-27T14:14:56Z

git2-hooks/src/lib.rs

+	}
+
+	fn format_oid(oid: Option<Oid>) -> String {
+		oid.map_or_else(|| "0".repeat(40), |id| id.to_string())


What about adding a comment explaining the else branch? (I now know why it’s this way, but I only know because I read the docs: https://git-scm.com/docs/githooks#_pre_push.)

cruessler · 2025-12-27T14:22:59Z

git2-hooks/src/lib.rs

-		} = result
-		else {
+		let HookResult::Run(response) = result else {
 			unreachable!("run_hook should've failed");


This message is not accurate anymore as the alternative to HookResult::Run is HookResult::NoHookFound. Other tests don’t have a message, so that’s also an option.

cruessler · 2025-12-27T15:22:39Z

asyncgit/src/sync/hooks.rs

+		)
+		.unwrap();
+
+		// Hook should succeed


Can this comment be removed?

cruessler · 2025-12-27T15:24:51Z

asyncgit/src/sync/hooks.rs

+		.unwrap();
+
+		// Hook should succeed
+		assert!(res.is_ok());


Similar to other tests. I feel like assert_eq!(response.stdout, "…") would make this test’s intent more clear.

cruessler · 2025-12-27T15:25:22Z

asyncgit/src/sync/hooks.rs

+			hook,
+		);
+
+		// Try to push master branch


Can this comment be removed?

cruessler · 2025-12-27T15:25:27Z

asyncgit/src/sync/hooks.rs

+		)
+		.unwrap();
+
+		// Hook should reject


Can this comment be removed?

cruessler · 2025-12-27T15:25:34Z

asyncgit/src/sync/hooks.rs

+	fn test_pre_push_hook_rejects_based_on_stdin() {
+		let (_td, repo) = repo_init().unwrap();
+
+		// Create a hook that rejects pushes to master branch


Can this comment be removed?

extrawurst requested a review from cruessler December 17, 2025 22:23

extrawurst added 20 commits December 17, 2025 19:13

proper pre-push hook implementation

2347a8b

better error handling, fix upstream sha

36bb24a

hooksresult refactor

3b885fe

explicit drop semantics

b90a365

feed correct stdin to pre-push hook

96f18c9

use correct remote advertised tips for pre_push hook

5aab2e0

fix branch destination ref logic

f8ce43f

Branch case now uses the same destination ref logic as the push path: it honors `push.default=upstream` (when not deleting) via `branch_push_destination_ref`, so the `<remote-ref>` field matches the refspec Git would use.

skip remote access if pre_push not set

5d7f749

provide same creds to hooks call as to push

4a33af4

cleanup

a3c73ef

simplify and document

b1802f4

improve error handling

a478074

simplify stdin

a81cc14

revert unrelated change

cf8d1cb

add comment

2561dc9

cleanup uneeded allow

8d2af0e

cleanup: no need to be public

bc0f381

integration test

b92803b

potential fix for broken pipe ci tests failing

61951ff

add more logging for diagnostics

6998e42

extrawurst force-pushed the pre-push-with-lfs branch from ce17490 to 6998e42 Compare December 18, 2025 00:13

cruessler requested changes Dec 27, 2025

View reviewed changes

Uh oh!

proper pre-push hook implementation #2811

Are you sure you want to change the base?

proper pre-push hook implementation #2811

Conversation

extrawurst commented Dec 17, 2025

Uh oh!

argenkiwi commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

extrawurst commented Dec 17, 2025

Uh oh!

argenkiwi commented Dec 17, 2025

Uh oh!

cruessler commented Dec 18, 2025

Uh oh!

cruessler commented Dec 21, 2025

Uh oh!

cruessler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

argenkiwi commented Dec 17, 2025 •

edited

Loading