a73x

1b7cbcb4

Make interdiff rebase-aware and stop recomputing bases at display time

a73x   2026-08-10 16:35

Commit message
Make interdiff rebase-aware and stop recomputing bases at display time

interdiff was a flat tree-to-tree diff between two revision trees with no
idea what base either one stood on. Once a rebase separated them, every
upstream commit rebased over showed up as the author's work: a patch with
one real edit and a upstream that advanced three commits emitted four
files, three of them pure churn.

Each revision already records the base it was taken against, so "did the
author rebase between these two" is exactly "did base change". Bases
equal: tree diff, as before, which also makes a pure squash an empty
interdiff. Bases differ: replay the older revision onto the newer one's
base with git2::merge_commits and diff that against the newer revision as
recorded. The direction matters -- the newer revision is the one under
review and is shown exactly as its author wrote it.

The replay stays an index and is never written to the object database:
rendering a diff is a read.

A conflicting replay is reported as a conflict naming the files. It means
upstream and the patch changed the same lines and no "only the author's
changes" view exists, so there is nothing honest to render.

The same recompute-at-display-time mistake made every merged patch's
historical diff unreadable: a fast-forward collapses merge_base(base_tip,
head) onto the head itself. Both the per-revision diff and the head diff
now read the recorded base and fall back to the old recompute only when
none was recorded.

Two disclosure gaps go with it. --between now says when the bases differ
and when one was never recorded, and patch show displays each revision's
base, so a reviewer can tell a clean interdiff from a polluted one.
--between on a single-revision patch now says how many revisions exist
instead of printing an empty diff that reads as "these are identical".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src/lib.rs
Old New
@@ -461,9 +461,18 @@ pub fn run(cli: cli::Cli, repo: &Repository) -> Result<(), error::Error> {
461 .as_deref() 461 .as_deref()
462 .map(|b| format!(" {}", b)) 462 .map(|b| format!(" {}", b))
463 .unwrap_or_default(); 463 .unwrap_or_default();
464 // Show the base too: whether a rebase separates two
465 // revisions changes what an interdiff between them
466 // means, and nothing else in the output says.
467 let base_display = rev
468 .base
469 .as_deref()
470 .filter(|b| b.len() >= 8)
471 .map(|b| format!(" base {}", &b[..8]))
472 .unwrap_or_default();
464 println!( 473 println!(
465 " r{}: {} ({}){}", 474 " r{}: {}{} ({}){}",
466 rev.number, short, rev.timestamp, body_display 475 rev.number, short, base_display, rev.timestamp, body_display
467 ); 476 );
468 } 477 }
469 } 478 }
src/patch.rs
Old New
@@ -458,6 +458,13 @@ pub fn diff(
458 458
459 /// Resolve the base tree for diffing: find the merge-base between the base branch 459 /// Resolve the base tree for diffing: find the merge-base between the base branch
460 /// and the given head OID, falling back to the base branch tip if no merge-base exists. 460 /// and the given head OID, falling back to the base branch tip if no merge-base exists.
461 ///
462 /// This is the pre-recorded-base behaviour and now only the fallback. Recomputing
463 /// a merge-base at display time answers a different question than the one asked:
464 /// once the patch has landed on the base branch the merge-base *is* the head, so
465 /// the diff is taken against its own tree and renders empty. Prefer
466 /// [`recorded_base_tree`], which reads the base captured when the revision was
467 /// recorded and was still true.
461 fn resolve_base_tree<'a>( 468 fn resolve_base_tree<'a>(
462 repo: &'a Repository, 469 repo: &'a Repository,
463 base_branch: &str, 470 base_branch: &str,
@@ -472,6 +479,20 @@ fn resolve_base_tree<'a>(
472 Ok(Some(repo.find_commit(tree_source)?.tree()?)) 479 Ok(Some(repo.find_commit(tree_source)?.tree()?))
473 } 480 }
474 481
482 /// The base a revision recorded, provided it recorded one and the commit is
483 /// still present. A base naming an object this clone does not have is no more
484 /// usable than no base at all, so both degrade the same way.
485 fn recorded_base(repo: &Repository, revision: &state::Revision) -> Option<Oid> {
486 let oid = Oid::from_str(revision.base.as_deref()?).ok()?;
487 repo.find_commit(oid).ok().map(|c| c.id())
488 }
489
490 /// Tree of a recorded base OID, or `None` when there is nothing recorded to read.
491 fn recorded_base_tree<'a>(repo: &'a Repository, base: Option<Oid>) -> Option<git2::Tree<'a>> {
492 let oid = base?;
493 repo.find_commit(oid).ok()?.tree().ok()
494 }
495
475 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff. 496 /// Generate a diff string from a patch's base and head using three-dot (merge-base) diff.
476 pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<String, Error> { 497 pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<String, Error> {
477 let head_oid = patch.resolve_head(repo)?; 498 let head_oid = patch.resolve_head(repo)?;
@@ -479,7 +500,10 @@ pub fn generate_diff(repo: &Repository, patch: &state::PatchState) -> Result<Str
479 .find_commit(head_oid) 500 .find_commit(head_oid)
480 .map_err(|e| Error::Cmd(format!("bad head ref: {}", e)))?; 501 .map_err(|e| Error::Cmd(format!("bad head ref: {}", e)))?;
481 let head_tree = head_commit.tree()?; 502 let head_tree = head_commit.tree()?;
482 let base_tree = resolve_base_tree(repo, &patch.base_ref, head_oid)?; 503 let base_tree = match recorded_base_tree(repo, patch.effective_base(repo)) {
504 Some(tree) => Some(tree),
505 None => resolve_base_tree(repo, &patch.base_ref, head_oid)?,
506 };
483 507
484 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; 508 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?;
485 format_diff(&git_diff) 509 format_diff(&git_diff)
@@ -499,37 +523,196 @@ fn generate_diff_at_revision(
499 523
500 let head_tree = repo.find_tree(Oid::from_str(&revision.tree)?)?; 524 let head_tree = repo.find_tree(Oid::from_str(&revision.tree)?)?;
501 let commit_oid = Oid::from_str(&revision.commit)?; 525 let commit_oid = Oid::from_str(&revision.commit)?;
502 let base_tree = resolve_base_tree(repo, &patch.base_ref, commit_oid)?; 526 let base_tree = match recorded_base_tree(repo, recorded_base(repo, revision)) {
527 Some(tree) => Some(tree),
528 None => resolve_base_tree(repo, &patch.base_ref, commit_oid)?,
529 };
503 530
504 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?; 531 let git_diff = repo.diff_tree_to_tree(base_tree.as_ref(), Some(&head_tree), None)?;
505 format_diff(&git_diff) 532 format_diff(&git_diff)
506 } 533 }
507 534
508 /// Compute the interdiff between two revisions. 535 /// Compute the interdiff between two revisions: what the author changed in
536 /// response to review, and nothing else.
537 ///
538 /// A flat tree-to-tree diff answers that only while both revisions stand on the
539 /// same base. Once a rebase separates them, every upstream commit rebased over
540 /// shows up as the author's work. With each revision carrying the base it was
541 /// recorded against, "did the author rebase between these two" is exactly "did
542 /// `base` change", and there are two cases:
543 ///
544 /// - **Bases equal.** Tree diff. This is also the pure-squash case: squashing
545 /// leaves the tree alone, so the interdiff is empty, which is the true and
546 /// useful answer — the author restructured commits and changed no code.
547 /// - **Bases differ.** Replay the *older* revision onto the *newer* one's base
548 /// and diff that against the newer revision as recorded. Upstream churn
549 /// appears on both sides and cancels.
550 ///
551 /// The direction is load-bearing. The newer revision is the one under review and
552 /// is shown exactly as its author recorded it; the older one is the reference
553 /// point brought forward. Replaying the newer one backwards would show the
554 /// reviewer a synthesised version of the very revision they are evaluating.
555 ///
556 /// `from`/`to` order only picks the direction of the diff. Which revision gets
557 /// replayed is decided by revision number, so `--between 2 1` is the reverse of
558 /// `--between 1 2` and not a different comparison.
509 pub fn interdiff( 559 pub fn interdiff(
510 repo: &Repository, 560 repo: &Repository,
511 patch: &PatchState, 561 patch: &PatchState,
512 from_rev: u32, 562 from_rev: u32,
513 to_rev: u32, 563 to_rev: u32,
514 ) -> Result<String, Error> { 564 ) -> Result<String, Error> {
515 let from = patch 565 let from = find_revision(patch, from_rev)?;
516 .revisions 566 let to = find_revision(patch, to_rev)?;
517 .iter() 567
518 .find(|r| r.number == from_rev) 568 if from_rev == to_rev {
519 .ok_or_else(|| Error::Cmd(format!("revision {} not found", from_rev)))?; 569 let count = patch.revisions.len();
520 let to = patch 570 return Err(Error::Cmd(format!(
571 "nothing to compare: revision {} against itself (patch has {} revision{})",
572 from_rev,
573 count,
574 if count == 1 { "" } else { "s" }
575 )));
576 }
577
578 let from_tree = repo.find_tree(Oid::from_str(&from.tree)?)?;
579 let to_tree = repo.find_tree(Oid::from_str(&to.tree)?)?;
580 let plain = |note: String| -> Result<String, Error> {
581 let git_diff = repo.diff_tree_to_tree(Some(&from_tree), Some(&to_tree), None)?;
582 Ok(note + &format_diff(&git_diff)?)
583 };
584
585 let (older, newer) = if from_rev < to_rev {
586 (from, to)
587 } else {
588 (to, from)
589 };
590 let (older_base, newer_base) = (recorded_base(repo, older), recorded_base(repo, newer));
591
592 // Migration. A revision that recorded no base cannot say whether a rebase
593 // happened, so this degrades to what every patch used to do — and says so,
594 // because an output that silently means two different things is the whole
595 // complaint.
596 let (Some(older_base), Some(newer_base)) = (older_base, newer_base) else {
597 let unknown: Vec<String> = [(older, older_base), (newer, newer_base)]
598 .iter()
599 .filter(|(_, base)| base.is_none())
600 .map(|(r, _)| format!("r{}", r.number))
601 .collect();
602 return plain(format!(
603 "note: no base recorded for {}; the diff may include upstream changes\n",
604 unknown.join(" and ")
605 ));
606 };
607
608 if older_base == newer_base {
609 return plain(String::new());
610 }
611
612 let Ok(older_commit) = Oid::from_str(&older.commit) else {
613 return plain(format!(
614 "note: no commit recorded for r{}; the diff may include upstream changes\n",
615 older.number
616 ));
617 };
618
619 let replayed = match replay_onto(repo, older_commit, newer_base)? {
620 Replay::Clean(index) => index,
621 Replay::Conflicted(paths) => {
622 return Err(Error::Cmd(format!(
623 "cannot compute interdiff: replaying r{} onto r{}'s base conflicts in {}\n\
624 upstream and the patch changed the same lines, so there is no honest \
625 \"only the author's changes\" view to show. \
626 Try `git-collab patch diff {} --revision {}` for r{}'s full diff.",
627 older.number,
628 newer.number,
629 paths.join(", "),
630 &patch.id[..8.min(patch.id.len())],
631 newer.number,
632 newer.number,
633 )))
634 }
635 };
636
637 let note = format!(
638 "note: bases differ (r{} {}, r{} {}); r{} replayed onto r{}'s base, upstream changes excluded\n",
639 older.number,
640 short_oid(older_base),
641 newer.number,
642 short_oid(newer_base),
643 older.number,
644 newer.number,
645 );
646
647 // The replayed side lives in an in-memory index rather than a written tree:
648 // rendering a diff is a read and has no business adding objects to the odb.
649 let mut opts = git2::DiffOptions::new();
650 let git_diff = if from_rev < to_rev {
651 // Older (replayed) -> newer. `diff_tree_to_index` only runs tree-first,
652 // so ask for it reversed. libgit2 swaps the path prefixes along with the
653 // sides, so they go in pre-swapped to come out as the `a/`, `b/` every
654 // consumer of a unified diff expects.
655 opts.reverse(true).old_prefix("b").new_prefix("a");
656 repo.diff_tree_to_index(Some(&to_tree), Some(&replayed), Some(&mut opts))?
657 } else {
658 repo.diff_tree_to_index(Some(&from_tree), Some(&replayed), Some(&mut opts))?
659 };
660 Ok(note + &format_diff(&git_diff)?)
661 }
662
663 fn find_revision(patch: &PatchState, number: u32) -> Result<&state::Revision, Error> {
664 patch
521 .revisions 665 .revisions
522 .iter() 666 .iter()
523 .find(|r| r.number == to_rev) 667 .find(|r| r.number == number)
524 .ok_or_else(|| Error::Cmd(format!("revision {} not found", to_rev)))?; 668 .ok_or_else(|| Error::Cmd(format!("revision {} not found", number)))
669 }
670
671 fn short_oid(oid: Oid) -> String {
672 oid.to_string().chars().take(8).collect()
673 }
525 674
526 let from_tree_oid = Oid::from_str(&from.tree)?; 675 /// A conflicted replay is an outcome, not a failure: it is the true answer to
527 let to_tree_oid = Oid::from_str(&to.tree)?; 676 /// "what did the author change", namely that no such view exists. Kept distinct
528 let from_tree = repo.find_tree(from_tree_oid)?; 677 /// from `Error` so a libgit2 failure is never reported to the reader as a
529 let to_tree = repo.find_tree(to_tree_oid)?; 678 /// conflict in a file it names.
679 enum Replay {
680 Clean(git2::Index),
681 Conflicted(Vec<String>),
682 }
530 683
531 let git_diff = repo.diff_tree_to_tree(Some(&from_tree), Some(&to_tree), None)?; 684 /// Cherry-pick a whole revision onto another base, in memory.
532 format_diff(&git_diff) 685 ///
686 /// `merge_commits(base, revision)` is the cherry-pick: their merge-base is the
687 /// base the revision was recorded on, so the three-way merge applies exactly the
688 /// revision's own changes on top of `base`. No working tree, no checkout, and
689 /// nothing written to the object database — the result stays an index, which is
690 /// all a diff needs.
691 ///
692 /// A conflict means upstream and the patch touched the same lines. Reporting
693 /// that is the honest answer; rendering the merge with conflict markers in it
694 /// would not be.
695 fn replay_onto(repo: &Repository, revision: Oid, base: Oid) -> Result<Replay, Error> {
696 let base_commit = repo.find_commit(base)?;
697 let revision_commit = repo.find_commit(revision)?;
698 let index = repo.merge_commits(&base_commit, &revision_commit, None)?;
699 if !index.has_conflicts() {
700 return Ok(Replay::Clean(index));
701 }
702 let mut paths: Vec<String> = index
703 .conflicts()?
704 .filter_map(|c| c.ok())
705 .filter_map(|c| {
706 let entry = c.our.or(c.their).or(c.ancestor)?;
707 String::from_utf8(entry.path).ok()
708 })
709 .collect();
710 paths.sort();
711 paths.dedup();
712 if paths.is_empty() {
713 paths.push("<unknown path>".to_string());
714 }
715 Ok(Replay::Conflicted(paths))
533 } 716 }
534 717
535 /// Format a git2::Diff as a unified diff string. 718 /// Format a git2::Diff as a unified diff string.
tests/interdiff_test.rs
Old New
@@ -0,0 +1,477 @@
1 //! Interdiff and per-revision diff behaviour.
2 //!
3 //! The defect these cover is a family: `interdiff` used to be a flat
4 //! tree-to-tree diff with no idea what base either revision stood on, so a
5 //! rebase between two revisions dragged every upstream commit rebased over
6 //! into the output and buried the author's actual response to review. The
7 //! related defects are the same mistake made at display time — recomputing a
8 //! merge-base when the answer was recorded at the time it was still true.
9
10 mod common;
11
12 use common::{write_raw_event, TestRepo};
13 use serde_json::json;
14
15 /// Every path a unified diff touches, sorted and deduplicated.
16 ///
17 /// Asserted as an exact set rather than "contains the file I care about":
18 /// the regression being prevented is extra files, and a `contains` assertion
19 /// is blind to exactly that.
20 fn changed_files(diff: &str) -> Vec<String> {
21 let mut paths: Vec<String> = diff
22 .lines()
23 .filter_map(|l| l.strip_prefix("diff --git a/"))
24 .map(|rest| rest.split(" b/").next().unwrap_or(rest).to_string())
25 .collect();
26 paths.sort();
27 paths.dedup();
28 paths
29 }
30
31 fn create_patch(repo: &TestRepo, branch: &str, title: &str) -> String {
32 let out = repo.run_ok(&["patch", "create", "-t", title, "-B", branch]);
33 out.trim()
34 .strip_prefix("Created patch ")
35 .unwrap_or_else(|| panic!("unexpected patch create output: {}", out))
36 .to_string()
37 }
38
39 /// A patch with one real edit, an upstream that advanced three commits while
40 /// it sat in review, and an author who rebased before answering the review.
41 ///
42 /// This is the measured 75%-noise case: the flat tree diff emitted four files,
43 /// one real and three pure upstream churn.
44 fn rebase_over_busy_upstream() -> (TestRepo, String) {
45 let repo = TestRepo::new("Alice", "alice@example.com");
46
47 repo.git(&["checkout", "-b", "feat-rebase"]);
48 repo.commit_file("feature.txt", "v1\n", "feature v1");
49 let id = create_patch(&repo, "feat-rebase", "Rebase noise");
50
51 // Upstream advances, touching only files the patch does not.
52 repo.git(&["checkout", "main"]);
53 repo.commit_file("up1.txt", "one\n", "upstream 1");
54 repo.commit_file("up2.txt", "two\n", "upstream 2");
55 repo.commit_file("up3.txt", "three\n", "upstream 3");
56
57 // The author rebases, then answers the review.
58 repo.git(&["checkout", "feat-rebase"]);
59 repo.git(&["rebase", "main"]);
60 repo.commit_file("feature.txt", "v2\n", "address review");
61 repo.run_ok(&["patch", "revise", &id]);
62 repo.git(&["checkout", "main"]);
63
64 (repo, id)
65 }
66
67 // ===========================================================================
68 // 8db8d346 — interdiff must be rebase-aware
69 // ===========================================================================
70
71 #[test]
72 fn interdiff_across_a_rebase_shows_only_the_authors_changes() {
73 let (repo, id) = rebase_over_busy_upstream();
74
75 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
76
77 assert_eq!(
78 changed_files(&out),
79 vec!["feature.txt".to_string()],
80 "interdiff across a rebase must exclude every upstream commit rebased \
81 over; got:\n{}",
82 out
83 );
84 assert!(
85 out.contains("+v2"),
86 "the author's actual change must survive: {}",
87 out
88 );
89 assert!(
90 !out.contains("up1.txt") && !out.contains("up2.txt") && !out.contains("up3.txt"),
91 "upstream churn leaked into the interdiff: {}",
92 out
93 );
94 }
95
96 #[test]
97 fn interdiff_across_a_rebase_is_reversible() {
98 let (repo, id) = rebase_over_busy_upstream();
99
100 // Asking for 2..1 is the reverse diff, not a different comparison. The
101 // newer revision is still the one shown as recorded.
102 let out = repo.run_ok(&["patch", "diff", &id, "--between", "2", "1"]);
103
104 assert_eq!(changed_files(&out), vec!["feature.txt".to_string()]);
105 assert!(
106 out.contains("+v1"),
107 "reversed interdiff should restore v1: {}",
108 out
109 );
110 }
111
112 #[test]
113 fn squashing_without_changing_the_tree_gives_an_empty_interdiff() {
114 let repo = TestRepo::new("Alice", "alice@example.com");
115
116 repo.git(&["checkout", "-b", "feat-squash"]);
117 repo.commit_file("a.txt", "a\n", "first");
118 repo.commit_file("b.txt", "b\n", "second");
119 let id = create_patch(&repo, "feat-squash", "Squash");
120
121 // Squash both commits into one. Same tree, different commit.
122 repo.git(&["reset", "--soft", "main"]);
123 repo.git(&["commit", "-m", "squashed"]);
124 repo.run_ok(&["patch", "revise", &id]);
125 repo.git(&["checkout", "main"]);
126
127 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
128
129 assert!(
130 changed_files(&out).is_empty(),
131 "a pure squash changes no code, so the interdiff must be empty: {}",
132 out
133 );
134 assert!(
135 out.contains("No diff available"),
136 "an empty interdiff should say so rather than print nothing: {}",
137 out
138 );
139 }
140
141 #[test]
142 fn interdiff_reports_a_conflict_rather_than_a_misleading_diff() {
143 let repo = TestRepo::new("Alice", "alice@example.com");
144 repo.commit_file("shared.txt", "original\n", "add shared");
145
146 repo.git(&["checkout", "-b", "feat-conflict"]);
147 repo.commit_file("shared.txt", "feature version\n", "feature edits shared");
148 let id = create_patch(&repo, "feat-conflict", "Conflicting rebase");
149
150 // Upstream rewrites the same line.
151 repo.git(&["checkout", "main"]);
152 repo.commit_file("shared.txt", "upstream version\n", "upstream edits shared");
153
154 // The author rebases onto that and resolves in favour of the feature.
155 repo.git(&["checkout", "feat-conflict"]);
156 repo.git(&["reset", "--hard", "main"]);
157 repo.commit_file(
158 "shared.txt",
159 "feature version, revised\n",
160 "rebased and revised",
161 );
162 repo.run_ok(&["patch", "revise", &id]);
163 repo.git(&["checkout", "main"]);
164
165 let err = repo.run_err(&["patch", "diff", &id, "--between", "1", "2"]);
166
167 assert!(
168 err.contains("conflict"),
169 "a conflicting replay must be reported as a conflict: {}",
170 err
171 );
172 assert!(
173 err.contains("shared.txt"),
174 "the conflict should name the file: {}",
175 err
176 );
177 }
178
179 // ===========================================================================
180 // 243b7bc0 — differing bases must be disclosed
181 // ===========================================================================
182
183 #[test]
184 fn interdiff_discloses_that_the_bases_differ() {
185 let (repo, id) = rebase_over_busy_upstream();
186
187 let shown = repo.run_ok(&["patch", "show", &id, "--json"]);
188 let json: serde_json::Value = serde_json::from_str(&shown).unwrap();
189 let revisions = json["revisions"].as_array().unwrap();
190 let base1 = revisions[0]["base"].as_str().unwrap().to_string();
191 let base2 = revisions[1]["base"].as_str().unwrap().to_string();
192 assert_ne!(base1, base2, "the fixture must actually rebase");
193
194 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
195
196 assert!(
197 out.contains("bases differ"),
198 "a reviewer cannot tell noise from signal unless the rebase is \
199 disclosed: {}",
200 out
201 );
202 assert!(
203 out.contains(&base1[..8]) && out.contains(&base2[..8]),
204 "the disclosure should name both bases: {}",
205 out
206 );
207 }
208
209 #[test]
210 fn interdiff_on_a_shared_base_says_nothing_about_bases() {
211 let repo = TestRepo::new("Alice", "alice@example.com");
212
213 repo.git(&["checkout", "-b", "feat-same-base"]);
214 repo.commit_file("a.txt", "a\n", "v1");
215 let id = create_patch(&repo, "feat-same-base", "Same base");
216 repo.commit_file("a.txt", "a2\n", "v2");
217 repo.run_ok(&["patch", "revise", &id]);
218 repo.git(&["checkout", "main"]);
219
220 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
221
222 assert_eq!(changed_files(&out), vec!["a.txt".to_string()]);
223 assert!(
224 !out.contains("bases differ"),
225 "an unrebased interdiff is already exactly the author's changes and \
226 needs no caveat: {}",
227 out
228 );
229 }
230
231 #[test]
232 fn patch_show_displays_each_revisions_base() {
233 let (repo, id) = rebase_over_busy_upstream();
234
235 let shown = repo.run_ok(&["patch", "show", &id, "--json"]);
236 let json: serde_json::Value = serde_json::from_str(&shown).unwrap();
237 let base2 = json["revisions"][1]["base"].as_str().unwrap().to_string();
238
239 let out = repo.run_ok(&["patch", "show", &id]);
240 assert!(
241 out.contains(&base2[..8]),
242 "`patch show` must display the base each revision stands on: {}",
243 out
244 );
245 }
246
247 // ===========================================================================
248 // 40001f77 — --between on a single-revision patch
249 // ===========================================================================
250
251 #[test]
252 fn between_on_a_single_revision_patch_says_so() {
253 let repo = TestRepo::new("Alice", "alice@example.com");
254 let id = repo.patch_create("Solo");
255
256 let err = repo.run_err(&["patch", "diff", &id, "--between", "1"]);
257
258 assert!(
259 err.contains("1 revision"),
260 "an empty diff reads as 'these revisions are identical'; the truth is \
261 'there is no second revision': {}",
262 err
263 );
264 }
265
266 #[test]
267 fn between_a_revision_and_itself_says_so() {
268 let repo = TestRepo::new("Alice", "alice@example.com");
269
270 repo.git(&["checkout", "-b", "feat-self"]);
271 repo.commit_file("a.txt", "a\n", "v1");
272 let id = create_patch(&repo, "feat-self", "Self compare");
273 repo.commit_file("a.txt", "a2\n", "v2");
274 repo.run_ok(&["patch", "revise", &id]);
275 repo.git(&["checkout", "main"]);
276
277 let err = repo.run_err(&["patch", "diff", &id, "--between", "2", "2"]);
278 assert!(
279 err.contains("2 revisions"),
280 "comparing a revision with itself should name how many exist: {}",
281 err
282 );
283 }
284
285 // ===========================================================================
286 // 57575b50 — a merged patch's historical diff stays readable
287 // ===========================================================================
288
289 #[test]
290 fn revision_diff_survives_the_patch_being_merged() {
291 let repo = TestRepo::new("Alice", "alice@example.com");
292
293 repo.git(&["checkout", "-b", "feat-merged"]);
294 repo.commit_file("a.txt", "hello\n", "add a");
295 let id = create_patch(&repo, "feat-merged", "Merged patch");
296
297 let before = repo.run_ok(&["patch", "diff", &id, "--revision", "1"]);
298 assert_eq!(changed_files(&before), vec!["a.txt".to_string()]);
299
300 repo.git(&["checkout", "main"]);
301 repo.git(&["merge", "--ff-only", "feat-merged"]);
302
303 let after = repo.run_ok(&["patch", "diff", &id, "--revision", "1"]);
304 assert_eq!(
305 changed_files(&after),
306 vec!["a.txt".to_string()],
307 "a fast-forward merge collapses the recomputed merge-base onto the \
308 revision itself; the recorded base must be used instead: {}",
309 after
310 );
311 assert!(after.contains("+hello"), "{}", after);
312 }
313
314 #[test]
315 fn patch_diff_survives_the_patch_being_merged() {
316 let repo = TestRepo::new("Alice", "alice@example.com");
317
318 repo.git(&["checkout", "-b", "feat-merged-head"]);
319 repo.commit_file("a.txt", "hello\n", "add a");
320 let id = create_patch(&repo, "feat-merged-head", "Merged patch head");
321
322 repo.git(&["checkout", "main"]);
323 repo.git(&["merge", "--ff-only", "feat-merged-head"]);
324
325 let out = repo.run_ok(&["patch", "diff", &id]);
326 assert_eq!(
327 changed_files(&out),
328 vec!["a.txt".to_string()],
329 "the head diff is lost to the same recompute: {}",
330 out
331 );
332 }
333
334 // ===========================================================================
335 // Migration: revisions with no recorded base degrade, they do not fail
336 // ===========================================================================
337
338 #[test]
339 fn revisions_without_a_recorded_base_still_diff_and_say_the_base_is_unknown() {
340 let work = TestRepo::new("Alice", "alice@example.com");
341
342 work.git(&["checkout", "-b", "legacy"]);
343 work.commit_file("x.txt", "one\n", "c1");
344 let c1 = work.git(&["rev-parse", "HEAD"]).trim().to_string();
345 let t1 = work.git(&["rev-parse", "HEAD^{tree}"]).trim().to_string();
346 work.commit_file("y.txt", "two\n", "c2");
347 let c2 = work.git(&["rev-parse", "HEAD"]).trim().to_string();
348 let t2 = work.git(&["rev-parse", "HEAD^{tree}"]).trim().to_string();
349 work.git(&["checkout", "main"]);
350
351 let repo = git2::Repository::open(work.dir.path()).unwrap();
352 let root = write_raw_event(
353 &repo,
354 None,
355 json!({
356 "type": "patch.create",
357 "title": "Legacy without a base",
358 "body": "",
359 "base_ref": "main",
360 "branch": "legacy",
361 "commit": c1,
362 "tree": t1,
363 }),
364 1,
365 );
366 let tip = write_raw_event(
367 &repo,
368 Some(root),
369 json!({
370 "type": "patch.revision",
371 "commit": c2,
372 "tree": t2,
373 }),
374 2,
375 );
376 let id = root.to_string();
377 repo.reference(&format!("refs/collab/patches/{}", id), tip, false, "legacy")
378 .unwrap();
379 drop(repo);
380
381 let out = work.run_ok(&["patch", "diff", &id[..8], "--between", "1", "2"]);
382
383 assert_eq!(
384 changed_files(&out),
385 vec!["y.txt".to_string()],
386 "old patches must degrade to the flat tree diff, not fail: {}",
387 out
388 );
389 assert!(
390 out.contains("no base recorded"),
391 "an undisclosed unknown base is the misleading case this family of \
392 defects is about: {}",
393 out
394 );
395 }
396
397 // ===========================================================================
398 // Rendering a diff is a read
399 // ===========================================================================
400
401 fn count_objects(dir: &std::path::Path) -> usize {
402 let mut n = 0;
403 if let Ok(entries) = std::fs::read_dir(dir) {
404 for e in entries.flatten() {
405 let p = e.path();
406 if p.is_dir() {
407 n += count_objects(&p);
408 } else {
409 n += 1;
410 }
411 }
412 }
413 n
414 }
415
416 /// A rebase where upstream and the patch edit the same file in different
417 /// regions. The replay has to auto-merge rather than reuse a blob wholesale,
418 /// which is the case that would otherwise tempt an implementation into writing
419 /// a merged tree to the object database in the middle of a read.
420 #[test]
421 fn an_auto_merging_replay_shows_only_the_authors_hunk_and_writes_nothing() {
422 let repo = TestRepo::new("Alice", "alice@example.com");
423 repo.commit_file("both.txt", "a\nb\nc\nd\ne\nf\ng\n", "seed");
424 repo.git(&["checkout", "-b", "feat-automerge"]);
425 repo.commit_file("both.txt", "a\nb\nc\nd\ne\nf\nGGG\n", "patch edits the tail");
426 let id = create_patch(&repo, "feat-automerge", "Auto-merging replay");
427 repo.git(&["checkout", "main"]);
428 repo.commit_file("both.txt", "AAA\nb\nc\nd\ne\nf\ng\n", "upstream edits the head");
429 repo.git(&["checkout", "feat-automerge"]);
430 repo.git(&["rebase", "main"]);
431 repo.commit_file("both.txt", "AAA\nb\nc\nd\ne\nf\nHHH\n", "address review");
432 repo.run_ok(&["patch", "revise", &id]);
433 repo.git(&["checkout", "main"]);
434
435 let objects = repo.dir.path().join(".git/objects");
436 let before = count_objects(&objects);
437 let out = repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
438 let after = count_objects(&objects);
439
440 assert_eq!(
441 changed_files(&out),
442 vec!["both.txt".to_string()],
443 "{}",
444 out
445 );
446 assert!(
447 out.contains("-GGG") && out.contains("+HHH"),
448 "the author's hunk must be the one shown: {}",
449 out
450 );
451 assert!(
452 !out.contains("AAA"),
453 "upstream's hunk in the same file must cancel: {}",
454 out
455 );
456 assert_eq!(
457 before, after,
458 "replaying a revision is done in memory; rendering a diff must not add \
459 objects to the object database"
460 );
461 }
462
463 #[test]
464 fn rendering_a_diff_moves_no_refs() {
465 let (repo, id) = rebase_over_busy_upstream();
466
467 let before = repo.git(&["for-each-ref", "refs/collab"]);
468 repo.run_ok(&["patch", "diff", &id]);
469 repo.run_ok(&["patch", "diff", &id, "--revision", "1"]);
470 repo.run_ok(&["patch", "diff", &id, "--between", "1", "2"]);
471 let after = repo.git(&["for-each-ref", "refs/collab"]);
472
473 assert_eq!(
474 before, after,
475 "rendering a diff must not append events or move refs"
476 );
477 }