diff --git a/asyncgit/src/sync/hooks.rs b/asyncgit/src/sync/hooks.rs index fe1a91912d..617ed408db 100644 --- a/asyncgit/src/sync/hooks.rs +++ b/asyncgit/src/sync/hooks.rs @@ -1,7 +1,19 @@ use super::{repository::repo, RepoPath}; -use crate::error::Result; -pub use git2_hooks::PrepareCommitMsgSource; +use crate::{ + error::Result, + 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}; +pub use git2_hooks::{PrePushRef, PrepareCommitMsgSource}; use scopetime::scope_time; +use std::collections::HashMap; /// #[derive(Debug, PartialEq, Eq)] @@ -15,17 +27,91 @@ 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(if response.stderr.is_empty() { + response.stdout + } else if response.stdout.is_empty() { + response.stderr + } else { + format!( + "{}\n{}", + response.stdout, response.stderr + ) + }) + } + } } } } +/// Retrieve advertised refs from the remote for the upcoming push. +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 { + repo.find_remote(name)? + } else { + repo.remote_anonymous(url)? + }; + + let callbacks = Callbacks::new(None, basic_credential); + 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()); + } + + Ok(map) +} + +/// 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)?; + + // 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(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}")) +} + /// see `git2_hooks::hooks_commit_msg` pub fn hooks_commit_msg( repo_path: &RepoPath, @@ -73,12 +159,121 @@ 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, + push: &PrePushTarget<'_>, + basic_credential: Option, +) -> Result { 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); + } - Ok(git2_hooks::hooks_pre_push(&repo, None)?.into()) + let advertised = advertised_remote_refs( + repo_path, + remote, + url, + basic_credential, + )?; + let updates = match push { + PrePushTarget::Branch { branch, delete } => { + let remote_ref = + get_remote_ref_for_push(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); + pre_push_tag_updates(repo_path, remote, &advertised)? + } + }; + + Ok(git2_hooks::hooks_pre_push( + &repo, None, remote, url, &updates, + )? + .into()) +} + +/// Build a single pre-push update line for a branch. +fn pre_push_branch_update( + repo_path: &RepoPath, + branch_name: &str, + remote_ref: &str, + delete: bool, + advertised: &HashMap, +) -> 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_oid = advertised.get(remote_ref).copied(); + + Ok(PrePushRef::new( + local_ref, local_oid, remote_ref, remote_oid, + )) +} + +/// Build pre-push updates for tags that are missing on the remote. +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)?; + 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()) + }); + let remote_ref = tag_ref.clone(); + let advertised_oid = advertised.get(&remote_ref).copied(); + updates.push(PrePushRef::new( + tag_ref.clone(), + tag_oid, + remote_ref, + advertised_oid, + )); + } + } + + Ok(updates) +} + +/// What is being pushed. +pub enum PrePushTarget<'a> { + /// Push a single branch. + Branch { + /// Local branch name being pushed. + branch: &'a str, + /// Whether this is a delete push. + delete: bool, + }, + /// Push tags. + Tags, } #[cfg(test)] @@ -248,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()); + } } diff --git a/asyncgit/src/sync/mod.rs b/asyncgit/src/sync/mod.rs index c5c7901cc2..2a5f413e8f 100644 --- a/asyncgit/src/sync/mod.rs +++ b/asyncgit/src/sync/mod.rs @@ -68,7 +68,7 @@ pub use git2::BranchType; pub use hooks::{ hooks_commit_msg, hooks_post_commit, hooks_pre_commit, hooks_pre_push, hooks_prepare_commit_msg, HookResult, - PrepareCommitMsgSource, + PrePushTarget, PrepareCommitMsgSource, }; pub use hunks::{reset_hunk, stage_hunk, unstage_hunk}; pub use ignore::add_to_ignore; 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 33fe1bf659..5c35c9dfdb 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,42 @@ impl HookPaths { ); let run_command = |command: &mut Command| { - command + let mut child = command .args(args) .current_dir(&self.pwd) .with_no_window() - .output() + .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), Some(input)) = + (child.stdin.take(), stdin) + { + 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() }; let output = if cfg!(windows) { @@ -210,21 +255,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(); + + // 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, + })) } } diff --git a/git2-hooks/src/lib.rs b/git2-hooks/src/lib.rs index dd1fb66484..627c834682 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,37 +48,99 @@ 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, + 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 { + /// 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 (0 = success) + pub code: i32, +} + #[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 == 0, + Self::NoHookFound => true, + } } - /// 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 != 0, + Self::NoHookFound => false, + } + } +} + +impl HookRunResponse { + /// Check if the hook succeeded (exit code 0) + pub const fn is_successful(&self) -> bool { + self.code == 0 } } @@ -172,9 +234,23 @@ 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. +/// +/// 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]>, + remote: Option<&str>, + url: &str, + updates: &[PrePushRef], ) -> Result { let hook = HookPaths::new(repo, other_paths, HOOK_PRE_PUSH)?; @@ -182,7 +258,25 @@ 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 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], + Some(stdin_data.as_bytes()), + ) } pub enum PrepareCommitMsgSource { @@ -251,6 +345,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(); @@ -339,22 +475,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, 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 +578,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 +614,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, 1); + assert_eq!(&response.stdout, "rejected\n"); } #[test] @@ -562,13 +693,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, 1); + assert_eq!(&response.stdout, "rejected\n"); assert_eq!(msg, String::from("msg\n")); } @@ -633,7 +763,7 @@ exit 0 ) .unwrap(); - assert!(matches!(res, HookResult::Ok { .. })); + assert!(res.is_ok()); assert_eq!(msg, String::from("msg:message\n")); } @@ -658,13 +788,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, 2); + assert_eq!(&response.stdout, "rejected\n"); assert_eq!( msg, @@ -684,9 +813,25 @@ exit 0 create_hook(&repo, HOOK_PRE_PUSH, hook); - let res = hooks_pre_push(&repo, None).unwrap(); + 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", + &updates, + ) + .unwrap(); - assert!(matches!(res, HookResult::Ok { .. })); + assert!(res.is_ok()); } #[test] @@ -698,12 +843,534 @@ 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 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", + &updates, + ) + .unwrap(); + let HookResult::Run(response) = res else { unreachable!() }; - assert_eq!(code.unwrap(), 3); - assert_eq!(&stdout, "failed\n"); + assert_eq!(response.code, 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 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", + &updates, + ) + .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 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", + &updates, + ) + .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=") + && !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(); + + // 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 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", + &updates, + ) + .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 + let hook = b"#!/bin/sh + 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", + &updates, + ) + .unwrap(); + + let HookResult::Run(response) = res else { + unreachable!("Expected Run result, got: {res:?}") + }; + + assert!(response.is_successful(), "Hook should succeed"); + + assert!( + response.stdout.contains("stdin_length="), + "Expected stdin to be non-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 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", + &updates, + ) + .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 + ); + } + + #[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 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", + &updates, + ) + .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 f900ce8613..26af35d1da 100644 --- a/src/popups/push.rs +++ b/src/popups/push.rs @@ -144,10 +144,35 @@ impl PushPopup { remote }; + // get remote URL for pre-push hook + let remote_url = asyncgit::sync::get_remote_url( + &self.repo.borrow(), + &remote, + )?; + + // 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(&self.repo.borrow())? - { + let repo = self.repo.borrow(); + if let HookResult::NotOk(e) = hooks_pre_push( + &repo, + Some(&remote), + &remote_url, + &asyncgit::sync::PrePushTarget::Branch { + branch: &self.branch, + delete: self.modifier.delete(), + }, + cred.clone(), + )? { 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..685a4d18a4 100644 --- a/src/popups/push_tags.rs +++ b/src/popups/push_tags.rs @@ -84,10 +84,34 @@ 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, + )?; + + // 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(&self.repo.borrow())? - { + let repo = self.repo.borrow(); + if let HookResult::NotOk(e) = hooks_pre_push( + &repo, + Some(&remote), + &remote_url, + &asyncgit::sync::PrePushTarget::Tags, + cred.clone(), + )? { log::error!("pre-push hook failed: {e}"); self.queue.push(InternalEvent::ShowErrorMsg(format!( "pre-push hook failed:\n{e}" @@ -100,7 +124,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(())