rewrite: move_commits: add MoveCommitsTarget enum to specify roots or commits to move

This also allows some minor optimizations to be performed, such as
avoiding recomputation of the connected target set when
`MoveCommitsTarget::Roots` is used since the connected target set is
identical to the target set (all descendants of the roots).

cli/src/commands/rebase.rs

@@ -30,6 +30,7 @@ use jj_lib::revset::RevsetIteratorExt;
 use jj_lib::rewrite::move_commits;
 use jj_lib::rewrite::EmptyBehaviour;
 use jj_lib::rewrite::MoveCommitsStats;
+use jj_lib::rewrite::MoveCommitsTarget;
 use jj_lib::rewrite::RebaseOptions;
 use jj_lib::settings::UserSettings;
 use tracing::instrument;
@@ -325,7 +326,7 @@ fn rebase_revisions(
         workspace_command,
         &new_parents.iter().ids().cloned().collect_vec(),
         &new_children,
-        &target_commits,
+        target_commits,
         rebase_options,
     )
 }
@@ -358,7 +359,7 @@ fn rebase_source(
         workspace_command,
         &new_parents.iter().ids().cloned().collect_vec(),
         &new_children,
-        &source_commits,
+        source_commits,
         rebase_options,
     )
 }
@@ -396,7 +397,7 @@ fn rebase_branch(
         workspace_command,
         &parent_ids,
         &[],
-        &root_commits,
+        root_commits,
         &rebase_options,
     )
 }
@@ -407,7 +408,7 @@ fn rebase_descendants_transaction(
     workspace_command: &mut WorkspaceCommandHelper,
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
-    target_roots: &[Commit],
+    target_roots: Vec<Commit>,
     rebase_options: &RebaseOptions,
 ) -> Result<(), CommandError> {
     if target_roots.is_empty() {
@@ -427,15 +428,6 @@ fn rebase_descendants_transaction(
         )
     };
 
-    let target_commits: Vec<_> =
-        RevsetExpression::commits(target_roots.iter().ids().cloned().collect_vec())
-            .descendants()
-            .evaluate_programmatic(tx.repo())?
-            .iter()
-            .commits(tx.repo().store())
-            .try_collect()?;
-    let target_roots = target_roots.iter().ids().cloned().collect_vec();
-
     let MoveCommitsStats {
         num_rebased_targets,
         num_rebased_descendants,
@@ -446,8 +438,7 @@ fn rebase_descendants_transaction(
         tx.repo_mut(),
         new_parent_ids,
         new_children,
-        &target_commits,
-        Some(&target_roots),
+        &MoveCommitsTarget::Roots(target_roots),
         rebase_options,
     )?;
 
@@ -551,7 +542,7 @@ fn rebase_revisions_transaction(
     workspace_command: &mut WorkspaceCommandHelper,
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
-    target_commits: &[Commit],
+    target_commits: Vec<Commit>,
     rebase_options: &RebaseOptions,
 ) -> Result<(), CommandError> {
     if target_commits.is_empty() {
@@ -579,8 +570,7 @@ fn rebase_revisions_transaction(
         tx.repo_mut(),
         new_parent_ids,
         new_children,
-        target_commits,
-        None,
+        &MoveCommitsTarget::Commits(target_commits),
         rebase_options,
     )?;
     // TODO(ilyagr): Consider making it possible for descendants of the target set

lib/src/rewrite.rs

@@ -472,62 +472,90 @@ pub struct MoveCommitsStats {
     pub num_abandoned: u32,
 }
 
+pub enum MoveCommitsTarget {
+    /// The commits to be moved. Commits should be mutable and in reverse
+    /// topological order.
+    Commits(Vec<Commit>),
+    /// The root commits to be moved, along with all their descendants.
+    Roots(Vec<Commit>),
+}
+
 /// Moves `target_commits` from their current location to a new location in the
 /// graph.
 ///
-/// Commits in `target_roots` are rebased onto the new parents
-/// given by `new_parent_ids`, while the `new_children` commits are
-/// rebased onto the heads of `target_commits`. If `target_roots` is
-/// `None`, it will be computed as the roots of the connected set of
-/// target commits. This assumes that `target_commits` and
-/// `new_children` can be rewritten, and there will be no cycles in
-/// the resulting graph. `target_commits` should be in reverse
-/// topological order. `target_roots`, if provided, should be a subset
-/// of `target_commits`.
+/// Commits in `target` are rebased onto the new parents given by
+/// `new_parent_ids`, while the `new_children` commits are rebased onto the
+/// heads of the commits in `targets`. This assumes that commits in `target` and
+/// `new_children` can be rewritten, and there will be no cycles in the
+/// resulting graph. Commits in `target` should be in reverse topological order.
 pub fn move_commits(
     settings: &UserSettings,
     mut_repo: &mut MutableRepo,
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
-    target_commits: &[Commit],
-    target_roots: Option<&[CommitId]>,
+    target: &MoveCommitsTarget,
     options: &RebaseOptions,
 ) -> BackendResult<MoveCommitsStats> {
-    if target_commits.is_empty() {
-        return Ok(MoveCommitsStats::default());
+    let target_commits: Vec<Commit>;
+    let target_commit_ids: HashSet<_>;
+    let connected_target_commits: Vec<Commit>;
+    let connected_target_commits_internal_parents: HashMap<CommitId, Vec<CommitId>>;
+    let target_roots: HashSet<CommitId>;
+
+    match target {
+        MoveCommitsTarget::Commits(commits) => {
+            if commits.is_empty() {
+                return Ok(MoveCommitsStats::default());
+            }
+
+            target_commits = commits.clone();
+            target_commit_ids = target_commits.iter().ids().cloned().collect();
+
+            connected_target_commits =
+                RevsetExpression::commits(target_commits.iter().ids().cloned().collect_vec())
+                    .connected()
+                    .evaluate_programmatic(mut_repo)
+                    .map_err(|err| err.expect_backend_error())?
+                    .iter()
+                    .commits(mut_repo.store())
+                    .try_collect()
+                    // TODO: Return evaluation error to caller
+                    .map_err(|err| err.expect_backend_error())?;
+            connected_target_commits_internal_parents =
+                compute_internal_parents_within(&target_commit_ids, &connected_target_commits);
+
+            target_roots = connected_target_commits_internal_parents
+                .iter()
+                .filter(|(commit_id, parents)| {
+                    target_commit_ids.contains(commit_id) && parents.is_empty()
+                })
+                .map(|(commit_id, _)| commit_id.clone())
+                .collect();
+        }
+        MoveCommitsTarget::Roots(roots) => {
+            if roots.is_empty() {
+                return Ok(MoveCommitsStats::default());
+            }
+
+            target_commits = RevsetExpression::commits(roots.iter().ids().cloned().collect_vec())
+                .descendants()
+                .evaluate_programmatic(mut_repo)
+                .map_err(|err| err.expect_backend_error())?
+                .iter()
+                .commits(mut_repo.store())
+                .try_collect()
+                // TODO: Return evaluation error to caller
+                .map_err(|err| err.expect_backend_error())?;
+            target_commit_ids = target_commits.iter().ids().cloned().collect();
+
+            connected_target_commits = target_commits.iter().cloned().collect_vec();
+            // We don't have to compute the internal parents for the connected target set,
+            // since the connected target set is the same as the target set.
+            connected_target_commits_internal_parents = HashMap::new();
+            target_roots = roots.iter().ids().cloned().collect();
+        }
     }
 
-    let target_commit_ids: HashSet<_> = target_commits.iter().ids().cloned().collect();
-
-    let connected_target_commits: Vec<_> =
-        RevsetExpression::commits(target_commits.iter().ids().cloned().collect_vec())
-            .connected()
-            .evaluate_programmatic(mut_repo)
-            .map_err(|err| err.expect_backend_error())?
-            .iter()
-            .commits(mut_repo.store())
-            .try_collect()
-            // TODO: Return evaluation error to caller
-            .map_err(|err| err.expect_backend_error())?;
-
-    // Compute the parents of all commits in the connected target set, allowing only
-    // commits in the target set as parents.
-    let connected_target_commits_internal_parents =
-        compute_internal_parents_within(&target_commit_ids, &connected_target_commits);
-
-    // Compute the roots of `target_commits` if not provided.
-    let target_roots: HashSet<_> = if let Some(target_roots) = target_roots {
-        target_roots.iter().cloned().collect()
-    } else {
-        connected_target_commits_internal_parents
-            .iter()
-            .filter(|(commit_id, parents)| {
-                target_commit_ids.contains(commit_id) && parents.is_empty()
-            })
-            .map(|(commit_id, _)| commit_id.clone())
-            .collect()
-    };
-
     // If a commit outside the target set has a commit in the target set as a
     // parent, then - after the transformation - it should have that commit's
     // ancestors which are not in the target set as parents.