From abc77e361bab5ba1c58b9311c96d2f03d712210b Mon Sep 17 00:00:00 2001
From: Ilya Grigoriev <ilyagr@users.noreply.github.com>
Date: Sun, 17 Dec 2023 21:28:14 -0800
Subject: [PATCH] `split.rs`: stop using DescendantRebaser::new

This requires creating a new public API as a substitute. I made the simplest
possible change and not a very elegant one. Eventually, the entire
`record_rewritten_commit` API should probably be refactored again.
---
 cli/src/commands/split.rs | 19 +++++++++----------
 lib/src/repo.rs           | 40 +++++++++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 20 deletions(-)

diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs
index 7fe3914521..21e78257f5 100644
--- a/cli/src/commands/split.rs
+++ b/cli/src/commands/split.rs
@@ -15,8 +15,7 @@ use std::io::Write;
 
 use jj_lib::backend::ObjectId;
 use jj_lib::repo::Repo;
-use jj_lib::rewrite::{merge_commit_trees, DescendantRebaser};
-use maplit::{hashmap, hashset};
+use jj_lib::rewrite::merge_commit_trees;
 use tracing::instrument;
 
 use crate::cli_util::{CommandError, CommandHelper, RevisionArg};
@@ -135,14 +134,14 @@ don't make any changes, then the operation will be aborted.
         .generate_new_change_id()
         .set_description(second_description)
         .write()?;
-    let mut rebaser = DescendantRebaser::new(
-        command.settings(),
-        tx.mut_repo(),
-        hashmap! { commit.id().clone() => hashset!{second_commit.id().clone()} },
-        hashset! {},
-    );
-    rebaser.rebase_all()?;
-    let num_rebased = rebaser.rebased().len();
+
+    // Currently, `rebase_descendents` would treat `commit` as being rewritten to
+    // *both* `first_commit` and `second_commit`, as if it was becoming divergent.
+    // However, we want only the `second_commit` to inherit `commit`'s branches and
+    // descendants.
+    tx.mut_repo()
+        .set_rewritten_commit(commit.id().clone(), [second_commit.id().clone()]);
+    let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?;
     if num_rebased > 0 {
         writeln!(ui.stderr(), "Rebased {num_rebased} descendant commits")?;
     }
diff --git a/lib/src/repo.rs b/lib/src/repo.rs
index 2954d5a352..bfca78c26b 100644
--- a/lib/src/repo.rs
+++ b/lib/src/repo.rs
@@ -807,12 +807,30 @@ impl MutableRepo {
         Ok(commit)
     }
 
-    /// Record a commit as having been rewritten in this transaction. This
-    /// record is used by `rebase_descendants()`.
+    /// Record a commit as having been rewritten to one or more ids in this
+    /// transaction.
     ///
-    /// Rewritten commits don't have to be recorded here. This is just a
-    /// convenient place to record it. It won't matter after the transaction
-    /// has been committed.
+    /// This record is used by `rebase_descendants` to know which commits have
+    /// children that need to be rebased, and where to rebase them to.
+    //  TODO(ilyagr): Make this API saner, e.g. make `self.rewritten_commits` public
+    // and make empty values correspond to abandoned commits.
+    pub fn set_rewritten_commit(
+        &mut self,
+        old_id: CommitId,
+        new_ids: impl IntoIterator<Item = CommitId>,
+    ) -> Option<HashSet<CommitId>> {
+        assert_ne!(old_id, *self.store().root_commit_id());
+        self.rewritten_commits
+            .insert(old_id, new_ids.into_iter().collect())
+    }
+
+    /// Record a commit as having been rewritten in this transaction. If it was
+    /// already rewritten, mark it as divergent (unlike
+    /// `set_rewritten_commit`)
+    ///
+    /// This record is used by `rebase_descendants` to know which commits have
+    /// children that need to be rebased, and where to rebase the children (as
+    /// well as branches) to.
     pub fn record_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) {
         assert_ne!(old_id, *self.store().root_commit_id());
         self.rewritten_commits
@@ -825,12 +843,11 @@ impl MutableRepo {
         self.rewritten_commits.clear();
     }
 
-    /// Record a commit as having been abandoned in this transaction. This
-    /// record is used by `rebase_descendants()`.
+    /// Record a commit as having been abandoned in this transaction.
     ///
-    /// Abandoned commits don't have to be recorded here. This is just a
-    /// convenient place to record it. It won't matter after the transaction
-    /// has been committed.
+    /// This record is used by `rebase_descendants` to know which commits have
+    /// children that need to be rebased, and where to rebase the children (as
+    /// well as branches) to.
     pub fn record_abandoned_commit(&mut self, old_id: CommitId) {
         assert_ne!(old_id, *self.store().root_commit_id());
         self.abandoned_commits.insert(old_id);
@@ -874,6 +891,9 @@ impl MutableRepo {
         Ok(Some(rebaser))
     }
 
+    // TODO(ilyagr): Either document that this also moves branches (rename the
+    // function and the related functions?) or change things so that this only
+    // rebases descendants.
     pub fn rebase_descendants_with_options(
         &mut self,
         settings: &UserSettings,