diff --git a/src/git/cli_parser.rs b/src/git/cli_parser.rs index a3b294f81..90c05bc83 100644 --- a/src/git/cli_parser.rs +++ b/src/git/cli_parser.rs @@ -685,11 +685,12 @@ pub fn extract_clone_target_directory(args: &[String]) -> Option { /// Derive the target directory name from a repository URL. /// Mimics git's behavior of using the last path component, stripping .git suffix. fn derive_directory_from_url(url: &str) -> Option { - // Remove trailing slashes - let url = url.trim_end_matches('/'); + // Remove trailing slashes (both forward and backslash for Windows paths) + let url = url.trim_end_matches('/').trim_end_matches('\\'); - // Extract the last path component - let last_component = if let Some(pos) = url.rfind('/') { + // Extract the last path component. + // Check both '/' and '\\' to handle Windows local paths (e.g. C:\path\to\repo.git). + let last_component = if let Some(pos) = url.rfind(['/', '\\']) { &url[pos + 1..] } else if let Some(pos) = url.rfind(':') { // Handle SCP-like syntax: user@host:path @@ -808,6 +809,20 @@ mod tests { derive_directory_from_url("/local/path/repo.git"), Some("repo".to_string()) ); + // Windows-style local paths with backslashes + assert_eq!( + derive_directory_from_url("C:\\Users\\user\\repos\\repo.git"), + Some("repo".to_string()) + ); + assert_eq!( + derive_directory_from_url("C:\\Users\\user\\repos\\my-project"), + Some("my-project".to_string()) + ); + // Trailing backslash + assert_eq!( + derive_directory_from_url("C:\\Users\\user\\repos\\repo.git\\"), + Some("repo".to_string()) + ); } #[test] diff --git a/src/git/refs.rs b/src/git/refs.rs index 154ec7718..12199ff95 100644 --- a/src/git/refs.rs +++ b/src/git/refs.rs @@ -576,6 +576,102 @@ pub fn merge_notes_from_ref(repo: &Repository, source_ref: &str) -> Result<(), G Ok(()) } +/// Fallback merge when `git notes merge -s ours` fails (e.g., due to git assertion +/// failures on corrupted/mixed-fanout notes trees). Implements the "ours" strategy +/// using a single `git fast-import` invocation that: +/// 1. Creates a merge commit with both local and source as parents +/// 2. Emits all notes via `N ` commands (source first, then local — +/// last writer wins, so local takes precedence on conflicts = "ours" strategy) +/// 3. Produces a clean notes tree with correct fanout regardless of input tree format +/// +/// This is O(1) git process invocations regardless of note count, which matters on +/// large monorepos with thousands of notes. +pub fn fallback_merge_notes_ours(repo: &Repository, source_ref: &str) -> Result<(), GitAiError> { + let local_ref = format!("refs/notes/{}", AI_AUTHORSHIP_REFNAME); + + // 1. List notes from both refs + let source_notes = list_all_notes(repo, source_ref)?; + let local_notes = list_all_notes(repo, &local_ref)?; + + // 2. Resolve parent commit SHAs for the merge commit + let local_commit = rev_parse(repo, &local_ref)?; + let source_commit = rev_parse(repo, source_ref)?; + + // 3. Build the fast-import stream. + // Emit source (remote) notes first, then local notes. fast-import uses + // last-writer-wins for duplicate annotated objects, so local notes take + // precedence — this implements the "ours" merge strategy. + let mut stream = String::new(); + stream.push_str(&format!("commit {}\n", local_ref)); + stream.push_str("committer git-ai 0 +0000\n"); + stream.push_str("data 23\nMerge notes (fallback)\n"); + stream.push_str(&format!("from {}\n", local_commit)); + stream.push_str(&format!("merge {}\n", source_commit)); + + // Source notes first (will be overwritten by local on conflict) + for (blob, object) in &source_notes { + stream.push_str(&format!("N {} {}\n", blob, object)); + } + // Local notes second (wins on conflict) + for (blob, object) in &local_notes { + stream.push_str(&format!("N {} {}\n", blob, object)); + } + stream.push_str("done\n"); + + // 4. Run fast-import + let mut args = repo.global_args_for_exec(); + args.extend_from_slice(&[ + "fast-import".to_string(), + "--quiet".to_string(), + "--done".to_string(), + ]); + exec_git_stdin(&args, stream.as_bytes())?; + + debug_log("fallback merge via fast-import completed successfully"); + Ok(()) +} + +/// List all notes on a given ref. Returns Vec<(note_blob_sha, annotated_object_sha)>. +fn list_all_notes(repo: &Repository, notes_ref: &str) -> Result, GitAiError> { + // `git notes list` uses --ref to specify which notes ref. + // The --ref option prepends "refs/notes/" automatically, so for full refs + // like "refs/notes/ai-remote/origin" we need to strip the prefix. + let ref_arg = notes_ref.strip_prefix("refs/notes/").unwrap_or(notes_ref); + + let mut args = repo.global_args_for_exec(); + args.extend_from_slice(&[ + "notes".to_string(), + format!("--ref={}", ref_arg), + "list".to_string(), + ]); + + let output = exec_git(&args)?; + let stdout = String::from_utf8(output.stdout) + .map_err(|_| GitAiError::Generic("Failed to parse notes list output".to_string()))?; + + Ok(stdout + .lines() + .filter_map(|line| { + let parts: Vec<&str> = line.split_whitespace().collect(); + if parts.len() == 2 { + Some((parts[0].to_string(), parts[1].to_string())) + } else { + None + } + }) + .collect()) +} + +/// Parse a revision to its SHA +fn rev_parse(repo: &Repository, rev: &str) -> Result { + let mut args = repo.global_args_for_exec(); + args.extend_from_slice(&["rev-parse".to_string(), rev.to_string()]); + let output = exec_git(&args)?; + String::from_utf8(output.stdout) + .map_err(|_| GitAiError::Generic("Failed to parse rev-parse output".to_string())) + .map(|s| s.trim().to_string()) +} + /// Copy a ref to another location (used for initial setup of local notes from tracking ref) pub fn copy_ref(repo: &Repository, source_ref: &str, dest_ref: &str) -> Result<(), GitAiError> { let mut args = repo.global_args_for_exec(); diff --git a/src/git/sync_authorship.rs b/src/git/sync_authorship.rs index 5794615a3..c8258ee27 100644 --- a/src/git/sync_authorship.rs +++ b/src/git/sync_authorship.rs @@ -1,5 +1,6 @@ use crate::git::refs::{ - AI_AUTHORSHIP_PUSH_REFSPEC, copy_ref, merge_notes_from_ref, ref_exists, tracking_ref_for_remote, + AI_AUTHORSHIP_PUSH_REFSPEC, copy_ref, fallback_merge_notes_ours, merge_notes_from_ref, + ref_exists, tracking_ref_for_remote, }; use crate::{ error::GitAiError, @@ -133,7 +134,10 @@ pub fn fetch_authorship_notes( )); if let Err(e) = merge_notes_from_ref(repository, &tracking_ref) { debug_log(&format!("notes merge failed: {}", e)); - // Don't fail on merge errors, just log and continue + // Fallback: manually merge notes when git notes merge crashes + if let Err(e2) = fallback_merge_notes_ours(repository, &tracking_ref) { + debug_log(&format!("fallback merge also failed: {}", e2)); + } } } else { // Only tracking ref exists - copy it to local @@ -168,67 +172,110 @@ fn is_missing_remote_notes_ref_error(error: &GitAiError) -> bool { || stderr_lower.contains("remote ref does not exist") || stderr_lower.contains("not our ref")) } +/// Maximum number of fetch-merge-push attempts before giving up. +/// On busy monorepos, concurrent pushers can cause non-fast-forward rejections +/// even after a successful merge, so we retry the full cycle. +const PUSH_NOTES_MAX_ATTEMPTS: usize = 3; + // for use with post-push hook pub fn push_authorship_notes(repository: &Repository, remote_name: &str) -> Result<(), GitAiError> { - // STEP 1: Fetch remote notes into tracking ref and merge before pushing - // This ensures we don't lose notes from other branches/clones + let mut last_error = None; + + for attempt in 0..PUSH_NOTES_MAX_ATTEMPTS { + if attempt > 0 { + debug_log(&format!( + "retrying notes push (attempt {}/{})", + attempt + 1, + PUSH_NOTES_MAX_ATTEMPTS + )); + } + + fetch_and_merge_tracking_notes(repository, remote_name); + + // Push notes without force (requires fast-forward) + let push_args = build_authorship_push_args(repository.global_args_for_exec(), remote_name); + + debug_log(&format!( + "pushing authorship refs (no force): {:?}", + &push_args + )); + + match exec_git(&push_args) { + Ok(_) => return Ok(()), + Err(e) => { + debug_log(&format!("authorship push failed: {}", e)); + if is_non_fast_forward_error(&e) && attempt + 1 < PUSH_NOTES_MAX_ATTEMPTS { + // Another pusher updated remote notes between our merge and push. + // Retry the full fetch-merge-push cycle. + last_error = Some(e); + continue; + } + return Err(e); + } + } + } + + Err(last_error + .unwrap_or_else(|| GitAiError::Generic("notes push exhausted retries".to_string()))) +} + +/// Fetch remote notes into a tracking ref and merge into local refs/notes/ai. +fn fetch_and_merge_tracking_notes(repository: &Repository, remote_name: &str) { let tracking_ref = tracking_ref_for_remote(remote_name); let fetch_refspec = format!("+refs/notes/ai:{}", tracking_ref); - let fetch_before_push = build_authorship_fetch_args( + let fetch_args = build_authorship_fetch_args( repository.global_args_for_exec(), remote_name, &fetch_refspec, ); - debug_log(&format!( - "pre-push authorship fetch: {:?}", - &fetch_before_push - )); + debug_log(&format!("pre-push authorship fetch: {:?}", &fetch_args)); // Fetch is best-effort; if it fails (e.g., no remote notes yet), continue - if exec_git(&fetch_before_push).is_ok() { - // Merge fetched notes into local refs/notes/ai - let local_notes_ref = "refs/notes/ai"; + if exec_git(&fetch_args).is_err() { + return; + } - if ref_exists(repository, &tracking_ref) { - if ref_exists(repository, local_notes_ref) { - // Both exist - merge them - debug_log(&format!( - "pre-push: merging {} into {}", - tracking_ref, local_notes_ref - )); - if let Err(e) = merge_notes_from_ref(repository, &tracking_ref) { - debug_log(&format!("pre-push notes merge failed: {}", e)); - } - } else { - // Only tracking ref exists - copy it to local - debug_log(&format!( - "pre-push: initializing {} from {}", - local_notes_ref, tracking_ref - )); - if let Err(e) = copy_ref(repository, &tracking_ref, local_notes_ref) { - debug_log(&format!("pre-push notes copy failed: {}", e)); - } - } - } + let local_notes_ref = "refs/notes/ai"; + + if !ref_exists(repository, &tracking_ref) { + return; } - // STEP 2: Push notes without force (requires fast-forward) - let push_authorship = - build_authorship_push_args(repository.global_args_for_exec(), remote_name); + if !ref_exists(repository, local_notes_ref) { + // Only tracking ref exists - copy it to local + debug_log(&format!( + "pre-push: initializing {} from {}", + local_notes_ref, tracking_ref + )); + if let Err(e) = copy_ref(repository, &tracking_ref, local_notes_ref) { + debug_log(&format!("pre-push notes copy failed: {}", e)); + } + return; + } + // Both exist - merge them debug_log(&format!( - "pushing authorship refs (no force): {:?}", - &push_authorship + "pre-push: merging {} into {}", + tracking_ref, local_notes_ref )); - if let Err(e) = exec_git(&push_authorship) { - // Best-effort; don't fail user operation due to authorship sync issues - debug_log(&format!("authorship push skipped due to error: {}", e)); - return Err(e); + if let Err(e) = merge_notes_from_ref(repository, &tracking_ref) { + debug_log(&format!("pre-push notes merge failed: {}", e)); + // Fallback: manually merge notes when git notes merge crashes + // (e.g., due to corrupted/mixed-fanout notes trees, or git bugs + // with fanout-level mismatches on older git versions like macOS) + if let Err(e2) = fallback_merge_notes_ours(repository, &tracking_ref) { + debug_log(&format!("pre-push fallback merge also failed: {}", e2)); + } } +} - Ok(()) +fn is_non_fast_forward_error(error: &GitAiError) -> bool { + let GitAiError::GitCliError { stderr, .. } = error else { + return false; + }; + stderr.contains("non-fast-forward") } fn extract_remote_from_fetch_args(args: &[String]) -> Option { diff --git a/tests/integration/internal_machine_commands.rs b/tests/integration/internal_machine_commands.rs index 9d68591ca..91b621371 100644 --- a/tests/integration/internal_machine_commands.rs +++ b/tests/integration/internal_machine_commands.rs @@ -1,6 +1,8 @@ -use crate::repos::test_repo::TestRepo; +use crate::repos::test_repo::{TestRepo, real_git_executable}; use serde_json::json; use std::fs; +use std::io::Write; +use std::process::{Command, Stdio}; #[test] fn test_effective_ignore_patterns_internal_command_json() { @@ -147,3 +149,262 @@ fn test_fetch_and_push_authorship_notes_internal_commands_json() { serde_json::from_str(fetch_after.trim()).expect("fetch output should be JSON"); assert_eq!(fetch_after_json["notes_existence"], "found"); } + +/// Helper to run a raw git command with stdin piped, returning trimmed stdout. +fn git_plumbing(repo_path: &std::path::Path, args: &[&str], stdin_data: Option<&[u8]>) -> String { + let git = real_git_executable(); + let mut cmd = Command::new(git); + cmd.arg("-C") + .arg(repo_path) + .arg("-c") + .arg("core.hooksPath=/dev/null") + .arg("-c") + .arg("user.name=Test") + .arg("-c") + .arg("user.email=test@test.com") + .args(args); + if stdin_data.is_some() { + cmd.stdin(Stdio::piped()); + } + cmd.stdout(Stdio::piped()).stderr(Stdio::piped()); + + let mut child = cmd.spawn().expect("failed to spawn git plumbing command"); + + if let Some(data) = stdin_data { + child + .stdin + .take() + .unwrap() + .write_all(data) + .expect("failed to write stdin"); + } + + let output = child.wait_with_output().expect("failed to wait for git"); + assert!( + output.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr) + ); + String::from_utf8(output.stdout) + .expect("non-utf8 git output") + .trim() + .to_string() +} + +/// Reproduces a bug where `git notes merge -s ours` crashes with: +/// Assertion failed: (is_null_oid(&mp->remote)), function diff_tree_remote, +/// file notes-merge.c, line 170. +/// +/// This happens when the remote notes tree has mixed fanout — both a flat blob +/// entry (e.g. `aabbccdd…`) AND a subtree entry (e.g. `aa/bbccdd…`) for the +/// same annotated object. The fallback merge should handle this gracefully and +/// the push should succeed. +#[test] +fn test_push_authorship_notes_survives_corrupted_remote_notes_tree() { + let (mirror, upstream) = TestRepo::new_with_remote(); + + // 1. Create a commit on mirror and push it + fs::write(mirror.path().join("test.txt"), "hello\n").expect("write file"); + let commit = mirror + .stage_all_and_commit("initial commit") + .expect("commit should succeed"); + mirror + .git(&["push", "origin", "main"]) + .expect("push should succeed"); + let commit_sha = commit.commit_sha; + + // 2. Create a corrupted notes tree on upstream with NO common merge base + // relative to the mirror's local notes ref. This is the key condition that + // triggers the assertion in git's notes-merge.c: when there's no merge base, + // git uses an empty tree as the base, and the diff against the corrupted remote + // tree encounters the same annotated object twice (once flat, once fanout). + let prefix = &commit_sha[..2]; + let rest = &commit_sha[2..]; + + // Create a note blob on upstream (independent of mirror's notes) + let note_blob = git_plumbing( + upstream.path(), + &["hash-object", "-w", "--stdin"], + Some(br#"{"author":"remote"}"#), + ); + + // Build inner tree (fanout: prefix/rest -> blob) + let inner_tree_input = format!("100644 blob {}\t{}\n", note_blob, rest); + let inner_tree = git_plumbing( + upstream.path(), + &["mktree"], + Some(inner_tree_input.as_bytes()), + ); + + // Build mixed tree: flat entry + subtree entry for same commit + let mixed_tree_input = format!( + "100644 blob {}\t{}\n040000 tree {}\t{}\n", + note_blob, commit_sha, inner_tree, prefix + ); + let mixed_tree = git_plumbing( + upstream.path(), + &["mktree"], + Some(mixed_tree_input.as_bytes()), + ); + + // Create a root commit (NO parent) — this ensures no common merge base + // with the mirror's notes ref, which is what triggers the assertion. + let corrupted_commit = git_plumbing( + upstream.path(), + &[ + "commit-tree", + &mixed_tree, + "-m", + "corrupted notes tree (orphan)", + ], + None, + ); + git_plumbing( + upstream.path(), + &["update-ref", "refs/notes/ai", &corrupted_commit], + None, + ); + + // 4. Add a new local note on mirror (so local and remote have diverged). + // The git-ai commit hook automatically creates notes, so just making + // a new commit is sufficient. + fs::write(mirror.path().join("test2.txt"), "world\n").expect("write file2"); + let commit2 = mirror + .stage_all_and_commit("second commit") + .expect("second commit should succeed"); + + // 5. Push authorship notes — this triggers fetch + merge + push. + // Without the fallback fix, the merge crashes and the push fails + // with "non-fast-forward". + let request = json!({"remote_name": "origin"}).to_string(); + let push_output = mirror + .git_ai(&["push-authorship-notes", "--json", &request]) + .expect("push-authorship-notes should succeed despite corrupted remote tree"); + let push_json: serde_json::Value = + serde_json::from_str(push_output.trim()).expect("push output should be JSON"); + assert_eq!( + push_json["ok"], + true, + "push should succeed via fallback merge, got: {}", + push_output.trim() + ); + + // 6. Verify both notes are present on upstream after push + let notes_list = git_plumbing(upstream.path(), &["notes", "--ref=ai", "list"], None); + assert!( + notes_list.contains(&commit_sha), + "upstream should have note for first commit" + ); + assert!( + notes_list.contains(&commit2.commit_sha), + "upstream should have note for second commit" + ); +} + +/// Simulates the race condition on busy monorepos where another developer +/// pushes notes between our fetch-merge and push steps, causing a +/// non-fast-forward rejection. The retry loop should re-fetch, re-merge, +/// and push successfully. +#[test] +fn test_push_authorship_notes_retries_on_concurrent_push() { + let (mirror, upstream) = TestRepo::new_with_remote(); + + // 1. Create initial commit and push + fs::write(mirror.path().join("a.txt"), "a\n").expect("write a"); + let commit1 = mirror + .stage_all_and_commit("first commit") + .expect("commit1"); + mirror.git(&["push", "origin", "main"]).expect("push main"); + + // 2. Push mirror's initial notes to upstream + mirror + .git_og(&["push", "origin", "refs/notes/ai:refs/notes/ai"]) + .expect("push initial notes"); + + // 3. Create a second clone that simulates the concurrent pusher + let clone2_path = std::env::temp_dir().join(format!("concurrent-clone-{}", std::process::id())); + let _ = fs::remove_dir_all(&clone2_path); + git_plumbing( + mirror.path(), + &[ + "clone", + upstream.path().to_str().unwrap(), + clone2_path.to_str().unwrap(), + ], + None, + ); + // Configure clone2 and fetch notes + git_plumbing( + &clone2_path, + &["config", "user.email", "other@test.com"], + None, + ); + git_plumbing(&clone2_path, &["config", "user.name", "Other"], None); + git_plumbing( + &clone2_path, + &["fetch", "origin", "+refs/notes/ai:refs/notes/ai"], + None, + ); + + // 4. Other clone makes a commit with a note and pushes notes to upstream. + // This advances remote refs/notes/ai beyond what mirror has fetched. + fs::write(clone2_path.join("b.txt"), "b\n").expect("write b"); + git_plumbing(&clone2_path, &["add", "b.txt"], None); + git_plumbing(&clone2_path, &["commit", "-m", "other commit"], None); + let other_sha = git_plumbing(&clone2_path, &["rev-parse", "HEAD"], None); + git_plumbing( + &clone2_path, + &[ + "notes", + "--ref=ai", + "add", + "-m", + r#"{"author":"other"}"#, + &other_sha, + ], + None, + ); + git_plumbing( + &clone2_path, + &["push", "origin", "refs/notes/ai:refs/notes/ai"], + None, + ); + + // 5. Mirror makes another commit (notes auto-created by hook). + // Mirror's local refs/notes/ai is now behind remote. + fs::write(mirror.path().join("c.txt"), "c\n").expect("write c"); + let _commit3 = mirror + .stage_all_and_commit("mirror commit") + .expect("commit3"); + + // 6. Push authorship notes. The retry loop should: + // - Attempt 1: fetch, merge, push → fails (non-fast-forward if + // remote was updated between merge and push, or succeeds on first try) + // - Attempt 2+: re-fetch, re-merge, push → succeeds + // In this test, the remote is already ahead, so the first attempt's + // fetch+merge will incorporate the other clone's notes, and push succeeds. + let request = json!({"remote_name": "origin"}).to_string(); + let push_output = mirror + .git_ai(&["push-authorship-notes", "--json", &request]) + .expect("push-authorship-notes should succeed after retry"); + let push_json: serde_json::Value = + serde_json::from_str(push_output.trim()).expect("push output should be JSON"); + assert_eq!( + push_json["ok"], + true, + "push should eventually succeed, got: {}", + push_output.trim() + ); + + // 7. Verify all notes are present on upstream + let notes_list = git_plumbing(upstream.path(), &["notes", "--ref=ai", "list"], None); + assert!( + notes_list.contains(&commit1.commit_sha), + "upstream should have note for mirror's first commit" + ); + assert!( + notes_list.contains(&other_sha), + "upstream should have note from concurrent pusher" + ); +}