From 2eaebf3d80eeb54176cb65fc7eace044cd9ba818 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 2 Oct 2026 13:42:46 -0400 Subject: [PATCH 1/2] Retry a fetch race as git 2.52 words it, and lead a git failure with git's error line A git command that fails with 'incorrect old value provided' is run again like the other cases of a ref another git moved meanwhile. A failed git command's message now has git's first 'error:' or 'fatal:' line right after 'git failed: ', so the first line of a child Run's cause, which is all a Ticket's line shows, says what went wrong instead of a banner. Co-Authored-By: Claude Opus 5.5 --- src/git.rs | 104 +++++++++++++++++++++++++++++++++++----------- src/run_ending.rs | 22 ++++++++++ src/spec_run.rs | 34 +++++++++++++++ 3 files changed, 136 insertions(+), 24 deletions(-) diff --git a/src/git.rs b/src/git.rs index 40cf9f530..436564f1c 100644 --- a/src/git.rs +++ b/src/git.rs @@ -23,9 +23,10 @@ impl Git { } /// Run `git ` and return its trimmed stdout. If it exits non-zero, - /// fail with the last lines of its stderr and then of its stdout, where a - /// hook's explanation can end up. While it fails on a lock file another - /// git holds, as when a Spec run's Tickets fetch or create worktrees from + /// fail with git's own `error:` or `fatal:` line, if it wrote one, then + /// the last lines of its stderr and of its stdout, where a hook's + /// explanation can end up. While it fails on a lock file another git + /// holds, as when a Spec run's Tickets fetch or create worktrees from /// one Launch directory at once, it is run again, for up to /// [`LOCK_WAIT`]. pub fn run(&self, args: &[&str]) -> Result { @@ -36,10 +37,7 @@ impl Git { output = self.output(args)?; } if !output.status.success() { - let tail: Vec = [&output.stderr, &output.stdout] - .into_iter() - .flat_map(|stream| last_lines(stream)) - .collect(); + let tail = [cause_first(&output.stderr), last_lines(&output.stdout)].concat(); let mut message = format!("git {} failed", args.join(" ")); if !tail.is_empty() { message = format!("{message}: {}", tail.join("\n")); @@ -92,12 +90,14 @@ const LOCK_WAIT: Duration = Duration::from_secs(10); /// Whether git's `stderr` says it failed because a lock file, such as /// `config.lock` or a ref's, already exists, or because another git moved a -/// ref between reading and updating it, as two fetches of one branch can. +/// ref between reading and updating it, as two fetches of one branch can, +/// in older git's wording or git 2.52's. fn held_lock(stderr: &[u8]) -> bool { let stderr = String::from_utf8_lossy(stderr); stderr.contains(".lock': File exists") || stderr.contains("could not lock config file") || (stderr.contains("cannot lock ref") && stderr.contains("but expected")) + || stderr.contains("incorrect old value provided") } /// The most lines of each stream a failure reports. @@ -105,15 +105,34 @@ const MAX_LINES: usize = 10; /// The last `MAX_LINES` non-empty lines of `stream`, trimmed. fn last_lines(stream: &[u8]) -> Vec { - let text = String::from_utf8_lossy(stream); - let lines: Vec<&str> = text + let mut lines = lines(stream); + lines.drain(..lines.len().saturating_sub(MAX_LINES)); + lines +} + +/// The lines of a failed git's `stderr` to report, `MAX_LINES` at most: the +/// first line starting `error:` or `fatal:`, which says what went wrong, +/// then the last of the others. With no such line, its last lines. So a +/// reader of the first line alone, as of a child Run's cause, reads git's +/// error and not a banner such as a fetch's `From `. +fn cause_first(stderr: &[u8]) -> Vec { + let mut lines = lines(stderr); + let cause = lines + .iter() + .position(|line| line.starts_with("error:") || line.starts_with("fatal:")) + .map(|at| lines.remove(at)); + let others = MAX_LINES - cause.iter().count(); + lines.drain(..lines.len().saturating_sub(others)); + cause.into_iter().chain(lines).collect() +} + +/// The non-empty lines of `stream`, trimmed. +fn lines(stream: &[u8]) -> Vec { + String::from_utf8_lossy(stream) .lines() .map(str::trim) .filter(|line| !line.is_empty()) - .collect(); - lines[lines.len().saturating_sub(MAX_LINES)..] - .iter() - .map(|line| line.to_string()) + .map(String::from) .collect() } @@ -168,6 +187,39 @@ mod tests { assert!(error.contains("failed to push some refs"), "{error}"); } + #[test] + fn a_failure_starts_with_gits_own_error_line() { + let (_temp, git) = repo_with_origin(); + let alias = "alias.fetch-main=!printf '%s\\n' \ + 'From https://github.com/acme/widgets' \ + ' * branch main -> FETCH_HEAD' \ + ' 93d8147..8cf48f0 main -> origin/main' \ + 'error: some local refs could not be updated' \ + 'error: try again' >&2; \ + echo fetched; exit 1"; + + let error = git + .run(&["-c", alias, "fetch-main"]) + .unwrap_err() + .to_string(); + + let lines: Vec<&str> = error.lines().collect(); + assert!( + lines[0].ends_with(" fetch-main failed: error: some local refs could not be updated"), + "{error}" + ); + assert_eq!( + lines[1..], + [ + "From https://github.com/acme/widgets", + "* branch main -> FETCH_HEAD", + "93d8147..8cf48f0 main -> origin/main", + "error: try again", + "fetched", + ] + ); + } + #[test] fn a_failure_includes_stdout() { let (_temp, git) = repo_with_origin(); @@ -220,16 +272,20 @@ mod tests { #[test] fn a_ref_another_git_moved_meanwhile_is_tried_again() { - let (temp, git) = repo_with_origin(); - let moved = temp.path().join("moved"); - let alias = format!( - "alias.race=!test -f {0} && exit 0; touch {0}; \ - echo \"error: cannot lock ref 'refs/remotes/origin/main': is at 1 but expected 2\" >&2; \ - exit 1", - moved.display() - ); - - git.run(&["-c", &alias, "race"]).unwrap(); + // The race as older git words it, then as git 2.52 does. + for error in [ + "error: cannot lock ref 'refs/remotes/origin/main': is at 1 but expected 2", + "error: fetching ref refs/remotes/origin/main failed: incorrect old value provided", + ] { + let (temp, git) = repo_with_origin(); + let moved = temp.path().join("moved"); + let alias = format!( + "alias.race=!test -f {0} && exit 0; touch {0}; echo \"{error}\" >&2; exit 1", + moved.display() + ); + + git.run(&["-c", &alias, "race"]).unwrap(); + } } #[test] diff --git a/src/run_ending.rs b/src/run_ending.rs index bb73832ba..fdb112abc 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -342,6 +342,28 @@ mod tests { } } + #[test] + fn a_failed_git_command_reads_back_as_gits_own_error() { + let cause = "git fetch origin issue-1346 failed: \ + error: fetching ref refs/remotes/origin/issue-1346 failed: \ + incorrect old value provided\n\ + From https://github.com/acme/widgets\n\ + * branch issue-1346 -> FETCH_HEAD\n\ + 93d814703..8cf48f061 issue-1346 -> origin/issue-1346"; + assert_eq!( + read_back(&failed(cause, None, None)), + ChildEnding { + outcome: failure( + "git fetch origin issue-1346 failed: \ + error: fetching ref refs/remotes/origin/issue-1346 failed: \ + incorrect old value provided", + None + ), + base_fix: None, + } + ); + } + #[test] fn the_advice_a_failed_run_gives_is_neither_its_cause_nor_its_session_log() { let advice = vec![ diff --git a/src/spec_run.rs b/src/spec_run.rs index 149c3fb28..49a717d79 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -676,6 +676,40 @@ mod tests { } } + #[test] + fn a_failed_tickets_line_says_its_cause_and_names_its_session_log() { + let tickets = [ticket(1348, true, &[], &[]), ticket(1349, true, &[], &[])]; + let cause = "git fetch origin issue-1346 failed: \ + error: fetching ref refs/remotes/origin/issue-1346 failed: \ + incorrect old value provided"; + let outcomes = BTreeMap::from([ + ( + 1348, + TicketOutcome::Failed { + cause: cause.to_string(), + log: None, + }, + ), + ( + 1349, + TicketOutcome::Failed { + cause: "claude exited 1".to_string(), + log: Some("/logs/1349.jsonl".to_string()), + }, + ), + ]); + + assert_eq!( + summarize(&tickets, &outcomes), + [ + "#1348 failed: git fetch origin issue-1346 failed: \ + error: fetching ref refs/remotes/origin/issue-1346 failed: \ + incorrect old value provided", + "#1349 failed: claude exited 1 (session log: /logs/1349.jsonl)", + ] + ); + } + #[test] fn the_checklist_ticks_done_tickets_and_says_where_each_other_stands() { let mut unready = ticket(26, true, &[], &[]); From 022bdee7174791516325a9f6f8732f6d59e7f802 Mon Sep 17 00:00:00 2001 From: Jacob Stephens Date: Fri, 2 Oct 2026 13:50:56 -0400 Subject: [PATCH 2/2] Address review: the retry's docs name the ref race, the error-first helper named for what it does, a cause of several lines tested with a git failure's own shape Co-Authored-By: Claude Opus 5.5 --- src/git.rs | 46 ++++++++++++++++++++++++++++++++++------------ src/run_ending.rs | 35 +++++++++-------------------------- src/spec_run.rs | 40 +++++++++++++--------------------------- 3 files changed, 56 insertions(+), 65 deletions(-) diff --git a/src/git.rs b/src/git.rs index 436564f1c..e80c116f7 100644 --- a/src/git.rs +++ b/src/git.rs @@ -26,9 +26,9 @@ impl Git { /// fail with git's own `error:` or `fatal:` line, if it wrote one, then /// the last lines of its stderr and of its stdout, where a hook's /// explanation can end up. While it fails on a lock file another git - /// holds, as when a Spec run's Tickets fetch or create worktrees from - /// one Launch directory at once, it is run again, for up to - /// [`LOCK_WAIT`]. + /// holds, or on a ref another git moved meanwhile, as when a Spec run's + /// Tickets fetch or create worktrees from one Launch directory at once, + /// it is run again, for up to [`LOCK_WAIT`]. pub fn run(&self, args: &[&str]) -> Result { let deadline = Instant::now() + LOCK_WAIT; let mut output = self.output(args)?; @@ -37,7 +37,7 @@ impl Git { output = self.output(args)?; } if !output.status.success() { - let tail = [cause_first(&output.stderr), last_lines(&output.stdout)].concat(); + let tail = [error_first(&output.stderr), last_lines(&output.stdout)].concat(); let mut message = format!("git {} failed", args.join(" ")); if !tail.is_empty() { message = format!("{message}: {}", tail.join("\n")); @@ -85,7 +85,8 @@ impl Git { } } -/// How long [`Git::run`] keeps trying a command that fails on a lock file. +/// How long [`Git::run`] keeps trying a command that fails on a lock file +/// or on a ref another git moved meanwhile. const LOCK_WAIT: Duration = Duration::from_secs(10); /// Whether git's `stderr` says it failed because a lock file, such as @@ -105,7 +106,7 @@ const MAX_LINES: usize = 10; /// The last `MAX_LINES` non-empty lines of `stream`, trimmed. fn last_lines(stream: &[u8]) -> Vec { - let mut lines = lines(stream); + let mut lines = non_empty_lines(stream); lines.drain(..lines.len().saturating_sub(MAX_LINES)); lines } @@ -115,19 +116,19 @@ fn last_lines(stream: &[u8]) -> Vec { /// then the last of the others. With no such line, its last lines. So a /// reader of the first line alone, as of a child Run's cause, reads git's /// error and not a banner such as a fetch's `From `. -fn cause_first(stderr: &[u8]) -> Vec { - let mut lines = lines(stderr); - let cause = lines +fn error_first(stderr: &[u8]) -> Vec { + let mut lines = non_empty_lines(stderr); + let error = lines .iter() .position(|line| line.starts_with("error:") || line.starts_with("fatal:")) .map(|at| lines.remove(at)); - let others = MAX_LINES - cause.iter().count(); + let others = MAX_LINES - usize::from(error.is_some()); lines.drain(..lines.len().saturating_sub(others)); - cause.into_iter().chain(lines).collect() + error.into_iter().chain(lines).collect() } /// The non-empty lines of `stream`, trimmed. -fn lines(stream: &[u8]) -> Vec { +fn non_empty_lines(stream: &[u8]) -> Vec { String::from_utf8_lossy(stream) .lines() .map(str::trim) @@ -245,6 +246,27 @@ mod tests { assert!(!error.contains("\n50\n"), "{error}"); } + #[test] + fn gits_own_error_line_is_kept_however_many_lines_follow_it() { + let (_temp, git) = repo_with_origin(); + + let error = git + .run(&[ + "-c", + "alias.fail=!{ echo 'fatal: no'; seq 1 100; } >&2; exit 1", + "fail", + ]) + .unwrap_err() + .to_string(); + + let lines: Vec<&str> = error.lines().collect(); + assert!(lines[0].ends_with(" fail failed: fatal: no"), "{error}"); + assert_eq!( + lines[1..], + ["92", "93", "94", "95", "96", "97", "98", "99", "100"] + ); + } + #[test] fn a_lock_another_git_holds_is_waited_for() { let commands: [(&str, &[&str]); 2] = [ diff --git a/src/run_ending.rs b/src/run_ending.rs index fdb112abc..b3c3bb766 100644 --- a/src/run_ending.rs +++ b/src/run_ending.rs @@ -328,42 +328,25 @@ mod tests { #[test] fn a_cause_of_several_lines_reads_back_as_its_first_line() { - let cause = "git push failed: exit status: 1\n\ - error: failed to push some refs\n\ - hint: fetch first"; + let cause = "git fetch origin issue-21 failed: \ + error: fetching ref refs/remotes/origin/issue-21 failed\n\ + From https://github.com/acme/widgets\n\ + * branch issue-21 -> FETCH_HEAD"; for log in [Some(LOG), None] { assert_eq!( read_back(&failed(cause, log, None)), ChildEnding { - outcome: failure("git push failed: exit status: 1", log), + outcome: failure( + "git fetch origin issue-21 failed: \ + error: fetching ref refs/remotes/origin/issue-21 failed", + log + ), base_fix: None, } ); } } - #[test] - fn a_failed_git_command_reads_back_as_gits_own_error() { - let cause = "git fetch origin issue-1346 failed: \ - error: fetching ref refs/remotes/origin/issue-1346 failed: \ - incorrect old value provided\n\ - From https://github.com/acme/widgets\n\ - * branch issue-1346 -> FETCH_HEAD\n\ - 93d814703..8cf48f061 issue-1346 -> origin/issue-1346"; - assert_eq!( - read_back(&failed(cause, None, None)), - ChildEnding { - outcome: failure( - "git fetch origin issue-1346 failed: \ - error: fetching ref refs/remotes/origin/issue-1346 failed: \ - incorrect old value provided", - None - ), - base_fix: None, - } - ); - } - #[test] fn the_advice_a_failed_run_gives_is_neither_its_cause_nor_its_session_log() { let advice = vec![ diff --git a/src/spec_run.rs b/src/spec_run.rs index 49a717d79..f6c61a366 100644 --- a/src/spec_run.rs +++ b/src/spec_run.rs @@ -677,36 +677,22 @@ mod tests { } #[test] - fn a_failed_tickets_line_says_its_cause_and_names_its_session_log() { - let tickets = [ticket(1348, true, &[], &[]), ticket(1349, true, &[], &[])]; - let cause = "git fetch origin issue-1346 failed: \ - error: fetching ref refs/remotes/origin/issue-1346 failed: \ - incorrect old value provided"; - let outcomes = BTreeMap::from([ - ( - 1348, - TicketOutcome::Failed { - cause: cause.to_string(), - log: None, - }, - ), - ( - 1349, - TicketOutcome::Failed { - cause: "claude exited 1".to_string(), - log: Some("/logs/1349.jsonl".to_string()), - }, - ), - ]); + fn a_failed_tickets_line_says_its_cause() { + let tickets = [ticket(23, true, &[], &[])]; + let cause = "git fetch origin issue-21 failed: \ + error: fetching ref refs/remotes/origin/issue-21 failed"; + let outcomes = BTreeMap::from([( + 23, + TicketOutcome::Failed { + cause: cause.to_string(), + log: None, + }, + )]); assert_eq!( summarize(&tickets, &outcomes), - [ - "#1348 failed: git fetch origin issue-1346 failed: \ - error: fetching ref refs/remotes/origin/issue-1346 failed: \ - incorrect old value provided", - "#1349 failed: claude exited 1 (session log: /logs/1349.jsonl)", - ] + ["#23 failed: git fetch origin issue-21 failed: \ + error: fetching ref refs/remotes/origin/issue-21 failed"] ); }