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,61 +472,89 @@ 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());
+ }
 
- let target_commit_ids: HashSet<_> = target_commits.iter().ids().cloned().collect();
+ 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());
+ }
 
- 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()
- };
+ 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();
+ }
+ }
 
  // 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