From 2347a8b9ac8d9013b68383e48db01666d077bc98 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 08:54:49 -0500 Subject: [PATCH 01/20] proper pre-push hook implementation --- asyncgit/src/sync/hooks.rs | 36 ++- git2-hooks/src/hookspath.rs | 60 +++-- git2-hooks/src/lib.rs | 423 +++++++++++++++++++++++++++++++----- src/popups/push.rs | 17 +- src/popups/push_tags.rs | 21 +- 5 files changed, 469 insertions(+), 88 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index fe1a91912d..6fe2cabb64 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -15,13 +15,17 @@ pub enum HookResult { impl From for HookResult { fn from(v: git2_hooks::HookResult) -> Self { match v { - git2_hooks::HookResult::Ok { .. } - | git2_hooks::HookResult::NoHookFound => Self::Ok, - git2_hooks::HookResult::RunNotSuccessful { - stdout, - stderr, - .. - } => Self::NotOk(format!("{stdout}{stderr}")), + git2_hooks::HookResult::NoHookFound => Self::Ok, + git2_hooks::HookResult::Run(response) => { + if response.is_successful() { + Self::Ok + } else { + Self::NotOk(format!( + "{}{}", + response.stdout, response.stderr + )) + } + } } } } @@ -73,12 +77,26 @@ pub fn hooks_prepare_commit_msg( } /// see `git2_hooks::hooks_pre_push` -pub fn hooks_pre_push(repo_path: &RepoPath) -> Result { +pub fn hooks_pre_push( + repo_path: &RepoPath, + remote: Option<&str>, + url: &str, + branch_name: Option<&str>, + remote_branch_name: Option<&str>, +) -> Result { scope_time!("hooks_pre_push"); let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_pre_push(&repo, None)?.into()) + Ok(git2_hooks::hooks_pre_push( + &repo, + None, + remote, + url, + branch_name, + remote_branch_name, + )? + .into()) } #[cfg(test)] diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index 33fe1bf659..0d379cb74b 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -141,6 +141,20 @@ impl HookPaths { /// this function calls hook scripts based on conventions documented here /// see pub fn run_hook_os_str(&self, args: I) -> Result + where + I: IntoIterator + Copy, + S: AsRef, + { + self.run_hook_os_str_with_stdin(args, None) + } + + /// this function calls hook scripts with stdin input based on conventions documented here + /// see + pub fn run_hook_os_str_with_stdin( + &self, + args: I, + stdin: Option<&[u8]>, + ) -> Result where I: IntoIterator + Copy, S: AsRef, @@ -153,11 +167,23 @@ impl HookPaths { ); let run_command = |command: &mut Command| { - command + let mut child = command .args(args) .current_dir(&self.pwd) .with_no_window() - .output() + .stdin(std::process::Stdio::piped()) + .stdout(std::process::Stdio::piped()) + .stderr(std::process::Stdio::piped()) + .spawn()?; + + if let Some(input) = stdin { + use std::io::Write; + if let Some(mut stdin_handle) = child.stdin.take() { + stdin_handle.write_all(input)?; + } + } + + child.wait_with_output() }; let output = if cfg!(windows) { @@ -210,21 +236,21 @@ impl HookPaths { } }?; - 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, - }) - } + let stderr = + String::from_utf8_lossy(&output.stderr).to_string(); + let stdout = + String::from_utf8_lossy(&output.stdout).to_string(); + + Ok(HookResult::Run(crate::HookRunResponse { + hook, + stdout, + stderr, + code: if output.status.success() { + None + } else { + output.status.code() + }, + })) } } diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index dd1fb66484..234006e7b9 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -48,37 +48,49 @@ pub const HOOK_PRE_PUSH: &str = "pre-push"; const HOOK_COMMIT_MSG_TEMP_FILE: &str = "COMMIT_EDITMSG"; +/// Response from running a hook +#[derive(Debug, PartialEq, Eq)] +pub struct HookRunResponse { + /// path of the hook that was run + pub hook: PathBuf, + /// stdout output emitted by hook + pub stdout: String, + /// stderr output emitted by hook + pub stderr: String, + /// exit code as reported back from process calling the hook (None if successful) + pub code: Option, +} + #[derive(Debug, PartialEq, Eq)] pub enum HookResult { /// No hook found NoHookFound, - /// Hook executed with non error return code - Ok { - /// path of the hook that was run - hook: PathBuf, - }, - /// Hook executed and returned an error code - RunNotSuccessful { - /// exit code as reported back from process calling the hook - code: Option, - /// stderr output emitted by hook - stdout: String, - /// stderr output emitted by hook - stderr: String, - /// path of the hook that was run - hook: PathBuf, - }, + /// Hook executed (check `HookRunResponse.code` for success/failure) + Run(HookRunResponse), } impl HookResult { - /// helper to check if result is ok + /// helper to check if result is ok (hook succeeded with exit code 0) pub const fn is_ok(&self) -> bool { - matches!(self, Self::Ok { .. }) + match self { + Self::Run(response) => response.code.is_none(), + Self::NoHookFound => false, + } } - /// helper to check if result was run and not rejected + /// helper to check if result was run and not successful (non-zero exit code) pub const fn is_not_successful(&self) -> bool { - matches!(self, Self::RunNotSuccessful { .. }) + match self { + Self::Run(response) => response.code.is_some(), + Self::NoHookFound => false, + } + } +} + +impl HookRunResponse { + /// Check if the hook succeeded (exit code 0) + pub const fn is_successful(&self) -> bool { + self.code.is_none() } } @@ -172,9 +184,25 @@ pub fn hooks_post_commit( } /// this hook is documented here +/// +/// According to git documentation, pre-push hook receives: +/// - remote name as first argument (or URL if remote is not named) +/// - remote URL as second argument +/// - information about refs being pushed via stdin in format: +/// ` SP SP SP LF` +/// +/// If `remote` is `None` or empty, the `url` is used for both arguments as per Git spec. +/// +/// Parameters: +/// - `branch_name`: Optional local branch name being pushed (e.g., "main"). If `None`, stdin will be empty. +/// - `remote_branch_name`: Optional remote branch name (if different from local) pub fn hooks_pre_push( repo: &Repository, other_paths: Option<&[&str]>, + remote: Option<&str>, + url: &str, + branch_name: Option<&str>, + remote_branch_name: Option<&str>, ) -> Result { let hook = HookPaths::new(repo, other_paths, HOOK_PRE_PUSH)?; @@ -182,7 +210,47 @@ pub fn hooks_pre_push( return Ok(HookResult::NoHookFound); } - hook.run_hook(&[]) + // If a remote is not named (None or empty), the URL is passed for both arguments + let remote_name = match remote { + Some(r) if !r.is_empty() => r, + _ => url, + }; + + // Build stdin according to Git pre-push hook spec: + // SP SP SP LF + + let stdin_data = if let Some(branch) = branch_name { + // Get local branch reference and commit + let local_branch = + repo.find_branch(branch, git2::BranchType::Local)?; + let local_ref = format!("refs/heads/{branch}"); + let local_commit = local_branch.get().peel_to_commit()?; + let local_sha = local_commit.id().to_string(); + + // Determine remote branch name (same as local if not specified) + let remote_branch = remote_branch_name.unwrap_or(branch); + let remote_ref = format!("refs/heads/{remote_branch}"); + + // Try to get remote commit SHA from upstream + // If there's no upstream (new branch), use 40 zeros as per Git spec + let remote_sha = if let Ok(upstream) = local_branch.upstream() + { + upstream.get().peel_to_commit()?.id().to_string() + } else { + "0".repeat(40) + }; + + // Format: refs/heads/branch local_sha refs/heads/branch remote_sha\n + format!("{local_ref} {local_sha} {remote_ref} {remote_sha}\n") + } else { + // No branch specified (e.g., pushing tags), use empty stdin + String::new() + }; + + hook.run_hook_os_str_with_stdin( + [remote_name, url], + Some(stdin_data.as_bytes()), + ) } pub enum PrepareCommitMsgSource { @@ -339,22 +407,16 @@ exit 0 let result = hook.run_hook(&[TEXT]).unwrap(); - let HookResult::RunNotSuccessful { - code, - stdout, - stderr, - hook: h, - } = result - else { + let HookResult::Run(response) = result else { unreachable!("run_hook should've failed"); }; - let stdout = stdout.as_str().trim_ascii_end(); + let stdout = response.stdout.as_str().trim_ascii_end(); - assert_eq!(code, Some(42)); - assert_eq!(h, hook.hook); + assert_eq!(response.code, Some(42)); + assert_eq!(response.hook, hook.hook); assert_eq!(stdout, TEXT, "{:?} != {TEXT:?}", stdout); - assert!(stderr.is_empty()); + assert!(response.stderr.is_empty()); } #[test] @@ -448,15 +510,17 @@ exit 1 create_hook(&repo, HOOK_PRE_COMMIT, hook); let res = hooks_pre_commit(&repo, None).unwrap(); - let HookResult::RunNotSuccessful { stdout, .. } = res else { + let HookResult::Run(response) = res else { unreachable!() }; assert!( - stdout + response + .stdout .lines() .any(|line| line.starts_with(PATH_EXPORT)), - "Could not find line starting with {PATH_EXPORT:?} in: {stdout:?}" + "Could not find line starting with {PATH_EXPORT:?} in: {:?}", + response.stdout ); } @@ -482,13 +546,12 @@ exit 1 let res = hooks_pre_commit(&repo, None).unwrap(); - let HookResult::RunNotSuccessful { code, stdout, .. } = res - else { + let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(code.unwrap(), 1); - assert_eq!(&stdout, "rejected\n"); + assert_eq!(response.code.unwrap(), 1); + assert_eq!(&response.stdout, "rejected\n"); } #[test] @@ -562,13 +625,12 @@ sys.exit(1) let mut msg = String::from("test"); let res = hooks_commit_msg(&repo, None, &mut msg).unwrap(); - let HookResult::RunNotSuccessful { code, stdout, .. } = res - else { + let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(code.unwrap(), 1); - assert_eq!(&stdout, "rejected\n"); + assert_eq!(response.code.unwrap(), 1); + assert_eq!(&response.stdout, "rejected\n"); assert_eq!(msg, String::from("msg\n")); } @@ -633,7 +695,7 @@ exit 0 ) .unwrap(); - assert!(matches!(res, HookResult::Ok { .. })); + assert!(res.is_ok()); assert_eq!(msg, String::from("msg:message\n")); } @@ -658,13 +720,12 @@ exit 2 ) .unwrap(); - let HookResult::RunNotSuccessful { code, stdout, .. } = res - else { + let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(code.unwrap(), 2); - assert_eq!(&stdout, "rejected\n"); + assert_eq!(response.code.unwrap(), 2); + assert_eq!(&response.stdout, "rejected\n"); assert_eq!( msg, @@ -684,9 +745,17 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); - let res = hooks_pre_push(&repo, None).unwrap(); + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://example.com/repo.git", + None, + None, + ) + .unwrap(); - assert!(matches!(res, HookResult::Ok { .. })); + assert!(res.is_ok()); } #[test] @@ -698,12 +767,256 @@ echo 'failed' exit 3 "; create_hook(&repo, HOOK_PRE_PUSH, hook); - let res = hooks_pre_push(&repo, None).unwrap(); - let HookResult::RunNotSuccessful { code, stdout, .. } = res - else { + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://example.com/repo.git", + None, + None, + ) + .unwrap(); + let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(code.unwrap(), 3); - assert_eq!(&stdout, "failed\n"); + assert_eq!(response.code.unwrap(), 3); + assert_eq!(&response.stdout, "failed\n"); + } + + #[test] + fn test_pre_push_no_remote_name() { + let (_td, repo) = repo_init(); + + let hook = b"#!/bin/sh +# Verify that when remote is None, URL is passed for both arguments +echo \"arg1=$1 arg2=$2\" +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let res = hooks_pre_push( + &repo, + None, + None, + "https://example.com/repo.git", + None, + None, + ) + .unwrap(); + + assert!(res.is_ok(), "Expected Ok result, got: {res:?}"); + } + + #[test] + fn test_pre_push_with_arguments() { + let (_td, repo) = repo_init(); + + // Hook that verifies it receives the correct arguments + // and prints them for verification + let hook = b"#!/bin/sh +echo \"remote_name=$1\" +echo \"remote_url=$2\" +# Read stdin to verify it's available (even if empty for now) +stdin_content=$(cat) +echo \"stdin_length=${#stdin_content}\" +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://example.com/repo.git", + None, + None, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + unreachable!("Expected Run result, got: {res:?}") + }; + + assert!(response.is_successful(), "Hook should succeed"); + + // Verify the hook received and echoed the correct arguments + assert!( + response.stdout.contains("remote_name=origin"), + "Expected stdout to contain 'remote_name=origin', got: {}", + response.stdout + ); + assert!( + response + .stdout + .contains("remote_url=https://example.com/repo.git"), + "Expected stdout to contain URL, got: {}", + response.stdout + ); + assert!( + response.stdout.contains("stdin_length=0"), + "Expected stdin to be empty (length 0), got: {}", + response.stdout + ); + } + + #[test] + fn test_pre_push_verifies_arguments() { + let (_td, repo) = repo_init(); + + // Hook that verifies and echoes the arguments it receives + let hook = b"#!/bin/sh +# Verify we got the expected arguments +if [ \"$1\" != \"origin\" ]; then + echo \"ERROR: Expected remote name 'origin', got '$1'\" >&2 + exit 1 +fi +if [ \"$2\" != \"https://github.com/user/repo.git\" ]; then + echo \"ERROR: Expected URL 'https://github.com/user/repo.git', got '$2'\" >&2 + exit 1 +fi +echo \"Arguments verified: remote=$1, url=$2\" +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://github.com/user/repo.git", + None, + None, + ) + .unwrap(); + + match res { + HookResult::Run(response) if response.is_successful() => { + // Success - arguments were correct + } + HookResult::Run(response) => { + panic!( + "Hook failed validation!\nstdout: {}\nstderr: {}", + response.stdout, response.stderr + ); + } + _ => unreachable!(), + } + } + + #[test] + fn test_pre_push_empty_stdin_currently() { + let (_td, repo) = repo_init(); + + // Hook that checks stdin content + // Currently we pass empty stdin, this test documents that behavior + let hook = b"#!/bin/sh +stdin_content=$(cat) +if [ -z \"$stdin_content\" ]; then + echo \"stdin is empty (expected for current implementation)\" + exit 0 +else + echo \"stdin_length=${#stdin_content}\" + echo \"stdin_content=$stdin_content\" + exit 0 +fi + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://github.com/user/repo.git", + None, + None, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + unreachable!("Expected Run result, got: {res:?}") + }; + + assert!(response.is_successful(), "Hook should succeed"); + + // Verify stdin is currently empty + assert!( + response.stdout.contains("stdin is empty"), + "Expected stdin to be empty, got: {}", + response.stdout + ); + } + + #[test] + fn test_pre_push_with_proper_stdin() { + let (_td, repo) = repo_init(); + + // Hook that verifies it receives refs information via stdin + // According to Git spec, format should be: + // SP SP SP LF + let hook = b"#!/bin/sh +# Read stdin +stdin_content=$(cat) +echo \"stdin received: $stdin_content\" + +# Verify stdin is not empty +if [ -z \"$stdin_content\" ]; then + echo \"ERROR: stdin is empty, expected ref information\" >&2 + exit 1 +fi + +# Verify stdin contains expected format +# Should have: refs/heads/master refs/heads/master +if ! echo \"$stdin_content\" | grep -q \"^refs/heads/\"; then + echo \"ERROR: stdin does not contain expected ref format\" >&2 + echo \"Got: $stdin_content\" >&2 + exit 1 +fi + +# Verify it has 4 space-separated fields +field_count=$(echo \"$stdin_content\" | awk '{print NF}') +if [ \"$field_count\" != \"4\" ]; then + echo \"ERROR: Expected 4 fields, got $field_count\" >&2 + exit 1 +fi + +echo \"SUCCESS: Received properly formatted stdin\" +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://github.com/user/repo.git", + Some("master"), + None, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + panic!("Expected Run result, got: {res:?}") + }; + + // This should now pass with proper stdin implementation + assert!( + response.is_successful(), + "Hook should succeed with proper stdin.\nstdout: {}\nstderr: {}", + response.stdout, + response.stderr + ); + + // Verify the hook received proper stdin format + assert!( + response.stdout.contains("SUCCESS"), + "Expected hook to validate stdin format.\nstdout: {}\nstderr: {}", + response.stdout, + response.stderr + ); } } diff --git a/src/popups/push.rs b/src/popups/push.rs index f900ce8613..269b143d06 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -144,10 +144,21 @@ impl PushPopup { remote }; + // get remote URL for pre-push hook + let remote_url = asyncgit::sync::get_remote_url( + &self.repo.borrow(), + &remote, + )? + .unwrap_or_default(); + // run pre push hook - can reject push - if let HookResult::NotOk(e) = - hooks_pre_push(&self.repo.borrow())? - { + if let HookResult::NotOk(e) = hooks_pre_push( + &self.repo.borrow(), + Some(&remote), + &remote_url, + Some(&self.branch), + None, + )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( "pre-push hook failed:\n{e}" diff --git a/src/popups/push_tags.rs b/src/popups/push_tags.rs index 30df245b16..7f5f15a6c2 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -84,10 +84,23 @@ impl PushTagsPopup { &mut self, cred: Option, ) -> Result<()> { + let remote = get_default_remote(&self.repo.borrow())?; + + // get remote URL for pre-push hook + let remote_url = asyncgit::sync::get_remote_url( + &self.repo.borrow(), + &remote, + )? + .unwrap_or_default(); + // run pre push hook - can reject push - if let HookResult::NotOk(e) = - hooks_pre_push(&self.repo.borrow())? - { + if let HookResult::NotOk(e) = hooks_pre_push( + &self.repo.borrow(), + Some(&remote), + &remote_url, + None, + None, + )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( "pre-push hook failed:\n{e}" @@ -100,7 +113,7 @@ impl PushTagsPopup { self.pending = true; self.progress = None; self.git_push.request(PushTagsRequest { - remote: get_default_remote(&self.repo.borrow())?, + remote, basic_credential: cred, })?; Ok(()) From 36bb24a25532dab83102dde5560af08b0bda1833 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 09:10:12 -0500 Subject: [PATCH 02/20] better error handling, fix upstream sha --- git2-hooks/src/lib.rs | 143 ++++++++++++++++++++++++++++++++++++++-- src/popups/push.rs | 14 +++- src/popups/push_tags.rs | 14 +++- 3 files changed, 162 insertions(+), 9 deletions(-) diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 234006e7b9..f3e9e348aa 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -231,12 +231,23 @@ pub fn hooks_pre_push( let remote_branch = remote_branch_name.unwrap_or(branch); let remote_ref = format!("refs/heads/{remote_branch}"); - // Try to get remote commit SHA from upstream - // If there's no upstream (new branch), use 40 zeros as per Git spec - let remote_sha = if let Ok(upstream) = local_branch.upstream() - { - upstream.get().peel_to_commit()?.id().to_string() + // Try to get remote commit SHA from the actual push target remote + // We need to look up refs/remotes//, not the upstream + let remote_sha = if let Some(remote_name) = remote { + // Try to find the remote tracking branch for the push target + let remote_ref_name = + format!("refs/remotes/{remote_name}/{remote_branch}"); + if let Ok(remote_ref) = + repo.find_reference(&remote_ref_name) + { + // Remote branch exists, get its SHA + remote_ref.peel_to_commit()?.id().to_string() + } else { + // Remote branch doesn't exist yet (new branch on remote) + "0".repeat(40) + } } else { + // No remote specified (shouldn't happen in practice) "0".repeat(40) }; @@ -1019,4 +1030,126 @@ exit 0 response.stderr ); } + + #[test] + fn test_pre_push_uses_push_target_remote_not_upstream() { + let (_td, repo) = repo_init(); + + // repo_init() already creates an initial commit on master + // Get the current HEAD commit + let head = repo.head().unwrap(); + let local_commit = head.target().unwrap(); + + // Set up scenario: + // - Local master is at local_commit (latest) + // - origin/master exists at local_commit (fully synced - upstream) + // - backup/master exists at old_commit (behind/different) + // - Branch tracks origin/master as upstream + // - We push to "backup" remote + // - Expected: remote SHA should be old_commit + // - Bug (before fix): remote SHA was from upstream origin/master + + // Create origin/master tracking branch (at same commit as local) + repo.reference( + "refs/remotes/origin/master", + local_commit, + true, + "create origin/master", + ) + .unwrap(); + + // Create backup/master at a different commit (simulating it's behind) + // We can't create a reference to a non-existent commit, so we'll + // create an actual old commit first + let sig = repo.signature().unwrap(); + let tree_id = { + let mut index = repo.index().unwrap(); + index.write_tree().unwrap() + }; + let tree = repo.find_tree(tree_id).unwrap(); + let old_commit = repo + .commit( + None, // Don't update any refs + &sig, + &sig, + "old backup commit", + &tree, + &[], // No parents + ) + .unwrap(); + + // Now create backup/master pointing to this old commit + repo.reference( + "refs/remotes/backup/master", + old_commit, + true, + "create backup/master at old commit", + ) + .unwrap(); + + // Configure branch.master.remote and branch.master.merge to set upstream + { + let mut config = repo.config().unwrap(); + config.set_str("branch.master.remote", "origin").unwrap(); + config + .set_str("branch.master.merge", "refs/heads/master") + .unwrap(); + } + + // Hook that extracts and prints the remote SHA + let hook = format!( + r#"#!/bin/sh +stdin_content=$(cat) +echo "stdin: $stdin_content" + +# Extract the 4th field (remote SHA) +remote_sha=$(echo "$stdin_content" | awk '{{print $4}}') +echo "remote_sha=$remote_sha" + +# When pushing to backup, we should get backup/master's old SHA +# NOT the SHA from origin/master upstream +if [ "$remote_sha" = "{}" ]; then + echo "BUG: Got origin/master SHA (upstream) instead of backup/master SHA" >&2 + exit 1 +fi + +if [ "$remote_sha" = "{}" ]; then + echo "SUCCESS: Got correct backup/master SHA" + exit 0 +fi + +echo "ERROR: Got unexpected SHA: $remote_sha" >&2 +echo "Expected backup/master: {}" >&2 +echo "Or origin/master (bug): {}" >&2 +exit 1 +"#, + local_commit, old_commit, old_commit, local_commit + ); + + create_hook(&repo, HOOK_PRE_PUSH, hook.as_bytes()); + + // Push to "backup" remote (which doesn't have master yet) + // This is different from the upstream "origin" + let res = hooks_pre_push( + &repo, + None, + Some("backup"), + "https://github.com/user/backup-repo.git", + Some("master"), + None, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + panic!("Expected Run result, got: {res:?}") + }; + + // This test now passes after fix + assert!( + response.is_successful(), + "Hook should receive backup/master SHA, not upstream origin/master SHA.\nstdout: {}\nstderr: {}", + response.stdout, + response.stderr + ); + } } diff --git a/src/popups/push.rs b/src/popups/push.rs index 269b143d06..a19cad2d24 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -148,8 +148,18 @@ impl PushPopup { let remote_url = asyncgit::sync::get_remote_url( &self.repo.borrow(), &remote, - )? - .unwrap_or_default(); + )?; + + // If remote doesn't have a URL configured, we can't push + let Some(remote_url) = remote_url else { + log::error!("remote '{remote}' has no URL configured"); + self.queue.push(InternalEvent::ShowErrorMsg(format!( + "Remote '{remote}' has no URL configured" + ))); + self.pending = false; + self.visible = false; + return Ok(()); + }; // run pre push hook - can reject push if let HookResult::NotOk(e) = hooks_pre_push( diff --git a/src/popups/push_tags.rs b/src/popups/push_tags.rs index 7f5f15a6c2..7edbb1a094 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -90,8 +90,18 @@ impl PushTagsPopup { let remote_url = asyncgit::sync::get_remote_url( &self.repo.borrow(), &remote, - )? - .unwrap_or_default(); + )?; + + // If remote doesn't have a URL configured, we can't push + let Some(remote_url) = remote_url else { + log::error!("remote '{remote}' has no URL configured"); + self.queue.push(InternalEvent::ShowErrorMsg(format!( + "Remote '{remote}' has no URL configured" + ))); + self.pending = false; + self.visible = false; + return Ok(()); + }; // run pre push hook - can reject push if let HookResult::NotOk(e) = hooks_pre_push( From 3b885feb0360d4ce995f18fab11588973fda7b68 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 09:29:06 -0500 Subject: [PATCH 03/20] hooksresult refactor --- git2-hooks/src/error.rs | 3 +++ git2-hooks/src/hookspath.rs | 10 +++++----- git2-hooks/src/lib.rs | 22 +++++++++++----------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/git2-hooks/src/error.rs b/git2-hooks/src/error.rs index dd88440b97..81dae71d64 100644 --- a/git2-hooks/src/error.rs +++ b/git2-hooks/src/error.rs @@ -14,6 +14,9 @@ pub enum HooksError { #[error("shellexpand error:{0}")] ShellExpand(#[from] shellexpand::LookupError), + + #[error("hook process terminated by signal without exit code")] + NoExitCode, } /// crate specific `Result` type diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index 0d379cb74b..87d2e1d42c 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -241,15 +241,15 @@ impl HookPaths { let stdout = String::from_utf8_lossy(&output.stdout).to_string(); + // Get exit code, or fail if process was killed by signal + let code = + output.status.code().ok_or(HooksError::NoExitCode)?; + Ok(HookResult::Run(crate::HookRunResponse { hook, stdout, stderr, - code: if output.status.success() { - None - } else { - output.status.code() - }, + code, })) } } diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index f3e9e348aa..68694323f9 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -57,8 +57,8 @@ pub struct HookRunResponse { pub stdout: String, /// stderr output emitted by hook pub stderr: String, - /// exit code as reported back from process calling the hook (None if successful) - pub code: Option, + /// exit code as reported back from process calling the hook (0 = success) + pub code: i32, } #[derive(Debug, PartialEq, Eq)] @@ -73,15 +73,15 @@ impl HookResult { /// helper to check if result is ok (hook succeeded with exit code 0) pub const fn is_ok(&self) -> bool { match self { - Self::Run(response) => response.code.is_none(), - Self::NoHookFound => false, + Self::Run(response) => response.code == 0, + Self::NoHookFound => true, } } /// helper to check if result was run and not successful (non-zero exit code) pub const fn is_not_successful(&self) -> bool { match self { - Self::Run(response) => response.code.is_some(), + Self::Run(response) => response.code != 0, Self::NoHookFound => false, } } @@ -90,7 +90,7 @@ impl HookResult { impl HookRunResponse { /// Check if the hook succeeded (exit code 0) pub const fn is_successful(&self) -> bool { - self.code.is_none() + self.code == 0 } } @@ -424,7 +424,7 @@ exit 0 let stdout = response.stdout.as_str().trim_ascii_end(); - assert_eq!(response.code, Some(42)); + assert_eq!(response.code, 42); assert_eq!(response.hook, hook.hook); assert_eq!(stdout, TEXT, "{:?} != {TEXT:?}", stdout); assert!(response.stderr.is_empty()); @@ -561,7 +561,7 @@ exit 1 unreachable!() }; - assert_eq!(response.code.unwrap(), 1); + assert_eq!(response.code, 1); assert_eq!(&response.stdout, "rejected\n"); } @@ -640,7 +640,7 @@ sys.exit(1) unreachable!() }; - assert_eq!(response.code.unwrap(), 1); + assert_eq!(response.code, 1); assert_eq!(&response.stdout, "rejected\n"); assert_eq!(msg, String::from("msg\n")); @@ -735,7 +735,7 @@ exit 2 unreachable!() }; - assert_eq!(response.code.unwrap(), 2); + assert_eq!(response.code, 2); assert_eq!(&response.stdout, "rejected\n"); assert_eq!( @@ -790,7 +790,7 @@ exit 3 let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(response.code.unwrap(), 3); + assert_eq!(response.code, 3); assert_eq!(&response.stdout, "failed\n"); } From b90a365b5b5dd809189969cb447eab783531cb07 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 09:42:07 -0500 Subject: [PATCH 04/20] explicit drop semantics --- git2-hooks/src/hookspath.rs | 5 +++-- src/clipboard.rs | 9 ++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index 87d2e1d42c..ad7f3d2157 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -176,11 +176,12 @@ impl HookPaths { .stderr(std::process::Stdio::piped()) .spawn()?; - if let Some(input) = stdin { + if let Some(mut stdin_handle) = child.stdin.take() { use std::io::Write; - if let Some(mut stdin_handle) = child.stdin.take() { + if let Some(input) = stdin { stdin_handle.write_all(input)?; } + drop(stdin_handle); } child.wait_with_output() diff --git a/src/clipboard.rs b/src/clipboard.rs index 79373a5b12..20b10cc383 100644 --- a/src/clipboard.rs +++ b/src/clipboard.rs @@ -26,12 +26,15 @@ fn exec_copy_with_args( .spawn() .map_err(|e| anyhow!("`{command:?}`: {e:?}"))?; - process + let mut stdin = process .stdin - .as_mut() - .ok_or_else(|| anyhow!("`{command:?}`"))? + .take() + .ok_or_else(|| anyhow!("`{command:?}`"))?; + + stdin .write_all(text.as_bytes()) .map_err(|e| anyhow!("`{command:?}`: {e:?}"))?; + drop(stdin); let out = process .wait_with_output() From 96f18c998368acc28be89d7b3f53b794edc6ba6e Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 13:21:49 -0500 Subject: [PATCH 05/20] feed correct stdin to pre-push hook --- asyncgit/src/sync/hooks.rs | 120 ++++++++++-- asyncgit/src/sync/mod.rs | 3 +- git2-hooks/src/lib.rs | 362 +++++++++++++++++++++++++++++-------- src/popups/push.rs | 15 +- src/popups/push_tags.rs | 10 +- 5 files changed, 413 insertions(+), 97 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 6fe2cabb64..7fd9efd462 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -1,6 +1,9 @@ use super::{repository::repo, RepoPath}; -use crate::error::Result; -pub use git2_hooks::PrepareCommitMsgSource; +use crate::{ + error::Result, sync::remotes::tags::tags_missing_remote, +}; +use git2::BranchType; +pub use git2_hooks::{PrePushRef, PrepareCommitMsgSource}; use scopetime::scope_time; /// @@ -30,6 +33,38 @@ impl From for HookResult { } } +fn pre_push_branch_update( + repo: &git2::Repository, + remote: Option<&str>, + branch_name: &str, + remote_branch_name: Option<&str>, + delete: bool, +) -> PrePushRef { + let local_ref = format!("refs/heads/{branch_name}"); + let local_oid = (!delete) + .then(|| { + repo.find_branch(branch_name, BranchType::Local) + .ok() + .and_then(|branch| branch.get().peel_to_commit().ok()) + .map(|commit| commit.id()) + }) + .flatten(); + + let remote_branch = remote_branch_name.unwrap_or(branch_name); + let remote_ref = format!("refs/heads/{remote_branch}"); + + let remote_oid = remote.and_then(|remote_name| { + repo.find_reference(&format!( + "refs/remotes/{remote_name}/{remote_branch}" + )) + .ok() + .and_then(|r| r.peel_to_commit().ok()) + .map(|c| c.id()) + }); + + PrePushRef::new(local_ref, local_oid, remote_ref, remote_oid) +} + /// see `git2_hooks::hooks_commit_msg` pub fn hooks_commit_msg( repo_path: &RepoPath, @@ -81,22 +116,81 @@ pub fn hooks_pre_push( repo_path: &RepoPath, remote: Option<&str>, url: &str, - branch_name: Option<&str>, - remote_branch_name: Option<&str>, + updates: &[PrePushRef], ) -> Result { scope_time!("hooks_pre_push"); let repo = repo(repo_path)?; - Ok(git2_hooks::hooks_pre_push( - &repo, - None, - remote, - url, - branch_name, - remote_branch_name, - )? - .into()) + Ok( + git2_hooks::hooks_pre_push( + &repo, None, remote, url, updates, + )? + .into(), + ) +} + +/// Build a single pre-push update line for a branch. +pub fn pre_push_branch_update( + repo_path: &RepoPath, + remote: Option<&str>, + branch_name: &str, + remote_branch_name: Option<&str>, + delete: bool, +) -> Result { + let repo = repo(repo_path)?; + let local_ref = format!("refs/heads/{branch_name}"); + let local_oid = (!delete) + .then(|| { + repo.find_branch(branch_name, BranchType::Local) + .ok() + .and_then(|branch| branch.get().peel_to_commit().ok()) + .map(|commit| commit.id()) + }) + .flatten(); + + let remote_branch = remote_branch_name.unwrap_or(branch_name); + let remote_ref = format!("refs/heads/{remote_branch}"); + + let remote_oid = remote.and_then(|remote_name| { + repo.find_reference(&format!( + "refs/remotes/{remote_name}/{remote_branch}" + )) + .ok() + .and_then(|r| r.peel_to_commit().ok()) + .map(|c| c.id()) + }); + + Ok(PrePushRef::new( + local_ref, local_oid, remote_ref, remote_oid, + )) +} + +/// Build pre-push updates for tags that are missing on the remote. +pub fn pre_push_tag_updates( + repo_path: &RepoPath, + remote: &str, +) -> Result> { + let repo = repo(repo_path)?; + let tags = tags_missing_remote(repo_path, remote, None)?; + let mut updates = Vec::with_capacity(tags.len()); + + for tag_ref in tags { + if let Ok(reference) = repo.find_reference(&tag_ref) { + let tag_oid = reference.target().or_else(|| { + reference.peel_to_commit().ok().map(|c| c.id()) + }); + + updates.push(PrePushRef::new( + tag_ref.clone(), + tag_oid, + tag_ref, + None, + )); + } + } + + Ok(updates) } #[cfg(test)] diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index c5c7901cc2..eb92118fb9 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -67,7 +67,8 @@ pub use diff::get_diff_commit; pub use git2::BranchType; pub use hooks::{ hooks_commit_msg, hooks_post_commit, hooks_pre_commit, - hooks_pre_push, hooks_prepare_commit_msg, HookResult, + hooks_pre_push, hooks_prepare_commit_msg, pre_push_branch_update, + pre_push_tag_updates, HookResult, PrePushRef, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 68694323f9..b06d19c3a7 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -38,7 +38,7 @@ pub use error::HooksError; use error::Result; use hookspath::HookPaths; -use git2::Repository; +use git2::{Oid, Repository}; pub const HOOK_POST_COMMIT: &str = "post-commit"; pub const HOOK_PRE_COMMIT: &str = "pre-commit"; @@ -48,6 +48,46 @@ pub const HOOK_PRE_PUSH: &str = "pre-push"; const HOOK_COMMIT_MSG_TEMP_FILE: &str = "COMMIT_EDITMSG"; +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct PrePushRef { + pub local_ref: String, + pub local_oid: Option, + pub remote_ref: String, + pub remote_oid: Option, +} + +impl PrePushRef { + #[must_use] + pub fn new( + local_ref: impl Into, + local_oid: Option, + remote_ref: impl Into, + remote_oid: Option, + ) -> Self { + Self { + local_ref: local_ref.into(), + local_oid, + remote_ref: remote_ref.into(), + remote_oid, + } + } + + fn format_oid(oid: Option) -> String { + oid.map_or_else(|| "0".repeat(40), |id| id.to_string()) + } + + #[must_use] + pub fn to_line(&self) -> String { + format!( + "{} {} {} {}", + self.local_ref, + Self::format_oid(self.local_oid), + self.remote_ref, + Self::format_oid(self.remote_oid) + ) + } +} + /// Response from running a hook #[derive(Debug, PartialEq, Eq)] pub struct HookRunResponse { @@ -192,17 +232,12 @@ pub fn hooks_post_commit( /// ` SP SP SP LF` /// /// If `remote` is `None` or empty, the `url` is used for both arguments as per Git spec. -/// -/// Parameters: -/// - `branch_name`: Optional local branch name being pushed (e.g., "main"). If `None`, stdin will be empty. -/// - `remote_branch_name`: Optional remote branch name (if different from local) pub fn hooks_pre_push( repo: &Repository, other_paths: Option<&[&str]>, remote: Option<&str>, url: &str, - branch_name: Option<&str>, - remote_branch_name: Option<&str>, + updates: &[PrePushRef], ) -> Result { let hook = HookPaths::new(repo, other_paths, HOOK_PRE_PUSH)?; @@ -219,44 +254,11 @@ pub fn hooks_pre_push( // Build stdin according to Git pre-push hook spec: // SP SP SP LF - let stdin_data = if let Some(branch) = branch_name { - // Get local branch reference and commit - let local_branch = - repo.find_branch(branch, git2::BranchType::Local)?; - let local_ref = format!("refs/heads/{branch}"); - let local_commit = local_branch.get().peel_to_commit()?; - let local_sha = local_commit.id().to_string(); - - // Determine remote branch name (same as local if not specified) - let remote_branch = remote_branch_name.unwrap_or(branch); - let remote_ref = format!("refs/heads/{remote_branch}"); - - // Try to get remote commit SHA from the actual push target remote - // We need to look up refs/remotes//, not the upstream - let remote_sha = if let Some(remote_name) = remote { - // Try to find the remote tracking branch for the push target - let remote_ref_name = - format!("refs/remotes/{remote_name}/{remote_branch}"); - if let Ok(remote_ref) = - repo.find_reference(&remote_ref_name) - { - // Remote branch exists, get its SHA - remote_ref.peel_to_commit()?.id().to_string() - } else { - // Remote branch doesn't exist yet (new branch on remote) - "0".repeat(40) - } - } else { - // No remote specified (shouldn't happen in practice) - "0".repeat(40) - }; - - // Format: refs/heads/branch local_sha refs/heads/branch remote_sha\n - format!("{local_ref} {local_sha} {remote_ref} {remote_sha}\n") - } else { - // No branch specified (e.g., pushing tags), use empty stdin - String::new() - }; + let mut stdin_data = String::new(); + for update in updates { + stdin_data.push_str(&update.to_line()); + stdin_data.push('\n'); + } hook.run_hook_os_str_with_stdin( [remote_name, url], @@ -330,6 +332,48 @@ mod tests { use pretty_assertions::assert_eq; use tempfile::TempDir; + fn branch_update( + repo: &Repository, + remote: Option<&str>, + branch: &str, + remote_branch: Option<&str>, + delete: bool, + ) -> PrePushRef { + let local_ref = format!("refs/heads/{branch}"); + let local_oid = (!delete).then(|| { + repo.find_branch(branch, git2::BranchType::Local) + .unwrap() + .get() + .peel_to_commit() + .unwrap() + .id() + }); + + let remote_branch = remote_branch.unwrap_or(branch); + let remote_ref = format!("refs/heads/{remote_branch}"); + let remote_oid = remote.and_then(|remote_name| { + repo.find_reference(&format!( + "refs/remotes/{remote_name}/{remote_branch}" + )) + .ok() + .and_then(|r| r.peel_to_commit().ok()) + .map(|c| c.id()) + }); + + PrePushRef::new(local_ref, local_oid, remote_ref, remote_oid) + } + + fn stdin_from_updates(updates: &[PrePushRef]) -> String { + updates + .iter() + .map(|u| format!("{}\n", u.to_line())) + .collect() + } + + fn head_branch(repo: &Repository) -> String { + repo.head().unwrap().shorthand().unwrap().to_string() + } + #[test] fn test_smoke() { let (_td, repo) = repo_init(); @@ -756,13 +800,21 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://example.com/repo.git", - None, - None, + &updates, ) .unwrap(); @@ -778,13 +830,22 @@ echo 'failed' exit 3 "; create_hook(&repo, HOOK_PRE_PUSH, hook); + + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://example.com/repo.git", - None, - None, + &updates, ) .unwrap(); let HookResult::Run(response) = res else { @@ -806,13 +867,16 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = + [branch_update(&repo, None, &branch, None, false)]; + let res = hooks_pre_push( &repo, None, None, "https://example.com/repo.git", - None, - None, + &updates, ) .unwrap(); @@ -836,13 +900,21 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://example.com/repo.git", - None, - None, + &updates, ) .unwrap(); @@ -866,12 +938,119 @@ exit 0 response.stdout ); assert!( - response.stdout.contains("stdin_length=0"), - "Expected stdin to be empty (length 0), got: {}", + response.stdout.contains("stdin_length=") + && !response.stdout.contains("stdin_length=0"), + "Expected stdin to be non-empty, got: {}", response.stdout ); } + #[test] + fn test_pre_push_multiple_updates() { + let (_td, repo) = repo_init(); + + let hook = b"#!/bin/sh +cat +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let branch = head_branch(&repo); + let branch_update = branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + ); + + // create a tag to add a second refspec + let head_commit = + repo.head().unwrap().peel_to_commit().unwrap(); + repo.tag_lightweight("v1", head_commit.as_object(), false) + .unwrap(); + let tag_ref = repo.find_reference("refs/tags/v1").unwrap(); + let tag_oid = tag_ref.target().unwrap(); + let tag_update = PrePushRef::new( + "refs/tags/v1", + Some(tag_oid), + "refs/tags/v1", + None, + ); + + let updates = [branch_update, tag_update]; + let expected_stdin = stdin_from_updates(&updates); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://example.com/repo.git", + &updates, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + unreachable!("Expected Run result, got: {res:?}") + }; + + assert!( + response.is_successful(), + "Hook should succeed: stdout {} stderr {}", + response.stdout, + response.stderr + ); + assert_eq!( + response.stdout, expected_stdin, + "stdin should include all refspec lines" + ); + } + + #[test] + fn test_pre_push_delete_ref_uses_zero_oid() { + let (_td, repo) = repo_init(); + + let hook = b"#!/bin/sh +cat +exit 0 + "; + + create_hook(&repo, HOOK_PRE_PUSH, hook); + + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + true, + )]; + let expected_stdin = stdin_from_updates(&updates); + + let res = hooks_pre_push( + &repo, + None, + Some("origin"), + "https://example.com/repo.git", + &updates, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + unreachable!("Expected Run result, got: {res:?}") + }; + + assert!(response.is_successful()); + assert_eq!(response.stdout, expected_stdin); + assert!( + response + .stdout + .contains("0000000000000000000000000000000000000000"), + "delete pushes must use zero oid for new object" + ); + } + #[test] fn test_pre_push_verifies_arguments() { let (_td, repo) = repo_init(); @@ -893,13 +1072,21 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://github.com/user/repo.git", - None, - None, + &updates, ) .unwrap(); @@ -922,28 +1109,34 @@ exit 0 let (_td, repo) = repo_init(); // Hook that checks stdin content - // Currently we pass empty stdin, this test documents that behavior let hook = b"#!/bin/sh -stdin_content=$(cat) -if [ -z \"$stdin_content\" ]; then - echo \"stdin is empty (expected for current implementation)\" - exit 0 -else - echo \"stdin_length=${#stdin_content}\" - echo \"stdin_content=$stdin_content\" - exit 0 -fi - "; + stdin_content=$(cat) + if [ -z \"$stdin_content\" ]; then + echo \"stdin was unexpectedly empty\" >&2 + exit 1 + fi + echo \"stdin_length=${#stdin_content}\" + echo \"stdin_content=$stdin_content\" + exit 0 + "; create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://github.com/user/repo.git", - None, - None, + &updates, ) .unwrap(); @@ -953,10 +1146,9 @@ fi assert!(response.is_successful(), "Hook should succeed"); - // Verify stdin is currently empty assert!( - response.stdout.contains("stdin is empty"), - "Expected stdin to be empty, got: {}", + response.stdout.contains("stdin_length="), + "Expected stdin to be non-empty, got: {}", response.stdout ); } @@ -1000,13 +1192,21 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("origin"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("origin"), "https://github.com/user/repo.git", - Some("master"), - None, + &updates, ) .unwrap(); @@ -1130,13 +1330,21 @@ exit 1 // Push to "backup" remote (which doesn't have master yet) // This is different from the upstream "origin" + let branch = head_branch(&repo); + let updates = [branch_update( + &repo, + Some("backup"), + &branch, + None, + false, + )]; + let res = hooks_pre_push( &repo, None, Some("backup"), "https://github.com/user/backup-repo.git", - Some("master"), - None, + &updates, ) .unwrap(); diff --git a/src/popups/push.rs b/src/popups/push.rs index a19cad2d24..cc89fef0f8 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -161,13 +161,22 @@ impl PushPopup { return Ok(()); }; + // build pre-push updates (single branch) + let repo = self.repo.borrow(); + let branch_update = asyncgit::sync::pre_push_branch_update( + &repo, + Some(&remote), + &self.branch, + None, + self.modifier.delete(), + )?; + // run pre push hook - can reject push if let HookResult::NotOk(e) = hooks_pre_push( - &self.repo.borrow(), + &repo, Some(&remote), &remote_url, - Some(&self.branch), - None, + &[branch_update], )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( diff --git a/src/popups/push_tags.rs b/src/popups/push_tags.rs index 7edbb1a094..d7243f762a 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -103,13 +103,17 @@ impl PushTagsPopup { return Ok(()); }; + // build updates for tags we are going to push + let repo = self.repo.borrow(); + let updates = + asyncgit::sync::pre_push_tag_updates(&repo, &remote)?; + // run pre push hook - can reject push if let HookResult::NotOk(e) = hooks_pre_push( - &self.repo.borrow(), + &repo, Some(&remote), &remote_url, - None, - None, + &updates, )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( From 5aab2e05b373f813dcccf540d2b7517dcbbc7527 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 13:46:00 -0500 Subject: [PATCH 06/20] use correct remote advertised tips for pre_push hook --- asyncgit/src/sync/hooks.rs | 131 ++++++++++++++++++++++--------------- asyncgit/src/sync/mod.rs | 7 +- src/popups/push.rs | 17 ++--- src/popups/push_tags.rs | 8 +-- 4 files changed, 91 insertions(+), 72 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 7fd9efd462..92c7de6097 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -1,10 +1,14 @@ use super::{repository::repo, RepoPath}; use crate::{ - error::Result, sync::remotes::tags::tags_missing_remote, + error::Result, + sync::remotes::{ + proxy_auto, tags::tags_missing_remote, Callbacks, + }, }; -use git2::BranchType; +use git2::{BranchType, Direction, Oid}; pub use git2_hooks::{PrePushRef, PrepareCommitMsgSource}; use scopetime::scope_time; +use std::collections::HashMap; /// #[derive(Debug, PartialEq, Eq)] @@ -33,36 +37,32 @@ impl From for HookResult { } } -fn pre_push_branch_update( - repo: &git2::Repository, +/// Retrieve advertised refs from the remote for the upcoming push. +pub fn advertised_remote_refs( + repo_path: &RepoPath, remote: Option<&str>, - branch_name: &str, - remote_branch_name: Option<&str>, - delete: bool, -) -> PrePushRef { - let local_ref = format!("refs/heads/{branch_name}"); - let local_oid = (!delete) - .then(|| { - repo.find_branch(branch_name, BranchType::Local) - .ok() - .and_then(|branch| branch.get().peel_to_commit().ok()) - .map(|commit| commit.id()) - }) - .flatten(); - - let remote_branch = remote_branch_name.unwrap_or(branch_name); - let remote_ref = format!("refs/heads/{remote_branch}"); - - let remote_oid = remote.and_then(|remote_name| { - repo.find_reference(&format!( - "refs/remotes/{remote_name}/{remote_branch}" - )) - .ok() - .and_then(|r| r.peel_to_commit().ok()) - .map(|c| c.id()) - }); + url: &str, +) -> Result> { + let repo = repo(repo_path)?; + let mut remote_handle = if let Some(name) = remote { + repo.find_remote(name)? + } else { + repo.remote_anonymous(url)? + }; + + let callbacks = Callbacks::new(None, None); + let conn = remote_handle.connect_auth( + Direction::Push, + Some(callbacks.callbacks()), + Some(proxy_auto()), + )?; + + let mut map = HashMap::new(); + for head in conn.list()? { + map.insert(head.name().to_string(), head.oid()); + } - PrePushRef::new(local_ref, local_oid, remote_ref, remote_oid) + Ok(map) } /// see `git2_hooks::hooks_commit_msg` @@ -116,27 +116,45 @@ pub fn hooks_pre_push( repo_path: &RepoPath, remote: Option<&str>, url: &str, - updates: &[PrePushRef], + push: &PrePushTarget<'_>, ) -> Result { scope_time!("hooks_pre_push"); let repo = repo(repo_path)?; + let advertised = advertised_remote_refs(repo_path, remote, url)?; + let updates = match push { + PrePushTarget::Branch { + branch, + remote_branch, + delete, + } => vec![pre_push_branch_update( + repo_path, + branch, + *remote_branch, + *delete, + &advertised, + )?], + PrePushTarget::Tags => { + // If remote is None, use url per git spec + let remote = remote.unwrap_or(url); + pre_push_tag_updates(repo_path, remote, &advertised)? + } + }; - Ok( - git2_hooks::hooks_pre_push( - &repo, None, remote, url, updates, - )? - .into(), - ) + Ok(git2_hooks::hooks_pre_push( + &repo, None, remote, url, &updates, + )? + .into()) } /// Build a single pre-push update line for a branch. -pub fn pre_push_branch_update( +#[allow(clippy::implicit_hasher)] +fn pre_push_branch_update( repo_path: &RepoPath, - remote: Option<&str>, branch_name: &str, remote_branch_name: Option<&str>, delete: bool, + advertised: &HashMap, ) -> Result { let repo = repo(repo_path)?; let local_ref = format!("refs/heads/{branch_name}"); @@ -152,14 +170,7 @@ pub fn pre_push_branch_update( let remote_branch = remote_branch_name.unwrap_or(branch_name); let remote_ref = format!("refs/heads/{remote_branch}"); - let remote_oid = remote.and_then(|remote_name| { - repo.find_reference(&format!( - "refs/remotes/{remote_name}/{remote_branch}" - )) - .ok() - .and_then(|r| r.peel_to_commit().ok()) - .map(|c| c.id()) - }); + let remote_oid = advertised.get(&remote_ref).copied(); Ok(PrePushRef::new( local_ref, local_oid, remote_ref, remote_oid, @@ -167,9 +178,11 @@ pub fn pre_push_branch_update( } /// Build pre-push updates for tags that are missing on the remote. -pub fn pre_push_tag_updates( +#[allow(clippy::implicit_hasher)] +fn pre_push_tag_updates( repo_path: &RepoPath, remote: &str, + advertised: &HashMap, ) -> Result> { let repo = repo(repo_path)?; let tags = tags_missing_remote(repo_path, remote, None)?; @@ -180,12 +193,13 @@ pub fn pre_push_tag_updates( let tag_oid = reference.target().or_else(|| { reference.peel_to_commit().ok().map(|c| c.id()) }); - + let remote_ref = tag_ref.clone(); + let advertised_oid = advertised.get(&remote_ref).copied(); updates.push(PrePushRef::new( tag_ref.clone(), tag_oid, - tag_ref, - None, + remote_ref, + advertised_oid, )); } } @@ -193,6 +207,21 @@ pub fn pre_push_tag_updates( Ok(updates) } +/// What is being pushed. +pub enum PrePushTarget<'a> { + /// Push a single branch. + Branch { + /// Local branch name being pushed. + branch: &'a str, + /// Optional remote branch name if different from local. + remote_branch: Option<&'a str>, + /// Whether this is a delete push. + delete: bool, + }, + /// Push tags. + Tags, +} + #[cfg(test)] mod tests { use std::{ffi::OsString, io::Write as _, path::Path}; diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index eb92118fb9..e8566a1cdb 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -66,10 +66,9 @@ 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_pre_push, hooks_prepare_commit_msg, pre_push_branch_update, - pre_push_tag_updates, HookResult, PrePushRef, - PrepareCommitMsgSource, + advertised_remote_refs, hooks_commit_msg, hooks_post_commit, + hooks_pre_commit, hooks_pre_push, hooks_prepare_commit_msg, + HookResult, PrePushRef, PrePushTarget, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; diff --git a/src/popups/push.rs b/src/popups/push.rs index cc89fef0f8..f79b14a7ca 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -161,22 +161,17 @@ impl PushPopup { return Ok(()); }; - // build pre-push updates (single branch) - let repo = self.repo.borrow(); - let branch_update = asyncgit::sync::pre_push_branch_update( - &repo, - Some(&remote), - &self.branch, - None, - self.modifier.delete(), - )?; - // run pre push hook - can reject push + let repo = self.repo.borrow(); if let HookResult::NotOk(e) = hooks_pre_push( &repo, Some(&remote), &remote_url, - &[branch_update], + &asyncgit::sync::PrePushTarget::Branch { + branch: &self.branch, + remote_branch: None, + delete: self.modifier.delete(), + }, )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( diff --git a/src/popups/push_tags.rs b/src/popups/push_tags.rs index d7243f762a..f555ba9397 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -103,17 +103,13 @@ impl PushTagsPopup { return Ok(()); }; - // build updates for tags we are going to push - let repo = self.repo.borrow(); - let updates = - asyncgit::sync::pre_push_tag_updates(&repo, &remote)?; - // run pre push hook - can reject push + let repo = self.repo.borrow(); if let HookResult::NotOk(e) = hooks_pre_push( &repo, Some(&remote), &remote_url, - &updates, + &asyncgit::sync::PrePushTarget::Tags, )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( From f8ce43f67dc8cfacf090e69105eef98f90f35514 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:08:30 -0500 Subject: [PATCH 07/20] fix branch destination ref logic 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 `` field matches the refspec Git would use. --- asyncgit/src/sync/hooks.rs | 61 +++++++++++++++++++++++++++----------- src/popups/push.rs | 1 - 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 92c7de6097..ae783479c4 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -5,6 +5,12 @@ use crate::{ proxy_auto, tags::tags_missing_remote, Callbacks, }, }; +use crate::{ + sync::branch::get_branch_upstream_merge, + sync::config::{ + push_default_strategy_config_repo, PushDefaultStrategyConfig, + }, +}; use git2::{BranchType, Direction, Oid}; pub use git2_hooks::{PrePushRef, PrepareCommitMsgSource}; use scopetime::scope_time; @@ -65,6 +71,29 @@ pub fn advertised_remote_refs( Ok(map) } +fn branch_push_destination_ref( + repo_path: &RepoPath, + branch: &str, + delete: bool, +) -> Result { + let repo = repo(repo_path)?; + let push_default_strategy = + push_default_strategy_config_repo(&repo)?; + + if !delete + && push_default_strategy + == PushDefaultStrategyConfig::Upstream + { + if let Ok(Some(branch_upstream_merge)) = + get_branch_upstream_merge(repo_path, branch) + { + return Ok(branch_upstream_merge); + } + } + + Ok(format!("refs/heads/{branch}")) +} + /// see `git2_hooks::hooks_commit_msg` pub fn hooks_commit_msg( repo_path: &RepoPath, @@ -123,17 +152,18 @@ pub fn hooks_pre_push( let repo = repo(repo_path)?; let advertised = advertised_remote_refs(repo_path, remote, url)?; let updates = match push { - PrePushTarget::Branch { - branch, - remote_branch, - delete, - } => vec![pre_push_branch_update( - repo_path, - branch, - *remote_branch, - *delete, - &advertised, - )?], + PrePushTarget::Branch { branch, delete } => { + let remote_ref = branch_push_destination_ref( + repo_path, branch, *delete, + )?; + vec![pre_push_branch_update( + repo_path, + branch, + &remote_ref, + *delete, + &advertised, + )?] + } PrePushTarget::Tags => { // If remote is None, use url per git spec let remote = remote.unwrap_or(url); @@ -152,7 +182,7 @@ pub fn hooks_pre_push( fn pre_push_branch_update( repo_path: &RepoPath, branch_name: &str, - remote_branch_name: Option<&str>, + remote_ref: &str, delete: bool, advertised: &HashMap, ) -> Result { @@ -167,10 +197,7 @@ fn pre_push_branch_update( }) .flatten(); - let remote_branch = remote_branch_name.unwrap_or(branch_name); - let remote_ref = format!("refs/heads/{remote_branch}"); - - let remote_oid = advertised.get(&remote_ref).copied(); + let remote_oid = advertised.get(remote_ref).copied(); Ok(PrePushRef::new( local_ref, local_oid, remote_ref, remote_oid, @@ -213,8 +240,6 @@ pub enum PrePushTarget<'a> { Branch { /// Local branch name being pushed. branch: &'a str, - /// Optional remote branch name if different from local. - remote_branch: Option<&'a str>, /// Whether this is a delete push. delete: bool, }, diff --git a/src/popups/push.rs b/src/popups/push.rs index f79b14a7ca..8c572ebabb 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -169,7 +169,6 @@ impl PushPopup { &remote_url, &asyncgit::sync::PrePushTarget::Branch { branch: &self.branch, - remote_branch: None, delete: self.modifier.delete(), }, )? { From 5d7f7494f7617a6fd7f0e48e8160a52cecb29dcb Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:13:45 -0500 Subject: [PATCH 08/20] skip remote access if pre_push not set --- asyncgit/src/sync/hooks.rs | 8 ++++++++ git2-hooks/src/lib.rs | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index ae783479c4..e03a2b7a80 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -150,6 +150,14 @@ pub fn hooks_pre_push( scope_time!("hooks_pre_push"); let repo = repo(repo_path)?; + if !git2_hooks::hook_available( + &repo, + None, + git2_hooks::HOOK_PRE_PUSH, + )? { + return Ok(HookResult::Ok); + } + let advertised = advertised_remote_refs(repo_path, remote, url)?; let updates = match push { PrePushTarget::Branch { branch, delete } => { diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index b06d19c3a7..66a243fa4c 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -48,6 +48,16 @@ pub const HOOK_PRE_PUSH: &str = "pre-push"; const HOOK_COMMIT_MSG_TEMP_FILE: &str = "COMMIT_EDITMSG"; +/// Check if a given hook is present considering config/paths and optional extra paths. +pub fn hook_available( + repo: &Repository, + other_paths: Option<&[&str]>, + hook: &str, +) -> Result { + let hook = HookPaths::new(repo, other_paths, hook)?; + Ok(hook.found()) +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct PrePushRef { pub local_ref: String, From 4a33af43977d394abee2d81d1465ba54c39a20f0 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:27:25 -0500 Subject: [PATCH 09/20] provide same creds to hooks call as to push --- asyncgit/src/sync/hooks.rs | 11 +++++++++-- asyncgit/src/sync/mod.rs | 2 +- src/popups/push.rs | 1 + src/popups/push_tags.rs | 1 + 4 files changed, 12 insertions(+), 3 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index e03a2b7a80..ce4889eb96 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -48,6 +48,7 @@ pub fn advertised_remote_refs( repo_path: &RepoPath, remote: Option<&str>, url: &str, + basic_credential: Option, ) -> Result> { let repo = repo(repo_path)?; let mut remote_handle = if let Some(name) = remote { @@ -56,7 +57,7 @@ pub fn advertised_remote_refs( repo.remote_anonymous(url)? }; - let callbacks = Callbacks::new(None, None); + let callbacks = Callbacks::new(None, basic_credential); let conn = remote_handle.connect_auth( Direction::Push, Some(callbacks.callbacks()), @@ -146,6 +147,7 @@ pub fn hooks_pre_push( remote: Option<&str>, url: &str, push: &PrePushTarget<'_>, + basic_credential: Option, ) -> Result { scope_time!("hooks_pre_push"); @@ -158,7 +160,12 @@ pub fn hooks_pre_push( return Ok(HookResult::Ok); } - let advertised = advertised_remote_refs(repo_path, remote, url)?; + let advertised = advertised_remote_refs( + repo_path, + remote, + url, + basic_credential, + )?; let updates = match push { PrePushTarget::Branch { branch, delete } => { let remote_ref = branch_push_destination_ref( diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index e8566a1cdb..edaf75fc0d 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -68,7 +68,7 @@ pub use git2::BranchType; pub use hooks::{ advertised_remote_refs, hooks_commit_msg, hooks_post_commit, hooks_pre_commit, hooks_pre_push, hooks_prepare_commit_msg, - HookResult, PrePushRef, PrePushTarget, PrepareCommitMsgSource, + HookResult, PrePushTarget, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; diff --git a/src/popups/push.rs b/src/popups/push.rs index 8c572ebabb..26af35d1da 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -171,6 +171,7 @@ impl PushPopup { branch: &self.branch, delete: self.modifier.delete(), }, + cred.clone(), )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( diff --git a/src/popups/push_tags.rs b/src/popups/push_tags.rs index f555ba9397..685a4d18a4 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -110,6 +110,7 @@ impl PushTagsPopup { Some(&remote), &remote_url, &asyncgit::sync::PrePushTarget::Tags, + cred.clone(), )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( From a3c73ef6024db1bf6a415b639bdd0ec5cb8bd613 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:36:30 -0500 Subject: [PATCH 10/20] cleanup --- asyncgit/src/sync/hooks.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index ce4889eb96..4cb0cb7d48 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -1,14 +1,13 @@ use super::{repository::repo, RepoPath}; use crate::{ error::Result, - sync::remotes::{ - proxy_auto, tags::tags_missing_remote, Callbacks, - }, -}; -use crate::{ - sync::branch::get_branch_upstream_merge, - sync::config::{ - push_default_strategy_config_repo, PushDefaultStrategyConfig, + sync::{ + branch::get_branch_upstream_merge, + config::{ + push_default_strategy_config_repo, + PushDefaultStrategyConfig, + }, + remotes::{proxy_auto, tags::tags_missing_remote, Callbacks}, }, }; use git2::{BranchType, Direction, Oid}; From b1802f4d9800558a8c0c003b5a5b18d88d4fe7f4 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:53:38 -0500 Subject: [PATCH 11/20] simplify and document --- asyncgit/src/sync/hooks.rs | 31 +++++++++++++++++++++---------- 1 file changed, 21 insertions(+), 10 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 4cb0cb7d48..f3f5c535f8 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -71,26 +71,38 @@ pub fn advertised_remote_refs( Ok(map) } -fn branch_push_destination_ref( +/// Determine the remote ref name for a branch push. +/// +/// Respects `push.default=upstream` config when set and upstream is configured. +/// Otherwise defaults to `refs/heads/{branch}`. Delete operations always use +/// the simple ref name. +fn get_remote_ref_for_push( repo_path: &RepoPath, branch: &str, delete: bool, ) -> Result { + // For delete operations, always use the simple ref name + // regardless of push.default configuration + if delete { + return Ok(format!("refs/heads/{branch}")); + } + let repo = repo(repo_path)?; let push_default_strategy = push_default_strategy_config_repo(&repo)?; - if !delete - && push_default_strategy - == PushDefaultStrategyConfig::Upstream - { - if let Ok(Some(branch_upstream_merge)) = + // When push.default=upstream, use the configured upstream ref if available + if push_default_strategy == PushDefaultStrategyConfig::Upstream { + if let Ok(Some(upstream_ref)) = get_branch_upstream_merge(repo_path, branch) { - return Ok(branch_upstream_merge); + return Ok(upstream_ref); } + // If upstream strategy is set but no upstream is configured, + // fall through to default behavior } + // Default: push to remote branch with same name as local Ok(format!("refs/heads/{branch}")) } @@ -167,9 +179,8 @@ pub fn hooks_pre_push( )?; let updates = match push { PrePushTarget::Branch { branch, delete } => { - let remote_ref = branch_push_destination_ref( - repo_path, branch, *delete, - )?; + let remote_ref = + get_remote_ref_for_push(repo_path, branch, *delete)?; vec![pre_push_branch_update( repo_path, branch, From a478074f7bf3a22eecc1b1514e3a669fc6472928 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:55:38 -0500 Subject: [PATCH 12/20] improve error handling --- asyncgit/src/sync/hooks.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index f3f5c535f8..12ba651a67 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -32,10 +32,16 @@ impl From for HookResult { if response.is_successful() { Self::Ok } else { - Self::NotOk(format!( - "{}{}", - response.stdout, response.stderr - )) + Self::NotOk(if response.stderr.is_empty() { + response.stdout + } else if response.stdout.is_empty() { + response.stderr + } else { + format!( + "{}\n{}", + response.stdout, response.stderr + ) + }) } } } From a81cc144980df13dc709fb497f0f3139ad26c95d Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 14:59:15 -0500 Subject: [PATCH 13/20] simplify stdin --- git2-hooks/src/hookspath.rs | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index ad7f3d2157..8a611b7efb 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -171,16 +171,20 @@ impl HookPaths { .args(args) .current_dir(&self.pwd) .with_no_window() - .stdin(std::process::Stdio::piped()) + .stdin(if stdin.is_some() { + std::process::Stdio::piped() + } else { + std::process::Stdio::null() + }) .stdout(std::process::Stdio::piped()) .stderr(std::process::Stdio::piped()) .spawn()?; - if let Some(mut stdin_handle) = child.stdin.take() { + if let (Some(mut stdin_handle), Some(input)) = + (child.stdin.take(), stdin) + { use std::io::Write; - if let Some(input) = stdin { - stdin_handle.write_all(input)?; - } + stdin_handle.write_all(input)?; drop(stdin_handle); } From cf8d1cbeabdd480dd33fd2ad9604b14fbabe7e56 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:01:11 -0500 Subject: [PATCH 14/20] revert unrelated change --- src/clipboard.rs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/clipboard.rs b/src/clipboard.rs index 20b10cc383..79373a5b12 100644 --- a/src/clipboard.rs +++ b/src/clipboard.rs @@ -26,15 +26,12 @@ fn exec_copy_with_args( .spawn() .map_err(|e| anyhow!("`{command:?}`: {e:?}"))?; - let mut stdin = process + process .stdin - .take() - .ok_or_else(|| anyhow!("`{command:?}`"))?; - - stdin + .as_mut() + .ok_or_else(|| anyhow!("`{command:?}`"))? .write_all(text.as_bytes()) .map_err(|e| anyhow!("`{command:?}`: {e:?}"))?; - drop(stdin); let out = process .wait_with_output() From 2561dc98441aafc894d023e9606ee4ad8dca896a Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:03:46 -0500 Subject: [PATCH 15/20] add comment --- git2-hooks/src/lib.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index 66a243fa4c..627c834682 100644 --- a/git2-hooks/src/lib.rs +++ b/git2-hooks/src/lib.rs @@ -242,6 +242,9 @@ pub fn hooks_post_commit( /// ` SP SP SP LF` /// /// If `remote` is `None` or empty, the `url` is used for both arguments as per Git spec. +/// +/// Note: The hook is called even when `updates` is empty (matching Git's behavior). +/// This can occur when pushing tags that already exist on the remote. pub fn hooks_pre_push( repo: &Repository, other_paths: Option<&[&str]>, From 8d2af0edb8b4857f34a1473c252562f08ebf0b22 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:05:15 -0500 Subject: [PATCH 16/20] cleanup uneeded allow --- asyncgit/src/sync/hooks.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 12ba651a67..c84c75327c 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -209,7 +209,6 @@ pub fn hooks_pre_push( } /// Build a single pre-push update line for a branch. -#[allow(clippy::implicit_hasher)] fn pre_push_branch_update( repo_path: &RepoPath, branch_name: &str, @@ -236,7 +235,6 @@ fn pre_push_branch_update( } /// Build pre-push updates for tags that are missing on the remote. -#[allow(clippy::implicit_hasher)] fn pre_push_tag_updates( repo_path: &RepoPath, remote: &str, From bc0f381a656ccf1b25a2069569eb12d8ca5286f2 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:07:16 -0500 Subject: [PATCH 17/20] cleanup: no need to be public --- asyncgit/src/sync/hooks.rs | 2 +- asyncgit/src/sync/mod.rs | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index c84c75327c..1eca617ef5 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -49,7 +49,7 @@ impl From for HookResult { } /// Retrieve advertised refs from the remote for the upcoming push. -pub fn advertised_remote_refs( +fn advertised_remote_refs( repo_path: &RepoPath, remote: Option<&str>, url: &str, diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index edaf75fc0d..2a5f413e8f 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -66,9 +66,9 @@ pub use config::{ pub use diff::get_diff_commit; pub use git2::BranchType; pub use hooks::{ - advertised_remote_refs, hooks_commit_msg, hooks_post_commit, - hooks_pre_commit, hooks_pre_push, hooks_prepare_commit_msg, - HookResult, PrePushTarget, PrepareCommitMsgSource, + hooks_commit_msg, hooks_post_commit, hooks_pre_commit, + hooks_pre_push, hooks_prepare_commit_msg, HookResult, + PrePushTarget, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; From b92803b4304101d7468f069631a27bc3c0fdf8d0 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:12:29 -0500 Subject: [PATCH 18/20] integration test --- asyncgit/src/sync/hooks.rs | 103 +++++++++++++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index 1eca617ef5..617ed408db 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -443,4 +443,107 @@ mod tests { assert_eq!(msg, String::from("msg\n")); } + + #[test] + fn test_pre_push_hook_receives_correct_stdin() { + let (_td, repo) = repo_init().unwrap(); + + // Create a pre-push hook that captures and validates stdin + let hook = b"#!/bin/sh +# Validate we receive correct format +stdin=$(cat) + +# Check we receive 4 space-separated fields +field_count=$(echo \"$stdin\" | awk '{print NF}') +if [ \"$field_count\" != \"4\" ]; then + echo \"ERROR: Expected 4 fields, got $field_count\" >&2 + exit 1 +fi + +# Check format contains refs/heads/ +if ! echo \"$stdin\" | grep -q \"^refs/heads/\"; then + echo \"ERROR: Invalid ref format\" >&2 + exit 1 +fi + +# Validate arguments +if [ \"$1\" != \"origin\" ]; then + echo \"ERROR: Wrong remote: $1\" >&2 + exit 1 +fi + +exit 0 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_PUSH, + hook, + ); + + // Directly test the git2-hooks layer with a simple update + let branch = + repo.head().unwrap().shorthand().unwrap().to_string(); + let commit_id = repo.head().unwrap().target().unwrap(); + let update = git2_hooks::PrePushRef::new( + format!("refs/heads/{}", branch), + Some(commit_id), + format!("refs/heads/{}", branch), + None, + ); + + let res = git2_hooks::hooks_pre_push( + &repo, + None, + Some("origin"), + "https://github.com/test/repo.git", + &[update], + ) + .unwrap(); + + // Hook should succeed + assert!(res.is_ok()); + } + + #[test] + fn test_pre_push_hook_rejects_based_on_stdin() { + let (_td, repo) = repo_init().unwrap(); + + // Create a hook that rejects pushes to master branch + let hook = b"#!/bin/sh +stdin=$(cat) +if echo \"$stdin\" | grep -q \"refs/heads/master\"; then + echo \"Direct pushes to master not allowed\" >&2 + exit 1 +fi +exit 0 + "; + + git2_hooks::create_hook( + &repo, + git2_hooks::HOOK_PRE_PUSH, + hook, + ); + + // Try to push master branch + let commit_id = repo.head().unwrap().target().unwrap(); + let update = git2_hooks::PrePushRef::new( + "refs/heads/master", + Some(commit_id), + "refs/heads/master", + None, + ); + + let res = git2_hooks::hooks_pre_push( + &repo, + None, + Some("origin"), + "https://github.com/test/repo.git", + &[update], + ) + .unwrap(); + + // Hook should reject + assert!(res.is_not_successful()); + } } From 61951ff4e8e2eb1c91054ff76d3a38af4d90fcd3 Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 15:15:56 -0500 Subject: [PATCH 19/20] potential fix for broken pipe ci tests failing --- git2-hooks/src/hookspath.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index 8a611b7efb..a1b2cddf83 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -184,7 +184,8 @@ impl HookPaths { (child.stdin.take(), stdin) { use std::io::Write; - stdin_handle.write_all(input)?; + // Ignore broken pipe errors - the hook may exit without reading stdin + let _ = stdin_handle.write_all(input); drop(stdin_handle); } From 6998e42c73c64b07b0158f64922bd8e8583d49da Mon Sep 17 00:00:00 2001 From: extrawurst Date: Wed, 17 Dec 2025 17:42:17 -0500 Subject: [PATCH 20/20] add more logging for diagnostics --- git2-hooks/src/hookspath.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/git2-hooks/src/hookspath.rs b/git2-hooks/src/hookspath.rs index a1b2cddf83..5c35c9dfdb 100644 --- a/git2-hooks/src/hookspath.rs +++ b/git2-hooks/src/hookspath.rs @@ -183,10 +183,23 @@ impl HookPaths { if let (Some(mut stdin_handle), Some(input)) = (child.stdin.take(), stdin) { - use std::io::Write; - // Ignore broken pipe errors - the hook may exit without reading stdin - let _ = stdin_handle.write_all(input); - drop(stdin_handle); + use std::io::{ErrorKind, Write}; + + // Write stdin to hook process + // Ignore broken pipe - hook may exit early without reading all input + let _ = + stdin_handle.write_all(input).inspect_err(|e| { + match e.kind() { + ErrorKind::BrokenPipe => { + log::debug!( + "Hook closed stdin early" + ); + } + _ => log::warn!( + "Failed to write stdin to hook: {e}" + ), + } + }); } child.wait_with_output()