rewrite: pass just IDs of new parents into rewrite::rebase*()

It's cheap to look up commits again from the cache in `Store` but it
can be expensive to look up commits we didn't end up needing. This
will make it easier to refactor further and be able to cheaply set
preliminary parents for a rewritten commits and then let the caller
update them.

cli/src/commands/new.rs

@@ -140,7 +140,7 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
                 command.settings(),
                 tx.mut_repo(),
                 child_commit,
-                vec![new_commit.clone()],
+                vec![new_commit.id().clone()],
             )?;
         }
     } else {
@@ -170,17 +170,16 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.",
         for child_commit in commits_to_rebase {
             let commit_parents = RevsetExpression::commits(child_commit.parent_ids().to_owned());
             let new_parents = commit_parents.minus(&old_parents);
-            let mut new_parent_commits: Vec<Commit> = new_parents
+            let mut new_parent_ids = new_parents
                 .evaluate_programmatic(tx.base_repo().as_ref())?
                 .iter()
-                .commits(tx.base_repo().store())
-                .try_collect()?;
-            new_parent_commits.push(new_commit.clone());
+                .collect_vec();
+            new_parent_ids.push(new_commit.id().clone());
             rebase_commit(
                 command.settings(),
                 tx.mut_repo(),
                 child_commit,
-                new_parent_commits,
+                new_parent_ids,
             )?;
         }
     }

cli/src/commands/rebase.rs

@@ -296,7 +296,10 @@ pub fn rebase_descendants(
             settings,
             tx.mut_repo(),
             old_commit.borrow().clone(),
-            new_parents.to_vec(),
+            new_parents
+                .iter()
+                .map(|parent| parent.id().clone())
+                .collect(),
             &rebase_options,
         )?;
     }
@@ -402,12 +405,11 @@ fn rebase_revision(
                     .parents()
                     .ancestors(),
             );
-        let new_child_parents: Vec<Commit> = new_child_parents_expression
+        let new_child_parents = new_child_parents_expression
             .evaluate_programmatic(tx.base_repo().as_ref())
             .unwrap()
             .iter()
-            .commits(tx.base_repo().store())
-            .try_collect()?;
+            .collect_vec();
 
         rebased_commit_ids.insert(
             child_commit.id().clone(),
@@ -447,23 +449,22 @@ fn rebase_revision(
     // above, and then the result would be rebased again by
     // `rebase_descendants_return_map`. Then, if we were trying to rebase
     // `old_commit` onto `Q`, new_parents would only account for one of these.
-    let new_parents: Vec<_> = new_parents
+    let new_parents = new_parents
         .iter()
         .map(|new_parent| {
             rebased_commit_ids
                 .get(new_parent.id())
-                .map_or(Ok(new_parent.clone()), |rebased_new_parent_id| {
-                    tx.repo().store().get_commit(rebased_new_parent_id)
-                })
+                .unwrap_or(new_parent.id())
+                .clone()
         })
-        .try_collect()?;
+        .collect();
 
     let tx_description = format!("rebase commit {}", old_commit.id().hex());
     // Finally, it's safe to rebase `old_commit`. We can skip rebasing if it is
     // already a child of `new_parents`. Otherwise, at this point, it should no
     // longer have any children; they have all been rebased and the originals
     // have been abandoned.
-    let skipped_commit_rebase = if old_commit.parents() == new_parents {
+    let skipped_commit_rebase = if old_commit.parent_ids() == new_parents {
         if let Some(mut formatter) = ui.status_formatter() {
             write!(formatter, "Skipping rebase of commit ")?;
             tx.write_commit_summary(formatter.as_mut(), &old_commit)?;

lib/src/repo.rs

@@ -1130,7 +1130,7 @@ impl MutableRepo {
         // Calculate an order where we rebase parents first, but if the parents were
         // rewritten, make sure we rebase the rewritten parent first.
         dag_walk::topo_order_reverse_ok(
-            to_visit.into_iter().map(|commit| Ok(commit)),
+            to_visit.into_iter().map(Ok),
             |commit| commit.id().clone(),
             |commit| -> Vec<BackendResult<Commit>> {
                 visited.insert(commit.id().clone());

lib/src/rewrite.rs

@@ -96,7 +96,7 @@ pub fn rebase_commit(
     settings: &UserSettings,
     mut_repo: &mut MutableRepo,
     old_commit: Commit,
-    new_parents: Vec<Commit>,
+    new_parents: Vec<CommitId>,
 ) -> BackendResult<Commit> {
     let rebased_commit = rebase_commit_with_options(
         settings,
@@ -120,27 +120,22 @@ pub fn rebase_commit_with_options(
     settings: &UserSettings,
     mut_repo: &mut MutableRepo,
     old_commit: Commit,
-    new_parents: Vec<Commit>,
+    mut new_parent_ids: Vec<CommitId>,
     options: &RebaseOptions,
 ) -> BackendResult<RebasedCommit> {
     // If specified, don't create commit where one parent is an ancestor of another.
-    let simplified_new_parents;
-    let new_parents = if options.simplify_ancestor_merge {
-        let mut new_parent_ids = new_parents.iter().map(|commit| commit.id());
+    if options.simplify_ancestor_merge {
         let head_set: HashSet<_> = mut_repo
             .index()
-            .heads(&mut new_parent_ids)
+            .heads(&mut new_parent_ids.iter())
             .into_iter()
             .collect();
-        simplified_new_parents = new_parents
-            .iter()
-            .filter(|commit| head_set.contains(commit.id()))
-            .cloned()
-            .collect_vec();
-        &simplified_new_parents[..]
-    } else {
-        &new_parents
+        new_parent_ids.retain(|id| head_set.contains(id))
     };
+    let new_parents: Vec<Commit> = new_parent_ids
+        .iter()
+        .map(|id| mut_repo.store().get_commit(id))
+        .try_collect()?;
 
     let old_parents = old_commit.parents();
     let old_parent_trees = old_parents
@@ -162,7 +157,7 @@ pub fn rebase_commit_with_options(
         )
     } else {
         let old_base_tree = merge_commit_trees(mut_repo, &old_parents)?;
-        let new_base_tree = merge_commit_trees(mut_repo, new_parents)?;
+        let new_base_tree = merge_commit_trees(mut_repo, &new_parents)?;
         let old_tree = old_commit.tree()?;
         (
             Some(old_base_tree.id()),
@@ -171,7 +166,7 @@ pub fn rebase_commit_with_options(
     };
     // Ensure we don't abandon commits with multiple parents (merge commits), even
     // if they're empty.
-    if let [parent] = new_parents {
+    if let [parent] = &new_parents[..] {
         let should_abandon = match options.empty {
             EmptyBehaviour::Keep => false,
             EmptyBehaviour::AbandonNewlyEmpty => {
@@ -320,15 +315,11 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             return Ok(());
         }
 
-        let new_parents: Vec<_> = new_parent_ids
-            .iter()
-            .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
-            .try_collect()?;
         let rebased_commit: RebasedCommit = rebase_commit_with_options(
             self.settings,
             self.mut_repo,
             old_commit,
-            new_parents,
+            new_parent_ids,
             &self.options,
         )?;
         let new_commit = match rebased_commit {

lib/tests/test_merge_trees.rs

@@ -581,9 +581,20 @@ fn test_simplify_conflict_after_resolving_parent() {
         .write()
         .unwrap();
 
-    let commit_b2 = rebase_commit(&settings, tx.mut_repo(), commit_b, vec![commit_d]).unwrap();
-    let commit_c2 =
-        rebase_commit(&settings, tx.mut_repo(), commit_c, vec![commit_b2.clone()]).unwrap();
+    let commit_b2 = rebase_commit(
+        &settings,
+        tx.mut_repo(),
+        commit_b,
+        vec![commit_d.id().clone()],
+    )
+    .unwrap();
+    let commit_c2 = rebase_commit(
+        &settings,
+        tx.mut_repo(),
+        commit_c,
+        vec![commit_b2.id().clone()],
+    )
+    .unwrap();
 
     // Test the setup: Both B and C should have conflicts.
     let tree_b2 = commit_b2.tree().unwrap();
@@ -599,7 +610,13 @@ fn test_simplify_conflict_after_resolving_parent() {
         .set_tree_id(tree_b3.id())
         .write()
         .unwrap();
-    let commit_c3 = rebase_commit(&settings, tx.mut_repo(), commit_c2, vec![commit_b3]).unwrap();
+    let commit_c3 = rebase_commit(
+        &settings,
+        tx.mut_repo(),
+        commit_c2,
+        vec![commit_b3.id().clone()],
+    )
+    .unwrap();
     tx.mut_repo().rebase_descendants(&settings).unwrap();
     let repo = tx.commit("test");
 

lib/tests/test_rewrite.rs

@@ -1730,7 +1730,7 @@ fn test_rebase_abandoning_empty() {
         &settings,
         tx.mut_repo(),
         commit_b,
-        vec![commit_b2.clone()],
+        vec![commit_b2.id().clone()],
         &rebase_options,
     )
     .unwrap();