diff --git a/src/run.rs b/src/run.rs index 702b540c5..4fa79ef03 100644 --- a/src/run.rs +++ b/src/run.rs @@ -438,7 +438,9 @@ impl RepairLoop<'_> { /// Fetch the Issue branch from origin and merge in any Foreign commits on /// it (never rebase), counting that as a round and handing a conflict to /// a conflict Repair. Then a review Repair reviews them from the head the - /// Run last knew as its own, the local head before the merge. + /// Run last knew as its own, the local head before the merge. Goes round + /// again until origin has nothing new, since more may land during either + /// Repair, and the push after them would be rejected. fn take_in_foreign_commits( &mut self, run_session: &mut impl FnMut(&str, &str) -> Result<()>, @@ -446,35 +448,37 @@ impl RepairLoop<'_> { let (issue, worktree, pr_url) = (self.issue, self.worktree, self.pr_url); let branch = worktree.branch(); let upstream = worktree.upstream(); - let foreign = worktree.new_commits_on_origin()?; - if foreign.is_empty() { - return Ok(()); - } - let own_head = worktree.head()?; - self.budgets.count_upstream_move( - &upstream, - format_args!("{upstream} has new commits; merging them in"), - )?; - for sha in &foreign { - progress::step(format_args!("merging new commit {sha} from {upstream}")); - } - if let Merge::Conflicted(pending) = worktree.merge_new_commits()? { + loop { + let foreign = worktree.new_commits_on_origin()?; + if foreign.is_empty() { + return Ok(()); + } + let own_head = worktree.head()?; + self.budgets.count_upstream_move( + &upstream, + format_args!("{upstream} has new commits; merging them in"), + )?; + for sha in &foreign { + progress::step(format_args!("merging new commit {sha} from {upstream}")); + } + if let Merge::Conflicted(pending) = worktree.merge_new_commits()? { + let kind = self + .budgets + .next_repair(&format!("conflict with new commits on {upstream}"))?; + run_session( + &kind, + &prompt::conflict_repair(issue, branch, branch, pr_url), + )?; + worktree.ensure_merged(&pending)?; + } let kind = self .budgets - .next_repair(&format!("conflict with new commits on {upstream}"))?; + .next_repair(&format!("new commits on {upstream} to review"))?; run_session( &kind, - &prompt::conflict_repair(issue, branch, branch, pr_url), + &prompt::review_repair(issue, branch, pr_url, &own_head), )?; - worktree.ensure_merged(&pending)?; } - let kind = self - .budgets - .next_repair(&format!("new commits on {upstream} to review"))?; - run_session( - &kind, - &prompt::review_repair(issue, branch, pr_url, &own_head), - ) } /// Go round again after a merge of `watched` failed, counting a Base diff --git a/tests/merge_run.rs b/tests/merge_run.rs index 6817f3da4..e2eda02ca 100644 --- a/tests/merge_run.rs +++ b/tests/merge_run.rs @@ -558,6 +558,61 @@ fn a_foreign_commit_that_conflicts_with_local_work_gets_a_conflict_repair_then_a ); } +#[test] +fn foreign_commits_pushed_during_the_conflict_repair_are_merged_in_and_reviewed_too() { + let scenario = Scenario::new(); + scenario.agent_does_in_session(1, &format!("{AGENT_OPENS_PR}{}", checks_on_head(RED))); + scenario.agent_does_in_session( + 2, + &format!( + "{}echo fixed > feature.txt\ngit commit -q -am \"Fix CI 1\"\n", + someone_pushes_to_issue_7("feature.txt", "theirs", "Their feature") + ), + ); + // While the conflict Repair works, someone pushes to issue-7 again. + scenario.agent_does_in_session( + 3, + &format!( + "echo both > feature.txt\ngit add feature.txt\ngit commit -q --no-edit\n{}", + someone_pushes_to_issue_7("late.txt", "late", "Late commit") + ), + ); + + let result = scenario.run(&["merge", &scenario.issue_url(7)]); + + assert_eq!(result.code, Some(0), "stderr: {}", result.stderr); + let late = sha_of(&scenario, "main", "Late commit"); + assert!( + result.stderr.contains(&format!( + "thirdshift: merging new commit {late} from origin/issue-7" + )), + "stderr: {}", + result.stderr + ); + let calls = scenario.claude_calls(); + assert_eq!( + calls.len(), + 5, + "implement, CI fix, conflict, and a review for each round of Foreign commits" + ); + let fix = sha_of(&scenario, "main", "Fix CI 1"); + assert_review_repair_from(&calls[3]["prompt"], &fix); + // The second round reviews from the conflict Repair's merge commit. + let parents = format!("{fix} {}", sha_of(&scenario, "main", "Their feature")); + let resolution = scenario + .origin_git(&["log", "--format=%H %P", "main"]) + .lines() + .find_map(|line| line.strip_suffix(&format!(" {parents}")).map(String::from)) + .expect("no merge of Their feature into Fix CI 1"); + assert_review_repair_from(&calls[4]["prompt"], &resolution); + assert_issue_7_merged_into(&scenario, "main"); + assert_eq!(scenario.origin_file("main", "late.txt").unwrap(), "late\n"); + assert_eq!( + scenario.origin_file("main", "feature.txt").unwrap(), + "both\n" + ); +} + #[test] fn a_review_repair_whose_background_work_was_killed_gets_a_resume() { let scenario = Scenario::new();