Refactor MutRepo to make DescendantRebaser private (pt 1)

cli/src/commands/move.rs

@@ -113,9 +113,10 @@ from the source will be moved into the destination.
         // rewritten source. Otherwise it will likely already have the content
         // changes we're moving, so applying them will have no effect and the
         // changes will disappear.
-        let mut rebaser = tx.mut_repo().create_descendant_rebaser(command.settings());
-        rebaser.rebase_all()?;
-        let rebased_destination_id = rebaser.rebased().get(destination.id()).unwrap().clone();
+        let rebase_map = tx
+            .mut_repo()
+            .rebase_descendants_return_map(command.settings())?;
+        let rebased_destination_id = rebase_map.get(destination.id()).unwrap().clone();
         destination = tx.mut_repo().store().get_commit(&rebased_destination_id)?;
     }
     // Apply the selected changes onto the destination

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")?;
     }

cli/tests/test_split_command.rs

@@ -32,6 +32,10 @@ fn test_split_by_paths() {
     @  qpvuntsmwlqt false
     ◉  zzzzzzzzzzzz true
     "###);
+    insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@"), @r###"
+    Author date:  2001-02-03 04:05:07.000 +07:00
+    Committer date: 2001-02-03 04:05:08.000 +07:00
+    "###);
 
     let edit_script = test_env.set_up_fake_editor();
     std::fs::write(
@@ -42,10 +46,10 @@ fn test_split_by_paths() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["split", "file2"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    First part: qpvuntsm 5eebce1d (no description set)
-    Second part: kkmpptxz 45833353 (no description set)
-    Working copy now at: kkmpptxz 45833353 (no description set)
-    Parent commit      : qpvuntsm 5eebce1d (no description set)
+    First part: qpvuntsm d62c056f (no description set)
+    Second part: zsuskuln 5a32af4a (no description set)
+    Working copy now at: zsuskuln 5a32af4a (no description set)
+    Parent commit      : qpvuntsm d62c056f (no description set)
     "###);
     insta::assert_snapshot!(
         std::fs::read_to_string(test_env.env_root().join("editor0")).unwrap(), @r###"
@@ -59,11 +63,22 @@ fn test_split_by_paths() {
     assert!(!test_env.env_root().join("editor1").exists());
 
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  kkmpptxzrspx false
+    @  zsuskulnrvyr false
     ◉  qpvuntsmwlqt false
     ◉  zzzzzzzzzzzz true
     "###);
 
+    // The author dates of the new commits should be inherited from the commit being
+    // split. The committer dates should be newer.
+    insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@"), @r###"
+    Author date:  2001-02-03 04:05:07.000 +07:00
+    Committer date: 2001-02-03 04:05:10.000 +07:00
+    "###);
+    insta::assert_snapshot!(get_recorded_dates(&test_env, &repo_path,"@-"), @r###"
+    Author date:  2001-02-03 04:05:07.000 +07:00
+    Committer date: 2001-02-03 04:05:10.000 +07:00
+    "###);
+
     let stdout = test_env.jj_cmd_success(&repo_path, &["diff", "-s", "-r", "@-"]);
     insta::assert_snapshot!(stdout, @r###"
     A file2
@@ -80,15 +95,15 @@ fn test_split_by_paths() {
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
     Rebased 1 descendant commits
-    First part: qpvuntsm 31425b56 (no description set)
-    Second part: yqosqzyt af096392 (empty) (no description set)
-    Working copy now at: kkmpptxz 28d4ec20 (no description set)
-    Parent commit      : yqosqzyt af096392 (empty) (no description set)
+    First part: qpvuntsm b76d731d (no description set)
+    Second part: znkkpsqq 924604b2 (empty) (no description set)
+    Working copy now at: zsuskuln fffe30fb (no description set)
+    Parent commit      : znkkpsqq 924604b2 (empty) (no description set)
     "###);
 
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  kkmpptxzrspx false
-    ◉  yqosqzytrlsw true
+    @  zsuskulnrvyr false
+    ◉  znkkpsqqskkl true
     ◉  qpvuntsmwlqt false
     ◉  zzzzzzzzzzzz true
     "###);
@@ -108,15 +123,15 @@ fn test_split_by_paths() {
     insta::assert_snapshot!(stderr, @r###"
     The given paths do not match any file: nonexistent
     Rebased 1 descendant commits
-    First part: qpvuntsm 0647b2cb (empty) (no description set)
-    Second part: kpqxywon d5d77af6 (no description set)
-    Working copy now at: kkmpptxz 86f228dc (no description set)
-    Parent commit      : kpqxywon d5d77af6 (no description set)
+    First part: qpvuntsm 7086b0bc (empty) (no description set)
+    Second part: lylxulpl 2252ed18 (no description set)
+    Working copy now at: zsuskuln a3f2136a (no description set)
+    Parent commit      : lylxulpl 2252ed18 (no description set)
     "###);
 
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    @  kkmpptxzrspx false
-    ◉  kpqxywonksrl false
+    @  zsuskulnrvyr false
+    ◉  lylxulplsnyw false
     ◉  qpvuntsmwlqt true
     ◉  zzzzzzzzzzzz true
     "###);
@@ -222,3 +237,8 @@ fn get_log_output(test_env: &TestEnvironment, cwd: &Path) -> String {
     let template = r#"separate(" ", change_id.short(), empty, description)"#;
     test_env.jj_cmd_success(cwd, &["log", "-T", template])
 }
+
+fn get_recorded_dates(test_env: &TestEnvironment, cwd: &Path, revset: &str) -> String {
+    let template = r#"separate("\n", "Author date:  " ++ author.timestamp(), "Committer date: " ++ committer.timestamp())"#;
+    test_env.jj_cmd_success(cwd, &["log", "--no-graph", "-T", template, "-r", revset])
+}

lib/src/commit_builder.rs

@@ -32,7 +32,8 @@ pub struct CommitBuilder<'repo> {
 }
 
 impl CommitBuilder<'_> {
-    pub fn for_new_commit<'repo>(
+    // Use MutRepo::new_commit() instead
+    pub(crate) fn for_new_commit<'repo>(
         mut_repo: &'repo mut MutableRepo,
         settings: &UserSettings,
         parents: Vec<CommitId>,
@@ -61,7 +62,8 @@ impl CommitBuilder<'_> {
         }
     }
 
-    pub fn for_rewrite_from<'repo>(
+    // Use MutRepo::rewrite_commit() instead
+    pub(crate) fn for_rewrite_from<'repo>(
         mut_repo: &'repo mut MutableRepo,
         settings: &UserSettings,
         predecessor: &Commit,

lib/src/repo.rs

@@ -796,6 +796,8 @@ impl MutableRepo {
         predecessor: &Commit,
     ) -> CommitBuilder {
         CommitBuilder::for_rewrite_from(self, settings, predecessor)
+        // CommitBuilder::write will record the rewrite in
+        // `self.rewritten_commits`
     }
 
     pub fn write_commit(
@@ -808,12 +810,43 @@ 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. See the
+    /// docstring for `record_rewritten_commit` for details.
+    //  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>,
+    ) {
+        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.
+    ///
+    /// The `rebase_descendants` logic treats these records as follows:
+    ///
+    /// - If a commit is recorded as rewritten to a single commit, its
+    ///   descendants would be rebased to become descendants of `new_id`. Any
+    ///   branches at `old_id` are also moved to `new_id`.
+    /// - If a commit is recorded as rewritten to more than one commit, it is
+    ///   assumed to have become divergent. Its descendants are *not* rebased.
+    ///   However, the *branches* that were at `old_id` are moved to each of the
+    ///   new ids, and thus become conflicted.
+    ///
+    /// In neither case would `rebase_descendants` modify the `old_id` commit
+    /// itself.
     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
@@ -826,12 +859,15 @@ 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.
+    ///
+    /// 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.
     ///
-    /// 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.
+    /// The `rebase_descendants` logic will rebase the descendants of `old_id`
+    /// to become the descendants of parent(s) of `old_id`. Any branches at
+    /// `old_id` would be moved to the parent(s) of `old_id` as well.
     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);
@@ -847,6 +883,7 @@ impl MutableRepo {
 
     /// Creates a `DescendantRebaser` to rebase descendants of the recorded
     /// rewritten and abandoned commits.
+    // TODO(ilyagr): Inline this. It's only used in tests.
     pub fn create_descendant_rebaser<'settings, 'repo>(
         &'repo mut self,
         settings: &'settings UserSettings,
@@ -874,6 +911,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,

lib/tests/test_rewrite.rs

@@ -1119,6 +1119,7 @@ fn test_rebase_descendants_update_branches_after_divergent_rewrite() {
     // B main         |/B2 main?
     // |         =>   |/
     // A              A
+    // TODO(ilyagr): Check what happens if B had a descendant with a branch on it.
     let mut tx = repo.start_transaction(&settings);
     let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo());
     let commit_a = graph_builder.initial_commit();