test_rewrite.rs: stop using DescendantRebaser when testing EmptyBehavior

This completes the process of removing DescendantRebaser-related APIs from
tests. It requires creating some new test utils and a new
`rebase_descendants_with_option_return_map`.

lib/src/repo.rs

@@ -925,24 +925,45 @@ impl MutableRepo {
         Ok(result)
     }
 
-    pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> {
-        self.rebase_descendants_with_options(settings, Default::default())
-    }
-
-    pub fn rebase_descendants_return_map(
+    /// This is similar to `rebase_descendants_return_map`, but the return value
+    /// needs more explaining.
+    ///
+    /// If the `options.empty` is the default, this function will only
+    /// rebase commits, and the return value is what you'd expect it to be.
+    ///
+    /// Otherwise, this function may rebase some commits and abandon others. The
+    /// behavior is such that only commits with a single parent will ever be
+    /// abandoned. In the returned map, an abandoned commit will look as a
+    /// key-value pair where the key is the abandoned commit and the value is
+    /// **its parent**. One can tell this case apart since the change ids of the
+    /// key and the value will not match. The parent will inherit the
+    /// descendants and the branches of the abandoned commit.
+    pub fn rebase_descendants_with_options_return_map(
         &mut self,
         settings: &UserSettings,
+        options: RebaseOptions,
     ) -> Result<HashMap<CommitId, CommitId>, TreeMergeError> {
         let result = Ok(self
             // We do not set RebaseOptions here, since this function does not currently return
             // enough information to describe the results of a rebase if some commits got
             // abandoned
-            .rebase_descendants_return_rebaser(settings, Default::default())?
+            .rebase_descendants_return_rebaser(settings, options)?
             .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone()));
         self.clear_descendant_rebaser_plans();
         result
     }
 
+    pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> {
+        self.rebase_descendants_with_options(settings, Default::default())
+    }
+
+    pub fn rebase_descendants_return_map(
+        &mut self,
+        settings: &UserSettings,
+    ) -> Result<HashMap<CommitId, CommitId>, TreeMergeError> {
+        self.rebase_descendants_with_options_return_map(settings, Default::default())
+    }
+
     pub fn set_wc_commit(
         &mut self,
         workspace_id: WorkspaceId,

lib/tests/test_rewrite.rs

@@ -19,14 +19,12 @@ use jj_lib::merged_tree::MergedTree;
 use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId};
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::rewrite::{
-    restore_tree, DescendantRebaser, EmptyBehaviour, RebaseOptions, RebasedDescendant,
-};
+use jj_lib::rewrite::{restore_tree, EmptyBehaviour, RebaseOptions};
 use maplit::{hashmap, hashset};
 use test_case::test_case;
 use testutils::{
-    assert_rebased, assert_rebased_onto, create_random_commit, create_tree, write_random_commit,
-    CommitGraphBuilder, TestRepo,
+    assert_abandoned_with_parent, assert_rebased_onto, create_random_commit, create_tree,
+    write_random_commit, CommitGraphBuilder, TestRepo,
 };
 
 #[test]
@@ -1474,30 +1472,10 @@ fn test_rebase_descendants_update_checkout_abandoned_merge() {
     assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]);
 }
 
-fn assert_rebase_skipped(
-    rebased: Option<RebasedDescendant>,
-    expected_old_commit: &Commit,
-    expected_new_commit: &Commit,
-) -> Commit {
-    if let Some(RebasedDescendant {
-        old_commit,
-        new_commit,
-    }) = rebased
-    {
-        assert_eq!(old_commit, *expected_old_commit,);
-        assert_eq!(new_commit, *expected_new_commit);
-        // Since it was abandoned, the change ID should be different.
-        assert_ne!(old_commit.change_id(), new_commit.change_id());
-        new_commit
-    } else {
-        panic!("expected rebased commit: {rebased:?}");
-    }
-}
-
 #[test_case(EmptyBehaviour::Keep; "keep all commits")]
 #[test_case(EmptyBehaviour::AbandonNewlyEmpty; "abandon newly empty commits")]
 #[test_case(EmptyBehaviour::AbandonAllEmpty ; "abandon all empty commits")]
-fn test_empty_commit_option(empty: EmptyBehaviour) {
+fn test_empty_commit_option(empty_behavior: EmptyBehaviour) {
     let settings = testutils::user_settings();
     let test_repo = TestRepo::init();
     let repo = &test_repo.repo;
@@ -1557,74 +1535,75 @@ fn test_empty_commit_option(empty: EmptyBehaviour) {
     let commit_h = create_commit(&[&commit_g], &tree_g);
     let commit_bd = create_commit(&[&commit_a], &tree_d);
 
-    let mut rebaser = DescendantRebaser::new(
-        &settings,
-        tx.mut_repo(),
-        hashmap! {
-            commit_b.id().clone() => hashset!{commit_bd.id().clone()}
-        },
-        hashset! {},
-    );
-    *rebaser.mut_options() = RebaseOptions {
-        empty: empty.clone(),
-    };
+    tx.mut_repo()
+        .record_rewritten_commit(commit_b.id().clone(), commit_bd.id().clone());
+    let rebase_map = tx
+        .mut_repo()
+        .rebase_descendants_with_options_return_map(
+            &settings,
+            RebaseOptions {
+                empty: empty_behavior.clone(),
+            },
+        )
+        .unwrap();
 
-    let new_head = match empty {
+    let new_head = match empty_behavior {
         EmptyBehaviour::Keep => {
             // The commit C isn't empty.
             let new_commit_c =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]);
             let new_commit_d =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_bd]);
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[commit_bd.id()]);
             let new_commit_e =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]);
-            let new_commit_f = assert_rebased(
-                rebaser.rebase_next().unwrap(),
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[commit_bd.id()]);
+            let new_commit_f = assert_rebased_onto(
+                tx.mut_repo(),
+                &rebase_map,
                 &commit_f,
-                &[&new_commit_c, &new_commit_d, &new_commit_e],
+                &[&new_commit_c.id(), &new_commit_d.id(), &new_commit_e.id()],
             );
             let new_commit_g =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]);
-            assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g])
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[new_commit_f.id()]);
+            assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_h, &[new_commit_g.id()])
         }
         EmptyBehaviour::AbandonAllEmpty => {
             // The commit C isn't empty.
             let new_commit_c =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]);
             // D and E are empty, and F is a clean merge with only one child. Thus, F is
             // also considered empty.
-            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
-            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd);
-            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c);
+            assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, commit_bd.id());
+            assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, commit_bd.id());
+            assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_f, new_commit_c.id());
             let new_commit_g =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c]);
-            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_h, &new_commit_g)
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[new_commit_c.id()]);
+            assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_h, new_commit_g.id())
         }
         EmptyBehaviour::AbandonNewlyEmpty => {
             // The commit C isn't empty.
             let new_commit_c =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_bd.id()]);
 
             // The changes in D are included in BD, so D is newly empty.
-            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
+            assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, commit_bd.id());
             // E was already empty, so F is a merge commit with C and E as parents.
             // Although it's empty, we still keep it because we don't want to drop merge
             // commits.
             let new_commit_e =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]);
-            let new_commit_f = assert_rebased(
-                rebaser.rebase_next().unwrap(),
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[commit_bd.id()]);
+            let new_commit_f = assert_rebased_onto(
+                tx.mut_repo(),
+                &rebase_map,
                 &commit_f,
-                &[&new_commit_c, &new_commit_e],
+                &[&new_commit_c.id(), &new_commit_e.id()],
             );
             let new_commit_g =
-                assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f]);
-            assert_rebased(rebaser.rebase_next().unwrap(), &commit_h, &[&new_commit_g])
+                assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_g, &[&new_commit_f.id()]);
+            assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_h, &[&new_commit_g.id()])
         }
     };
 
-    assert!(rebaser.rebase_next().unwrap().is_none());
-    assert_eq!(rebaser.rebased().len(), 6);
+    assert_eq!(rebase_map.len(), 6);
 
     assert_eq!(
         *tx.mut_repo().view().heads(),

lib/testutils/src/lib.rs

@@ -31,7 +31,6 @@ use jj_lib::local_backend::LocalBackend;
 use jj_lib::merged_tree::MergedTree;
 use jj_lib::repo::{MutableRepo, ReadonlyRepo, Repo, RepoLoader, StoreFactories};
 use jj_lib::repo_path::{RepoPath, RepoPathBuf};
-use jj_lib::rewrite::RebasedDescendant;
 use jj_lib::settings::UserSettings;
 use jj_lib::signing::Signer;
 use jj_lib::store::Store;
@@ -443,11 +442,10 @@ impl<'settings, 'repo> CommitGraphBuilder<'settings, 'repo> {
     }
 }
 
-pub fn assert_rebased_onto(
+fn assert_in_rebased_map(
     repo: &impl Repo,
     rebased: &HashMap<CommitId, CommitId>,
     expected_old_commit: &Commit,
-    expected_new_parent_ids: &[&CommitId],
 ) -> Commit {
     let new_commit_id = rebased.get(expected_old_commit.id()).unwrap_or_else(|| {
         panic!(
@@ -456,40 +454,43 @@ pub fn assert_rebased_onto(
         )
     });
     let new_commit = repo.store().get_commit(new_commit_id).unwrap().clone();
-    assert_eq!(new_commit.change_id(), expected_old_commit.change_id());
+    new_commit
+}
+
+pub fn assert_rebased_onto(
+    repo: &impl Repo,
+    rebased: &HashMap<CommitId, CommitId>,
+    expected_old_commit: &Commit,
+    expected_new_parent_ids: &[&CommitId],
+) -> Commit {
+    let new_commit = assert_in_rebased_map(repo, rebased, expected_old_commit);
     assert_eq!(
         new_commit.parent_ids().to_vec(),
         expected_new_parent_ids
             .iter()
             .map(|x| (*x).clone())
             .collect_vec()
     );
+    assert_eq!(new_commit.change_id(), expected_old_commit.change_id());
     new_commit
 }
 
-// Short-term TODO: We will delete this function shortly; it will be replaced by
-// `assert_rebased_onto` above
-pub fn assert_rebased(
-    rebased: Option<RebasedDescendant>,
+/// If `expected_old_commit` was abandoned, the `rebased` map indicates the
+/// commit the children of `expected_old_commit` should be rebased to, which
+/// would have a different change id. This happens when the EmptyBehavior in
+/// RebaseOptions is not the default; because of the details of the
+/// implementation this returned parent commit is always singular.
+pub fn assert_abandoned_with_parent(
+    repo: &impl Repo,
+    rebased: &HashMap<CommitId, CommitId>,
     expected_old_commit: &Commit,
-    expected_new_parents: &[&Commit],
+    expected_new_parent_id: &CommitId,
 ) -> Commit {
-    if let Some(RebasedDescendant {
-        old_commit,
-        new_commit,
-    }) = rebased
-    {
-        assert_eq!(old_commit, *expected_old_commit);
-        assert_eq!(new_commit.change_id(), expected_old_commit.change_id());
-        assert_eq!(
-            new_commit.parent_ids(),
-            expected_new_parents
-                .iter()
-                .map(|commit| commit.id().clone())
-                .collect_vec()
-        );
-        new_commit
-    } else {
-        panic!("expected rebased commit: {rebased:?}");
-    }
+    let new_parent_commit = assert_in_rebased_map(repo, rebased, expected_old_commit);
+    assert_eq!(new_parent_commit.id(), expected_new_parent_id);
+    assert_ne!(
+        new_parent_commit.change_id(),
+        expected_old_commit.change_id()
+    );
+    new_parent_commit
 }