From eef9089302922257809f45eb7e4a29bbff8e98a3 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Tue, 13 Aug 2024 13:38:51 -0700 Subject: [PATCH] rewrite: don't resolve intermediate parent tree when rebasing Let's say we're updating one parent of a merge: ``` E E' /|\ /|\ B C D -> B C D' \|/ \|/ A A ``` When rebasing `E` to create `E'` there, we do that by merging the changes compared to the auto-merged parents. The auto-merged parents before is `B+(C-A)+(D-A)`, and after it's `B+(C-A)+(D'-A)`. Then we rebase the diff, which gives us `E' = B+(C-A)+(D'-A) + (E - (B+(C-A)+(D-A))) = D' + (E - D')`. However, we currently don't do quite that simplification because we first resolve conflicts when possible in the two auto-merged parent trees (before and after). That rarely makes a difference to the result, but it's wasteful to do it. It does make a difference in some cases where our merge algorithm is lossy, which currently is only the "A+(A-B)=A" case. I added a test case showing where it does make a difference. It's a non-obvious cases but I think the new behavior is more correct (the old behavior was a conflict). --- lib/src/rewrite.rs | 12 +++++-- lib/tests/test_merge_trees.rs | 67 +++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index cd8a5d6b17..9e5f56f616 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -249,8 +249,16 @@ impl<'repo> CommitRewriter<'repo> { self.old_commit.tree_id().clone(), ) } else { - let old_base_tree = merge_commit_trees(self.mut_repo, &old_parents)?; - let new_base_tree = merge_commit_trees(self.mut_repo, &new_parents)?; + let old_base_tree = merge_commit_trees_no_resolve_without_repo( + self.mut_repo.store(), + self.mut_repo.index(), + &old_parents, + )?; + let new_base_tree = merge_commit_trees_no_resolve_without_repo( + self.mut_repo.store(), + self.mut_repo.index(), + &new_parents, + )?; let old_tree = self.old_commit.tree()?; ( old_base_tree.id() == *self.old_commit.tree_id(), diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index 79dedf68da..efea162bd7 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -645,3 +645,70 @@ fn test_simplify_conflict_after_resolving_parent() { // TODO: Add tests for simplification of multi-way conflicts. Both the content // and the executable bit need testing. + +#[test] +fn test_rebase_on_lossy_merge() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Test this rebase: + // D foo=2 D' foo=3 + // |\ |\ + // | C foo=2 | C' foo=3 + // | | => | | + // B | foo=2 B | foo=2 + // |/ |/ + // A foo=1 A foo=1 + // + // Commit D effectively discarded a change from "1" to "2", so one + // reasonable result in D' is "3". That's what the result would be if we + // didn't have the "A+(A-B)=A" rule. It's also what the result currently + // is because we don't attempt to resolve the auto-merged parents (if we + // had, it would have been resolved to just "2" before the rebase and we + // get a conflict after the rebase). + let path = RepoPath::from_internal_string("foo"); + let mut tx = repo.start_transaction(&settings); + let repo_mut = tx.repo_mut(); + let tree_1 = create_tree(repo, &[(path, "1")]); + let tree_2 = create_tree(repo, &[(path, "2")]); + let tree_3 = create_tree(repo, &[(path, "3")]); + let commit_a = repo_mut + .new_commit( + &settings, + vec![repo.store().root_commit_id().clone()], + tree_1.id(), + ) + .write() + .unwrap(); + let commit_b = repo_mut + .new_commit(&settings, vec![commit_a.id().clone()], tree_2.id()) + .write() + .unwrap(); + let commit_c = repo_mut + .new_commit(&settings, vec![commit_a.id().clone()], tree_2.id()) + .write() + .unwrap(); + let commit_d = repo_mut + .new_commit( + &settings, + vec![commit_b.id().clone(), commit_c.id().clone()], + tree_2.id(), + ) + .write() + .unwrap(); + + let commit_c2 = repo_mut + .new_commit(&settings, vec![commit_a.id().clone()], tree_3.id()) + .write() + .unwrap(); + let commit_d2 = rebase_commit( + &settings, + repo_mut, + commit_d, + vec![commit_b.id().clone(), commit_c2.id().clone()], + ) + .unwrap(); + + assert_eq!(*commit_d2.tree_id(), tree_3.id()); +}