From e384f97b20aab1688405fcb8be770bf357e540d2 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 20 Mar 2022 22:16:53 -0700 Subject: [PATCH] transaction: check that we haven't forgotten to rebase descendants (#111) If we have recorded in `MutableRepo` that commits have been abandoned or rewritten, we should always rebase descendants before committing the transaction (otherwise there's no reason to record the rewrites). That's not much of a risk in the CLI because we already have that logic in a central place there (`finish_transaction()`), but other users of the library crate could easily miss it. Perhaps we should automatically do any necessary rebasing we commit the transaction in the library crate instead, but for now let's just have a check for that to catch such bugs. --- lib/src/repo.rs | 6 +++++- lib/src/transaction.rs | 5 +++++ lib/tests/test_commit_builder.rs | 1 + lib/tests/test_merge_trees.rs | 1 + lib/tests/test_operations.rs | 13 ++++++------- 5 files changed, 18 insertions(+), 8 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 14cfb6fcb..fabf34a62 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -518,6 +518,10 @@ impl MutableRepo { self.abandoned_commits.clear(); } + pub fn has_rewrites(&self) -> bool { + !(self.rewritten_commits.is_empty() && self.abandoned_commits.is_empty()) + } + /// Creates a `DescendantRebaser` to rebase descendants of the recorded /// rewritten and abandoned commits. pub fn create_descendant_rebaser<'settings, 'repo>( @@ -533,7 +537,7 @@ impl MutableRepo { } pub fn rebase_descendants(&mut self, settings: &UserSettings) -> usize { - if self.rewritten_commits.is_empty() && self.abandoned_commits.is_empty() { + if !self.has_rewrites() { // Optimization return 0; } diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index b1abec09b..4e9cd48b7 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -69,6 +69,11 @@ impl Transaction { /// operation will not be seen when loading the repo at head. pub fn write(mut self) -> UnpublishedOperation { let mut_repo = self.repo.take().unwrap(); + // TODO: Should we instead just do the rebasing here if necessary? + assert!( + !mut_repo.has_rewrites(), + "BUG: Descendants have not been rebased after the last rewrites." + ); let base_repo = mut_repo.base_repo().clone(); let (mut_index, view) = mut_repo.consume(); let index = base_repo.index_store().write_index(mut_index).unwrap(); diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index f3d8e7790..e5f4cfff4 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -112,6 +112,7 @@ fn test_rewrite(use_git: bool) { CommitBuilder::for_rewrite_from(&rewrite_settings, &store, &initial_commit) .set_tree(rewritten_tree.id().clone()) .write_to_repo(tx.mut_repo()); + tx.mut_repo().rebase_descendants(&settings); tx.commit(); assert_eq!(rewritten_commit.parents(), vec![store.root_commit()]); assert_eq!( diff --git a/lib/tests/test_merge_trees.rs b/lib/tests/test_merge_trees.rs index af281ee30..02bb6f125 100644 --- a/lib/tests/test_merge_trees.rs +++ b/lib/tests/test_merge_trees.rs @@ -620,6 +620,7 @@ fn test_simplify_conflict_after_resolving_parent(use_git: bool) { .set_tree(tree_b3.id().clone()) .write_to_repo(tx.mut_repo()); let commit_c3 = rebase_commit(&settings, tx.mut_repo(), &commit_c2, &[commit_b3]); + tx.mut_repo().rebase_descendants(&settings); let repo = tx.commit(); // The conflict should now be resolved. diff --git a/lib/tests/test_operations.rs b/lib/tests/test_operations.rs index 5f68001ce..51a491d9b 100644 --- a/lib/tests/test_operations.rs +++ b/lib/tests/test_operations.rs @@ -154,28 +154,27 @@ fn test_isolation(use_git: bool) { let rewrite1 = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial) .set_description("rewrite1".to_string()) .write_to_repo(mut_repo1); + mut_repo1.rebase_descendants(&settings); let rewrite2 = CommitBuilder::for_rewrite_from(&settings, repo.store(), &initial) .set_description("rewrite2".to_string()) .write_to_repo(mut_repo2); + mut_repo2.rebase_descendants(&settings); // Neither transaction has committed yet, so each transaction sees its own // commit. assert_heads(repo.as_repo_ref(), vec![initial.id()]); - assert_heads(mut_repo1.as_repo_ref(), vec![initial.id(), rewrite1.id()]); - assert_heads(mut_repo2.as_repo_ref(), vec![initial.id(), rewrite2.id()]); + assert_heads(mut_repo1.as_repo_ref(), vec![rewrite1.id()]); + assert_heads(mut_repo2.as_repo_ref(), vec![rewrite2.id()]); // The base repo and tx2 don't see the commits from tx1. tx1.commit(); assert_heads(repo.as_repo_ref(), vec![initial.id()]); - assert_heads(mut_repo2.as_repo_ref(), vec![initial.id(), rewrite2.id()]); + assert_heads(mut_repo2.as_repo_ref(), vec![rewrite2.id()]); // The base repo still doesn't see the commits after both transactions commit. tx2.commit(); assert_heads(repo.as_repo_ref(), vec![initial.id()]); // After reload, the base repo sees both rewrites. let repo = repo.reload(); - assert_heads( - repo.as_repo_ref(), - vec![initial.id(), rewrite1.id(), rewrite2.id()], - ); + assert_heads(repo.as_repo_ref(), vec![rewrite1.id(), rewrite2.id()]); }