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

@@ -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;
         }

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();

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!(

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.

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()]);
 }