a73x

35a8d4fa

Address review: sound walk bound, reopen clause, JSON hint

a73x   2026-08-10 07:30

Commit message
Address review: sound walk bound, reopen clause, JSON hint

Two blockers, both silent state loss.

The walk bound was unsound. `merge_base_many` is not the octopus base --
it computes `merge_base(oids[0], merge(oids[1..]))`, so its answer
depends on argument order and is an ancestor of only some of its inputs.
Verified on a linear chain A-B-C-D-E: `git merge-base A B D` is A but
`git merge-base B A D` is B, and B is not an ancestor of A. Bounding
with B hides A, so a merge commit between them is never walked and the
scan records nothing -- silently, and for a squash, where `looks_merged`
cannot flag it either. Now folds binary `merge_base`, which is sound by
induction; git2 0.19 exposes no octopus equivalent.

The retry re-closed a deliberately reopened issue, every sync, forever,
and silently. The invariant as stated was too strong. It now carries the
spec's reopen clause: a merged patch's `fixes` issue is closed unless it
has been closed for this patch once already and since reopened. The
distinguishing fact is already durable in the DAG -- a half-failure has
no `IssueClose` attributable to this patch, a reopen has one -- so the
guard needs no new state. Since it is only consulted while the issue
reads open, "this patch's close is in the DAG" is by itself proof that
something reopened it afterwards, and no ordering analysis is needed.
The close reason is written and read through one function so the writer
and the marker cannot drift apart.

Also:

- The inline close warned about an unresolvable `fixes` issue on every
  pass, not just the one that recorded the merge. A trailer stays in
  history forever, so while any other open patch keeps that base walked
  this repeated the same unactionable warning every sync -- exactly what
  the quiet retry pass avoids, arriving by another route. The rule is
  now stated once and applied at both call sites.
- The no-write assertion in the reachability test captured its baseline
  after the call it was meant to guard, so it could not fail. Reordered,
  and verified to bite by probing it with a write.
- `--json` reports the hint as a separate `looks_merged` field, skipped
  when false, on both `show_json` and `list_json`. `status` stays the
  recorded fact. Added at the `*_json` layer rather than on `PatchState`,
  which is what the on-disk fold cache stores.

Tests: multi-base bound both directly and end to end (three bases -- for
two, `merge_base_many` degenerates to the correct binary `merge_base`,
so no two-base test can distinguish the implementations); reopen
survives repeated syncs, and via `patch merge`; a different patch's
close still fires, proving the guard is scoped to the patch and not the
issue; two clones recording the same merge converge to one DAG tip.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
README.md
Old New
@@ -62,9 +62,14 @@ $ git-collab patch merge a1b2c3d4
62 ``` 62 ```
63 63
64 Either way, a patch created with `--fixes` closes the issue it fixes at the same 64 Either way, a patch created with `--fixes` closes the issue it fixes at the same
65 moment. A patch whose commits are simply reachable from its base tip is shown as 65 moment. `git-collab issue reopen` then stays reopened: the fix landing and
66 turning out to be wrong is ordinary, and nothing re-closes an issue over a
67 decision you made after it.
68
69 A patch whose commits are simply reachable from its base tip is shown as
66 `merged?` — a hint, and one that can only ever be a hint, so it decides nothing 70 `merged?` — a hint, and one that can only ever be a hint, so it decides nothing
67 and writes nothing. 71 and writes nothing. `--json` reports it as a separate `looks_merged` field,
72 never inside `status`.
68 73
69 ## Install 74 ## Install
70 75
src/merge_scan.rs
Old New
@@ -96,16 +96,69 @@ pub fn record_merge(
96 /// `PatchMerge` could never retry, because on the retry pass the merge is a 96 /// `PatchMerge` could never retry, because on the retry pass the merge is a
97 /// no-op. Being idempotent is what makes it safe to run every time: an already 97 /// no-op. Being idempotent is what makes it safe to run every time: an already
98 /// closed issue is left alone, so repeated syncs append nothing. 98 /// closed issue is left alone, so repeated syncs append nothing.
99 ///
100 /// The invariant needs the reopen clause in [`reopened_after_this_patch_closed`]
101 /// or it is too strong — see there.
99 pub fn close_fixed_issue(repo: &Repository, patch: &PatchState) -> CloseOutcome { 102 pub fn close_fixed_issue(repo: &Repository, patch: &PatchState) -> CloseOutcome {
100 close_fixed_issue_inner(repo, patch, true) 103 close_fixed_issue_inner(repo, patch, true)
101 } 104 }
102 105
103 /// `warn_unresolvable` is false on the retry pass. An issue that does not 106 /// The `IssueClose` reason this feature writes, and the marker it reads back to
104 /// resolve — deleted, or an ambiguous prefix — will not resolve on the next 107 /// recognize its own close. One function so the writer and the reader cannot
105 /// sync either, and the attempt made alongside the merge already said so; going 108 /// drift: if this string changes, a close written by an older version stops
106 /// on to warn about it on every sync forever is noise nobody can act on. A 109 /// being attributable, and the reopen guard below silently stops guarding.
107 /// failure to *append* still warns either way, because that one is worth 110 pub fn merge_close_reason(patch_id: &str) -> String {
108 /// retrying and worth hearing about. 111 format!("merged in patch {:.8}", patch_id)
112 }
113
114 /// Whether this patch's close of `issue_ref` already happened and the issue was
115 /// subsequently reopened.
116 ///
117 /// Called only when the issue is currently open, which is what makes the test
118 /// this cheap: if this patch's `IssueClose` is in the DAG *and* the issue reads
119 /// open, then something reopened it after that close landed. No ordering
120 /// analysis is needed — the status fold already resolved the ordering, and the
121 /// answer it reached is the one to respect.
122 ///
123 /// This is the difference between the two states the retry cannot otherwise
124 /// tell apart:
125 ///
126 /// - A genuine half-failure — the merge landed, the close did not — has **no**
127 /// `IssueClose` attributable to this patch. The close never happened, and
128 /// restoring it is exactly the retry's job.
129 /// - A deliberate `issue reopen` has one, followed by a reopen. Reopening a
130 /// merged patch's issue is ordinary: the fix landed and turned out to be
131 /// wrong. Without this clause the retry re-closes it on every sync forever,
132 /// silently, which makes `issue reopen` unusable for any issue a merged patch
133 /// names — a worse bug than the one the retry fixes.
134 ///
135 /// The retry restores a close that never happened. It must never override a
136 /// decision made after one did.
137 fn reopened_after_this_patch_closed(repo: &Repository, issue_ref: &str, patch_id: &str) -> bool {
138 let marker = merge_close_reason(patch_id);
139 let Ok(events) = dag::walk_events(repo, issue_ref) else {
140 // Unreadable DAG. Treat as "guard applies": declining to close is
141 // recoverable by hand, re-closing a reopened issue every sync is not.
142 return true;
143 };
144 events.iter().any(|(_oid, event)| {
145 matches!(&event.action, Action::IssueClose { reason: Some(r) } if *r == marker)
146 })
147 }
148
149 /// The rule for `warn_unresolvable`: **an unresolvable `fixes` issue is
150 /// reported once, by the operation that recorded the merge, or by any command
151 /// the user invoked by hand.** It is never reported by a pass that merely
152 /// re-observed an already-recorded merge.
153 ///
154 /// An issue that does not resolve — deleted, or an ambiguous prefix — will not
155 /// resolve on the next sync either. Both the retry pass and the trailer scan
156 /// revisit merged patches on every sync (a trailer stays in history forever), so
157 /// warning from either would repeat until someone edits git history, about
158 /// something nobody can act on.
159 ///
160 /// A failure to *append* the close warns unconditionally, because that one is
161 /// transient, is worth retrying, and is worth hearing about every time.
109 fn close_fixed_issue_inner( 162 fn close_fixed_issue_inner(
110 repo: &Repository, 163 repo: &Repository,
111 patch: &PatchState, 164 patch: &PatchState,
@@ -143,7 +196,10 @@ fn close_fixed_issue_inner(
143 if issue.status != IssueStatus::Open { 196 if issue.status != IssueStatus::Open {
144 return CloseOutcome::NothingToDo; 197 return CloseOutcome::NothingToDo;
145 } 198 }
146 let reason = format!("merged in patch {:.8}", patch.id); 199 if reopened_after_this_patch_closed(repo, &issue_ref, &patch.id) {
200 return CloseOutcome::NothingToDo;
201 }
202 let reason = merge_close_reason(&patch.id);
147 if let Err(e) = crate::issue::close(repo, &issue_id, Some(&reason)) { 203 if let Err(e) = crate::issue::close(repo, &issue_id, Some(&reason)) {
148 eprintln!( 204 eprintln!(
149 "warning: patch {:.8}: failed to close fixed issue {:.8}: {} — \ 205 "warning: patch {:.8}: failed to close fixed issue {:.8}: {} — \
@@ -220,9 +276,13 @@ pub fn scan_and_record_merges(
220 /// merge could never retry. 276 /// merge could never retry.
221 /// 277 ///
222 /// So the rule is the invariant, not the emission: a merged patch's `fixes` 278 /// So the rule is the invariant, not the emission: a merged patch's `fixes`
223 /// issue is closed. Cheap, because it only looks at patches that are merged 279 /// issue is closed, *unless it has been closed for this patch once already and
224 /// *and* carry a `fixes`; idempotent, because an already-closed issue is left 280 /// since reopened* — see `reopened_after_this_patch_closed` for why the bare
225 /// alone, which is what keeps repeated syncs from appending anything. 281 /// invariant is too strong. Cheap, because it only looks at patches that are
282 /// merged *and* carry a `fixes`, and the DAG walk behind the reopen guard is
283 /// only reached while the issue is still open, which after one successful close
284 /// it never is. Idempotent, which is what keeps repeated syncs from appending
285 /// anything.
226 /// 286 ///
227 /// Patches merged during this very scan already had their close attempted 287 /// Patches merged during this very scan already had their close attempted
228 /// inline, in the same operation that emitted the merge. This pass is for the 288 /// inline, in the same operation that emitted the merge. This pass is for the
@@ -277,14 +337,40 @@ fn scan_one_base(
277 emitted 337 emitted
278 } 338 }
279 339
280 /// Walk `base_tip` back to the oldest base among `patches`, collecting 340 /// A commit that is an ancestor of every one of `bases`, or `None` when there
281 /// `Patch:` trailers. 341 /// is no such commit or the set is empty.
342 ///
343 /// Not `merge_base_many`, which is the obvious call and the wrong one.
344 /// libgit2's `git_merge_base_many` is *not* the octopus base: it computes
345 /// `merge_base(oids[0], merge(oids[1..]))`, so its answer depends on argument
346 /// order and is an ancestor of only some of its inputs. On a linear chain
347 /// A-B-C-D-E, `git merge-base A B D` is A but `git merge-base B A D` is B —
348 /// and B is not an ancestor of A. Using it as a walk bound hides commits
349 /// between A and B, which is where a merge of the A-based patch lives, so the
350 /// scan silently records nothing for exactly the case it exists to serve.
351 ///
352 /// Folding binary `merge_base` is sound by induction: each step returns a
353 /// common ancestor of the accumulator and the next base, so the result is an
354 /// ancestor of everything folded in so far. git2 0.19 exposes no octopus
355 /// equivalent, which would be the other way.
356 ///
357 /// `None` on the first pair with no common ancestor (disjoint histories) — the
358 /// safe direction, since an absent bound only costs a longer walk.
359 pub fn common_ancestor(repo: &Repository, bases: &[Oid]) -> Option<Oid> {
360 let mut acc = *bases.first()?;
361 for base in &bases[1..] {
362 acc = repo.merge_base(acc, *base).ok()?;
363 }
364 Some(acc)
365 }
366
367 /// Walk `base_tip` back to a commit older than every open patch's base,
368 /// collecting `Patch:` trailers.
282 /// 369 ///
283 /// The bound matters: everything older than the oldest open patch's base 370 /// The bound matters: nothing older than the oldest open patch's base can have
284 /// cannot have merged a currently-open patch, so walking it is pure cost. The 371 /// merged a currently-open patch, so walking it is pure cost. It has to be an
285 /// merge-base of all the recorded bases is an ancestor of every one of them, so 372 /// ancestor of *every* recorded base, or the walk skips the region where some
286 /// hiding it can never hide a commit we needed — and when the bases are 373 /// patch's merge actually sits — see `common_ancestor`.
287 /// linearly ordered, which is the normal case, it *is* the oldest of them.
288 /// 374 ///
289 /// The first trailer seen for a given prefix wins. The walk is newest-first, so 375 /// The first trailer seen for a given prefix wins. The walk is newest-first, so
290 /// that is the newest commit carrying it: the rebased tip, or the squash commit. 376 /// that is the newest commit carrying it: the rebased tip, or the squash commit.
@@ -301,11 +387,9 @@ fn collect_trailers(
301 .iter() 387 .iter()
302 .filter_map(|p| p.effective_base(repo)) 388 .filter_map(|p| p.effective_base(repo))
303 .collect(); 389 .collect();
304 if !bases.is_empty() { 390 if let Some(bound) = common_ancestor(repo, &bases) {
305 if let Ok(bound) = repo.merge_base_many(&bases) { 391 // A failure to hide is not fatal — it only means a longer walk.
306 // A failure to hide is not fatal — it only means a longer walk. 392 let _ = revwalk.hide(bound);
307 let _ = revwalk.hide(bound);
308 }
309 } 393 }
310 394
311 let mut found = Vec::new(); 395 let mut found = Vec::new();
@@ -400,7 +484,15 @@ fn record_from_trailer(
400 // Whether or not the merge was freshly emitted, the `fixes` issue must end 484 // Whether or not the merge was freshly emitted, the `fixes` issue must end
401 // up closed — see `close_fixed_issue` for why the retry has to be able to 485 // up closed — see `close_fixed_issue` for why the retry has to be able to
402 // run on a pass where the merge itself was a no-op. 486 // run on a pass where the merge itself was a no-op.
403 close_fixed_issue(repo, &patch); 487 //
488 // Warn about an unresolvable issue only on the pass that actually recorded
489 // the merge. A trailer stays in history forever, so while any other open
490 // patch keeps this base branch walked, this line is reached on every sync
491 // for an already-merged patch — and warning there would repeat, every sync,
492 // about something nobody can act on. That is the same noise
493 // `retry_pending_closes` is quiet about, arriving by a different route.
494 let warn = outcome == MergeOutcome::Recorded;
495 close_fixed_issue_inner(repo, &patch, warn);
404 496
405 Ok(outcome) 497 Ok(outcome)
406 } 498 }
src/patch.rs
Old New
@@ -178,6 +178,35 @@ pub fn list_to_writer(
178 Ok(()) 178 Ok(())
179 } 179 }
180 180
181 /// Serialize a patch for `--json`, adding `"looks_merged": true` when
182 /// reachability suggests a merge nobody has recorded.
183 ///
184 /// The hint lives beside `status`, never inside it: `status` is the recorded
185 /// fact and stays `"open"`. But leaving the hint out of JSON altogether is not
186 /// neutral either — the text and JSON paths render the same entries, so a
187 /// consumer that only reads JSON cannot tell "no hint" from "hints are not
188 /// reported here", and JSON-reading agents are the one consumer with no text
189 /// output to fall back on.
190 ///
191 /// It is added here rather than on `PatchState` because computing it needs the
192 /// repository, and because `PatchState` is what the on-disk fold cache stores:
193 /// a derived field in there would be written to disk as though it were folded
194 /// state, and served stale from a cache entry keyed on a DAG tip that says
195 /// nothing about where the base branch has since moved.
196 ///
197 /// Skipped entirely when false, so it never reads as a positive assertion that
198 /// a patch is unmerged — the hint cannot see a squash, so its absence means
199 /// nothing.
200 fn patch_json_value(repo: &Repository, patch: &PatchState) -> Result<serde_json::Value, Error> {
201 let mut value = serde_json::to_value(patch)?;
202 if patch.looks_merged(repo) {
203 if let Some(obj) = value.as_object_mut() {
204 obj.insert("looks_merged".to_string(), serde_json::Value::Bool(true));
205 }
206 }
207 Ok(value)
208 }
209
181 pub fn list_json( 210 pub fn list_json(
182 repo: &Repository, 211 repo: &Repository,
183 show_closed: bool, 212 show_closed: bool,
@@ -186,14 +215,17 @@ pub fn list_json(
186 labels: &[String], 215 labels: &[String],
187 ) -> Result<String, crate::error::Error> { 216 ) -> Result<String, crate::error::Error> {
188 let entries = list(repo, show_closed, show_archived, None, None, sort, labels)?; 217 let entries = list(repo, show_closed, show_archived, None, None, sort, labels)?;
189 let patches: Vec<&PatchState> = entries.iter().map(|e| &e.patch).collect(); 218 let patches: Vec<serde_json::Value> = entries
219 .iter()
220 .map(|e| patch_json_value(repo, &e.patch))
221 .collect::<Result<_, _>>()?;
190 Ok(serde_json::to_string_pretty(&patches)?) 222 Ok(serde_json::to_string_pretty(&patches)?)
191 } 223 }
192 224
193 pub fn show_json(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> { 225 pub fn show_json(repo: &Repository, id_prefix: &str) -> Result<String, crate::error::Error> {
194 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?; 226 let (ref_name, id) = state::resolve_patch_ref(repo, id_prefix)?;
195 let p = PatchState::from_ref(repo, &ref_name, &id)?; 227 let p = PatchState::from_ref(repo, &ref_name, &id)?;
196 Ok(serde_json::to_string_pretty(&p)?) 228 Ok(serde_json::to_string_pretty(&patch_json_value(repo, &p)?)?)
197 } 229 }
198 230
199 pub fn show(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::error::Error> { 231 pub fn show(repo: &Repository, id_prefix: &str) -> Result<PatchState, crate::error::Error> {
tests/collab_test.rs
Old New
@@ -1056,6 +1056,10 @@ fn test_reachability_is_a_hint_and_does_not_set_the_status() {
1056 repo.reference("refs/heads/main", feat_tip, true, "manual merge") 1056 repo.reference("refs/heads/main", feat_tip, true, "manual merge")
1057 .unwrap(); 1057 .unwrap();
1058 1058
1059 // Captured before anything derives, so the assertion below spans the
1060 // derivation it is meant to guard.
1061 let tip_before = repo.refname_to_id(&ref_name).unwrap();
1062
1059 // The patch now looks merged, and is still Open until someone records it. 1063 // The patch now looks merged, and is still Open until someone records it.
1060 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap(); 1064 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();
1061 assert!(state.looks_merged(&repo), "the hint should fire"); 1065 assert!(state.looks_merged(&repo), "the hint should fire");
@@ -1066,13 +1070,13 @@ fn test_reachability_is_a_hint_and_does_not_set_the_status() {
1066 ); 1070 );
1067 assert_eq!(state.status_display(&repo), "merged?"); 1071 assert_eq!(state.status_display(&repo), "merged?");
1068 1072
1069 // Recording it is what makes it Merged — and reading it never did.
1070 let tip_before = repo.refname_to_id(&ref_name).unwrap();
1071 assert_eq!( 1073 assert_eq!(
1072 repo.refname_to_id(&ref_name).unwrap(), 1074 repo.refname_to_id(&ref_name).unwrap(),
1073 tip_before, 1075 tip_before,
1074 "deriving state must not append to the DAG" 1076 "deriving state must not append to the DAG"
1075 ); 1077 );
1078
1079 // Recording it is what makes it Merged.
1076 patch::merge(&repo, &id, None, true).unwrap(); 1080 patch::merge(&repo, &id, None, true).unwrap();
1077 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap(); 1081 let state = PatchState::from_ref_uncached(&repo, &ref_name, &id).unwrap();
1078 assert_eq!(state.status, PatchStatus::Merged); 1082 assert_eq!(state.status, PatchStatus::Merged);
tests/merge_recording_test.rs
Old New
@@ -94,6 +94,60 @@ fn patch_events_ref(repo: &TestRepo, short: &str) -> String {
94 .to_string() 94 .to_string()
95 } 95 }
96 96
97 /// The issue's ref wherever it currently lives — `close` archives it and
98 /// `reopen` moves it back, so neither namespace alone will do.
99 fn issue_ref_of(repo: &TestRepo, short: &str) -> String {
100 let out = repo.git(&["for-each-ref", "--format=%(refname)", "refs/collab/"]);
101 out.lines()
102 .find(|r| r.contains(short) && r.contains("issues/"))
103 .unwrap_or_else(|| panic!("no issue ref for {} in {}", short, out))
104 .to_string()
105 }
106
107 /// Like `patch_with_trailer`, but the patch declares the issue it fixes.
108 fn patch_fixing_issue(repo: &TestRepo, branch: &str, file: &str, issue: &str) -> String {
109 repo.git(&["checkout", "-b", branch]);
110 repo.commit_file(file, "content", &format!("work on {}", branch));
111 let out = repo.run_ok(&[
112 "patch", "create", "-t", branch, "-B", branch, "--fixes", issue,
113 ]);
114 let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
115 stamp_head(repo, &short);
116 repo.run_ok(&["patch", "revise", &short, "-b", "stamped"]);
117 repo.git(&["checkout", "main"]);
118 short
119 }
120
121 /// Run `git` in another working tree, under `env_from`'s isolated HOME so the
122 /// signing key and git config are the test's, not the developer's.
123 fn git_in(env_from: &TestRepo, dir: &std::path::Path, args: &[&str]) -> String {
124 let mut command = Command::new("git");
125 env_from.apply_env(&mut command);
126 let output = command.args(args).current_dir(dir).output().unwrap();
127 assert!(
128 output.status.success(),
129 "git {:?} failed: {}",
130 args,
131 String::from_utf8_lossy(&output.stderr)
132 );
133 String::from_utf8(output.stdout).unwrap()
134 }
135
136 /// Run `git-collab` in another working tree, same isolation as `git_in`.
137 fn collab_in(env_from: &TestRepo, dir: &std::path::Path, args: &[&str]) -> String {
138 let mut command = Command::new(env!("CARGO_BIN_EXE_git-collab"));
139 env_from.apply_env(&mut command);
140 let output = command.args(args).current_dir(dir).output().unwrap();
141 assert!(
142 output.status.success(),
143 "git-collab {:?} failed:\nstdout: {}\nstderr: {}",
144 args,
145 String::from_utf8_lossy(&output.stdout),
146 String::from_utf8_lossy(&output.stderr)
147 );
148 String::from_utf8(output.stdout).unwrap()
149 }
150
97 // --------------------------------------------------------------------------- 151 // ---------------------------------------------------------------------------
98 // The event 152 // The event
99 // --------------------------------------------------------------------------- 153 // ---------------------------------------------------------------------------
@@ -635,3 +689,263 @@ fn a_recorded_merge_is_not_reported_as_a_hint() {
635 list 689 list
636 ); 690 );
637 } 691 }
692
693 // ---------------------------------------------------------------------------
694 // The walk bound must be an ancestor of *every* recorded base
695 // ---------------------------------------------------------------------------
696
697 #[test]
698 fn common_ancestor_is_an_ancestor_of_every_base_whatever_the_order() {
699 // `merge_base_many` is the obvious call and the wrong one: it computes
700 // `merge_base(oids[0], merge(oids[1..]))`, so on a linear chain A-B-C-D-E
701 // it answers A for `[A, B, D]` but B for `[B, A, D]` — and B is not an
702 // ancestor of A. Feeding it the orderings it gets wrong is the only way to
703 // tell the two implementations apart.
704 //
705 // This needs *three* bases. For two, `merge_base_many` degenerates to the
706 // symmetric binary `merge_base` and is correct, so no two-base test — at
707 // this level or end to end — can distinguish them.
708 let repo = TestRepo::new("Alice", "alice@example.com");
709 let mut chain = Vec::new();
710 for name in ["A", "B", "C", "D", "E"] {
711 chain.push(repo.commit_file("f.txt", name, name));
712 }
713 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
714 let oid = |s: &String| git2::Oid::from_str(s).unwrap();
715 let (a, b, d) = (oid(&chain[0]), oid(&chain[1]), oid(&chain[3]));
716
717 for order in [[a, b, d], [b, a, d], [d, a, b], [d, b, a]] {
718 let bound = git_collab::merge_scan::common_ancestor(&git_repo, &order)
719 .expect("a linear chain always has a common ancestor");
720 for base in order {
721 let is_ancestor =
722 bound == base || git_repo.graph_descendant_of(base, bound).unwrap_or(false);
723 assert!(
724 is_ancestor,
725 "bound {:.8} must be an ancestor of every base, but is not of {:.8}",
726 bound, base
727 );
728 }
729 assert_eq!(bound, a, "the bound should be the oldest base");
730 }
731 }
732
733 #[test]
734 fn common_ancestor_of_nothing_is_nothing() {
735 let repo = TestRepo::new("Alice", "alice@example.com");
736 let git_repo = git2::Repository::open(repo.dir.path()).unwrap();
737 assert!(git_collab::merge_scan::common_ancestor(&git_repo, &[]).is_none());
738 }
739
740 #[test]
741 fn sync_records_a_merge_older_than_another_open_patchs_base() {
742 // The multi-base bound, end to end. Three open patches share `main` with
743 // bases at three points of its history, and the oldest one's merge sits
744 // below the other two's bases. A bound that is not an ancestor of *every*
745 // base hides that merge commit and the scan records nothing — silently, and
746 // for a squash, so `looks_merged` cannot flag it either.
747 let (repo, _bare) = repo_with_origin();
748
749 // P1, based at main's first commit, squash-merged onto main.
750 let p1 = patch_with_trailer(&repo, "feat1", "a.txt");
751 squash_merge_keeping_message(&repo, "feat1");
752
753 // main moves on, and P2 is based there.
754 repo.commit_file("upstream1.txt", "u1", "upstream work");
755 let p2 = patch_with_trailer(&repo, "feat2", "b.txt");
756
757 // main moves again, and P3 is based there. Three distinct bases.
758 repo.commit_file("upstream2.txt", "u2", "more upstream work");
759 let p3 = patch_with_trailer(&repo, "feat3", "c.txt");
760
761 repo.run_ok(&["sync"]);
762
763 assert_eq!(
764 show_json(&repo, &p1)["status"],
765 "merged",
766 "the merge sits below the other patches' bases; the bound must not hide it"
767 );
768 assert_eq!(show_json(&repo, &p2)["status"], "open");
769 assert_eq!(show_json(&repo, &p3)["status"], "open");
770 }
771
772 // ---------------------------------------------------------------------------
773 // The retry must not override a deliberate reopen
774 // ---------------------------------------------------------------------------
775
776 #[test]
777 fn an_issue_reopened_after_a_merge_closed_it_stays_open() {
778 // Reopening a merged patch's issue is ordinary — the fix landed and turned
779 // out to be wrong. The retry exists to restore a close that never happened;
780 // it must never override a decision made after one did. Without the reopen
781 // clause this re-closes on every sync, forever, and silently.
782 let (repo, _bare) = repo_with_origin();
783 let issue = repo.issue_open("Broken thing");
784 let _short = patch_fixing_issue(&repo, "feat", "a.txt", &issue);
785 squash_merge_keeping_message(&repo, "feat");
786
787 repo.run_ok(&["sync"]);
788 assert_eq!(issue_json(&repo, &issue)["status"], "closed");
789
790 repo.run_ok(&["issue", "reopen", &issue]);
791 assert_eq!(issue_json(&repo, &issue)["status"], "open");
792 let after_reopen = count_events(&repo, &issue_ref_of(&repo, &issue));
793
794 repo.run_ok(&["sync"]);
795 repo.run_ok(&["sync"]);
796
797 assert_eq!(
798 issue_json(&repo, &issue)["status"],
799 "open",
800 "a deliberate reopen must survive the retry"
801 );
802 assert_eq!(
803 count_events(&repo, &issue_ref_of(&repo, &issue)),
804 after_reopen,
805 "and nothing should have been appended trying"
806 );
807 }
808
809 #[test]
810 fn patch_merge_does_not_reclose_an_issue_reopened_after_its_merge() {
811 // The same guard on the hand-recorded path. `patch merge` on an
812 // already-merged patch is a no-op, and it must not smuggle a re-close in.
813 let repo = TestRepo::new("Alice", "alice@example.com");
814 let issue = repo.issue_open("Broken thing");
815 repo.git(&["checkout", "-b", "feat"]);
816 repo.commit_file("a.txt", "x", "the fix");
817 let out = repo.run_ok(&[
818 "patch", "create", "-t", "The fix", "-B", "feat", "--fixes", &issue,
819 ]);
820 let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
821 repo.git(&["checkout", "main"]);
822 repo.git(&["merge", "--ff-only", "feat"]);
823
824 repo.run_ok(&["patch", "merge", &short]);
825 assert_eq!(issue_json(&repo, &issue)["status"], "closed");
826
827 repo.run_ok(&["issue", "reopen", &issue]);
828 repo.run_ok(&["patch", "merge", &short]);
829
830 assert_eq!(issue_json(&repo, &issue)["status"], "open");
831 }
832
833 #[test]
834 fn a_reopened_issue_is_still_closed_by_a_different_merged_patch() {
835 // The guard is scoped to the patch whose close was overridden, not to the
836 // issue. Another patch naming the same issue has had no decision of its own
837 // overridden, so its close still fires.
838 let (repo, _bare) = repo_with_origin();
839 let issue = repo.issue_open("Broken thing");
840
841 let first = patch_fixing_issue(&repo, "feat1", "a.txt", &issue);
842 squash_merge_keeping_message(&repo, "feat1");
843 repo.run_ok(&["sync"]);
844 assert_eq!(issue_json(&repo, &issue)["status"], "closed");
845
846 repo.run_ok(&["issue", "reopen", &issue]);
847 let second = patch_fixing_issue(&repo, "feat2", "b.txt", &issue);
848 squash_merge_keeping_message(&repo, "feat2");
849
850 repo.run_ok(&["sync"]);
851
852 assert_eq!(show_json(&repo, &first)["status"], "merged");
853 assert_eq!(show_json(&repo, &second)["status"], "merged");
854 assert_eq!(
855 issue_json(&repo, &issue)["status"],
856 "closed",
857 "the second patch's close has not been overridden by anyone"
858 );
859 }
860
861 // ---------------------------------------------------------------------------
862 // Two clones recording the same merge converge
863 // ---------------------------------------------------------------------------
864
865 #[test]
866 fn two_clones_recording_a_merge_concurrently_converge() {
867 let (alice, bare) = repo_with_origin();
868 alice.git(&["checkout", "-b", "feat"]);
869 alice.commit_file("a.txt", "x", "the patch");
870 let out = alice.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
871 let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
872 alice.git(&["checkout", "main"]);
873 alice.git(&["merge", "--ff-only", "feat"]);
874 alice.git(&["push", "origin", "main"]);
875 alice.run_ok(&["sync"]);
876
877 // Bob clones the same bare remote and picks up the patch.
878 let bob_root = TempDir::new().unwrap();
879 let bob = bob_root.path().join("clone");
880 git_in(
881 &alice,
882 bob_root.path(),
883 &["clone", bare.path().to_str().unwrap(), "clone"],
884 );
885 git_in(&alice, &bob, &["config", "user.name", "Bob"]);
886 git_in(&alice, &bob, &["config", "user.email", "bob@example.com"]);
887 git_in(&alice, &bob, &["config", "collab.autoSync", "false"]);
888 collab_in(&alice, &bob, &["init"]);
889 collab_in(&alice, &bob, &["sync"]);
890
891 // Both record the merge without having seen the other's event.
892 alice.run_ok(&["patch", "merge", &short]);
893 collab_in(&alice, &bob, &["patch", "merge", &short]);
894
895 // Alice pushes first, so Bob's sync has to reconcile a genuinely divergent
896 // DAG rather than fast-forward.
897 alice.run_ok(&["sync"]);
898 collab_in(&alice, &bob, &["sync"]);
899 alice.run_ok(&["sync"]);
900
901 let bob_json: Value =
902 serde_json::from_str(&collab_in(&alice, &bob, &["patch", "show", &short, "--json"]))
903 .unwrap();
904 assert_eq!(show_json(&alice, &short)["status"], "merged");
905 assert_eq!(bob_json["status"], "merged");
906
907 let events_ref = patch_events_ref(&alice, &short);
908 assert_eq!(
909 alice.git(&["rev-parse", &events_ref]).trim(),
910 git_in(&alice, &bob, &["rev-parse", &events_ref]).trim(),
911 "both clones must end at the same DAG tip"
912 );
913 }
914
915 // ---------------------------------------------------------------------------
916 // The hint is reported in --json too: beside the status, never inside it
917 // ---------------------------------------------------------------------------
918
919 #[test]
920 fn json_reports_the_merge_hint_beside_the_status() {
921 let repo = TestRepo::new("Alice", "alice@example.com");
922 repo.git(&["checkout", "-b", "feat"]);
923 repo.commit_file("a.txt", "x", "the patch");
924 let out = repo.run_ok(&["patch", "create", "-t", "Feature", "-B", "feat"]);
925 let short = out.trim().strip_prefix("Created patch ").unwrap().to_string();
926 repo.git(&["checkout", "main"]);
927
928 // Not merged: no hint at all, so its absence never reads as an assertion.
929 let json = show_json(&repo, &short);
930 assert_eq!(json["status"], "open");
931 assert!(json.get("looks_merged").is_none(), "{}", json);
932
933 repo.git(&["merge", "--ff-only", "feat"]);
934
935 let json = show_json(&repo, &short);
936 assert_eq!(json["status"], "open", "the hint is never the status");
937 assert_eq!(json["looks_merged"], true);
938
939 let list: Value = serde_json::from_str(&repo.run_ok(&["patch", "list", "--json"])).unwrap();
940 assert_eq!(list[0]["status"], "open");
941 assert_eq!(
942 list[0]["looks_merged"], true,
943 "list and show render the same entries; the hint has to be in both"
944 );
945
946 // Once recorded it is a fact, and the hint goes away.
947 repo.run_ok(&["patch", "merge", &short]);
948 let json = show_json(&repo, &short);
949 assert_eq!(json["status"], "merged");
950 assert!(json.get("looks_merged").is_none(), "{}", json);
951 }