rebase -s: add support for --insert-after and --insert-before options

This is required when performing `rebase -s a -s b` where "b" is a
descendant of "a". Both "a" and "b" should be regarded as the roots of
the target set and be rebased onto the new destination.

CHANGELOG.md

@@ -34,6 +34,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   
 * When reconfiguring the author, warn that the working copy won't be updated
 
+* `jj rebase -s` can now be used with the `--insert-after` and `--insert-before`
+  options, like `jj rebase -r`.
+
 ### Fixed bugs
 
 * Release binaries for Intel Macs have been restored. They were previously

cli/src/commands/rebase.rs

@@ -12,7 +12,6 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::borrow::Borrow;
 use std::io::Write;
 use std::rc::Rc;
 use std::sync::Arc;
@@ -29,8 +28,6 @@ use jj_lib::repo::Repo;
 use jj_lib::revset::RevsetExpression;
 use jj_lib::revset::RevsetIteratorExt;
 use jj_lib::rewrite::move_commits;
-use jj_lib::rewrite::rebase_commit_with_options;
-use jj_lib::rewrite::CommitRewriter;
 use jj_lib::rewrite::EmptyBehaviour;
 use jj_lib::rewrite::MoveCommitsStats;
 use jj_lib::rewrite::RebaseOptions;
@@ -41,7 +38,6 @@ use crate::cli_util::short_commit_hash;
 use crate::cli_util::CommandHelper;
 use crate::cli_util::RevisionArg;
 use crate::cli_util::WorkspaceCommandHelper;
-use crate::cli_util::WorkspaceCommandTransaction;
 use crate::command_error::cli_error;
 use crate::command_error::user_error;
 use crate::command_error::CommandError;
@@ -173,26 +169,24 @@ pub(crate) struct RebaseArgs {
     /// The revision(s) to insert after (can be repeated to create a merge
     /// commit)
     ///
-    /// Only works with `-r`.
+    /// Only works with `-r` and `-s`.
     #[arg(
         long,
         short = 'A',
         visible_alias = "after",
         conflicts_with = "destination",
-        conflicts_with = "source",
         conflicts_with = "branch"
     )]
     insert_after: Vec<RevisionArg>,
     /// The revision(s) to insert before (can be repeated to create a merge
     /// commit)
     ///
-    /// Only works with `-r`.
+    /// Only works with `-r` and `-s`.
     #[arg(
         long,
         short = 'B',
         visible_alias = "before",
         conflicts_with = "destination",
-        conflicts_with = "source",
         conflicts_with = "branch"
     )]
     insert_before: Vec<RevisionArg>,
@@ -245,69 +239,27 @@ pub(crate) fn cmd_rebase(
             EmptyBehaviour::Keep,
             "clap should forbid `-r --skip-empty`"
         );
-        let target_commits: Vec<_> = workspace_command
-            .parse_union_revsets(&args.revisions)?
-            .evaluate_to_commits()?
-            .try_collect()?; // in reverse topological order
-        if !args.insert_after.is_empty() && !args.insert_before.is_empty() {
-            let after_commits =
-                workspace_command.resolve_some_revsets_default_single(&args.insert_after)?;
-            let before_commits =
-                workspace_command.resolve_some_revsets_default_single(&args.insert_before)?;
-            rebase_revisions_after_before(
-                ui,
-                command.settings(),
-                &mut workspace_command,
-                &after_commits,
-                &before_commits,
-                &target_commits,
-            )?;
-        } else if !args.insert_after.is_empty() {
-            let after_commits =
-                workspace_command.resolve_some_revsets_default_single(&args.insert_after)?;
-            rebase_revisions_after(
-                ui,
-                command.settings(),
-                &mut workspace_command,
-                &after_commits,
-                &target_commits,
-            )?;
-        } else if !args.insert_before.is_empty() {
-            let before_commits =
-                workspace_command.resolve_some_revsets_default_single(&args.insert_before)?;
-            rebase_revisions_before(
-                ui,
-                command.settings(),
-                &mut workspace_command,
-                &before_commits,
-                &target_commits,
-            )?;
-        } else {
-            let new_parents = workspace_command
-                .resolve_some_revsets_default_single(&args.destination)?
-                .into_iter()
-                .collect_vec();
-            rebase_revisions(
-                ui,
-                command.settings(),
-                &mut workspace_command,
-                &new_parents,
-                &target_commits,
-            )?;
-        }
-    } else if !args.source.is_empty() {
-        let new_parents = workspace_command
-            .resolve_some_revsets_default_single(&args.destination)?
-            .into_iter()
-            .collect_vec();
-        let source_commits = workspace_command.resolve_some_revsets_default_single(&args.source)?;
-        rebase_descendants_transaction(
+
+        rebase_revisions(
             ui,
             command.settings(),
             &mut workspace_command,
-            new_parents,
-            &source_commits,
-            rebase_options,
+            &args.revisions,
+            &args.destination,
+            &args.insert_after,
+            &args.insert_before,
+            &rebase_options,
+        )?;
+    } else if !args.source.is_empty() {
+        rebase_source(
+            ui,
+            command.settings(),
+            &mut workspace_command,
+            &args.source,
+            &args.destination,
+            &args.insert_after,
+            &args.insert_before,
+            &rebase_options,
         )?;
     } else {
         let new_parents = workspace_command
@@ -331,6 +283,83 @@ pub(crate) fn cmd_rebase(
     Ok(())
 }
 
+#[allow(clippy::too_many_arguments)]
+fn rebase_revisions(
+    ui: &mut Ui,
+    settings: &UserSettings,
+    workspace_command: &mut WorkspaceCommandHelper,
+    revisions: &[RevisionArg],
+    destination: &[RevisionArg],
+    insert_after: &[RevisionArg],
+    insert_before: &[RevisionArg],
+    rebase_options: &RebaseOptions,
+) -> Result<(), CommandError> {
+    let target_commits: Vec<_> = workspace_command
+        .parse_union_revsets(revisions)?
+        .evaluate_to_commits()?
+        .try_collect()?; // in reverse topological order
+    workspace_command.check_rewritable(target_commits.iter().ids())?;
+
+    let (new_parents, new_children) =
+        compute_rebase_destination(workspace_command, destination, insert_after, insert_before)?;
+    if !destination.is_empty() && new_children.is_empty() {
+        for commit in target_commits.iter() {
+            if new_parents.contains(commit) {
+                return Err(user_error(format!(
+                    "Cannot rebase {} onto itself",
+                    short_commit_hash(commit.id()),
+                )));
+            }
+        }
+    }
+    rebase_revisions_transaction(
+        ui,
+        settings,
+        workspace_command,
+        &new_parents.iter().ids().cloned().collect_vec(),
+        &new_children,
+        &target_commits,
+        &[],
+        rebase_options,
+    )
+}
+
+#[allow(clippy::too_many_arguments)]
+fn rebase_source(
+    ui: &mut Ui,
+    settings: &UserSettings,
+    workspace_command: &mut WorkspaceCommandHelper,
+    source: &[RevisionArg],
+    destination: &[RevisionArg],
+    insert_after: &[RevisionArg],
+    insert_before: &[RevisionArg],
+    rebase_options: &RebaseOptions,
+) -> Result<(), CommandError> {
+    let source_commits = workspace_command
+        .resolve_some_revsets_default_single(source)?
+        .into_iter()
+        .collect_vec();
+    workspace_command.check_rewritable(source_commits.iter().ids())?;
+
+    let (new_parents, new_children) =
+        compute_rebase_destination(workspace_command, destination, insert_after, insert_before)?;
+    if !destination.is_empty() && new_children.is_empty() {
+        for commit in source_commits.iter() {
+            check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?;
+        }
+    }
+
+    rebase_descendants_transaction(
+        ui,
+        settings,
+        workspace_command,
+        &new_parents.iter().ids().cloned().collect_vec(),
+        &new_children,
+        &source_commits,
+        rebase_options,
+    )
+}
+
 fn rebase_branch(
     ui: &mut Ui,
     settings: &UserSettings,
@@ -339,249 +368,186 @@ fn rebase_branch(
     branch_commits: &IndexSet<Commit>,
     rebase_options: RebaseOptions,
 ) -> Result<(), CommandError> {
-    let parent_ids = new_parents
-        .iter()
-        .map(|commit| commit.id().clone())
-        .collect_vec();
+    let parent_ids = new_parents.iter().ids().cloned().collect_vec();
     let branch_commit_ids = branch_commits
         .iter()
         .map(|commit| commit.id().clone())
         .collect_vec();
-    let roots_expression = RevsetExpression::commits(parent_ids)
+    let roots_expression = RevsetExpression::commits(parent_ids.clone())
         .range(&RevsetExpression::commits(branch_commit_ids))
         .roots();
-    let root_commits: IndexSet<_> = roots_expression
+    let root_commits: Vec<_> = roots_expression
         .evaluate_programmatic(workspace_command.repo().as_ref())
         .unwrap()
         .iter()
         .commits(workspace_command.repo().store())
         .try_collect()?;
+    workspace_command.check_rewritable(root_commits.iter().ids())?;
+    for commit in root_commits.iter() {
+        check_rebase_destinations(workspace_command.repo(), &new_parents, commit)?;
+    }
+
     rebase_descendants_transaction(
         ui,
         settings,
         workspace_command,
-        new_parents,
+        &parent_ids,
+        &[],
         &root_commits,
-        rebase_options,
+        &rebase_options,
     )
 }
 
-/// Rebases `old_commits` onto `new_parents`.
-fn rebase_descendants(
-    tx: &mut WorkspaceCommandTransaction,
-    settings: &UserSettings,
-    new_parents: Vec<Commit>,
-    old_commits: &[impl Borrow<Commit>],
-    rebase_options: RebaseOptions,
-) -> Result<usize, CommandError> {
-    for old_commit in old_commits.iter() {
-        let rewriter = CommitRewriter::new(
-            tx.mut_repo(),
-            old_commit.borrow().clone(),
-            new_parents
-                .iter()
-                .map(|parent| parent.id().clone())
-                .collect(),
-        );
-        rebase_commit_with_options(settings, rewriter, &rebase_options)?;
-    }
-    let num_rebased = old_commits.len()
-        + tx.mut_repo()
-            .rebase_descendants_with_options(settings, rebase_options)?;
-    Ok(num_rebased)
-}
-
 fn rebase_descendants_transaction(
     ui: &mut Ui,
     settings: &UserSettings,
     workspace_command: &mut WorkspaceCommandHelper,
-    new_parents: Vec<Commit>,
-    old_commits: &IndexSet<Commit>,
-    rebase_options: RebaseOptions,
+    new_parent_ids: &[CommitId],
+    new_children: &[Commit],
+    target_roots: &[Commit],
+    rebase_options: &RebaseOptions,
 ) -> Result<(), CommandError> {
-    workspace_command.check_rewritable(old_commits.iter().ids())?;
-    let (skipped_commits, old_commits) = old_commits
-        .iter()
-        .partition::<Vec<_>, _>(|commit| commit.parent_ids().iter().eq(new_parents.iter().ids()));
-    let num_skipped_rebases = skipped_commits.len();
+    if target_roots.is_empty() {
+        return Ok(());
+    }
+
+    let mut tx = workspace_command.start_transaction();
+    let tx_description = if target_roots.len() == 1 {
+        format!(
+            "rebase commit {} and descendants",
+            target_roots.first().unwrap().id().hex()
+        )
+    } else {
+        format!(
+            "rebase {} commits and their descendants",
+            target_roots.len()
+        )
+    };
+
+    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,
+        num_skipped_rebases,
+        num_abandoned,
+    } = move_commits(
+        settings,
+        tx.mut_repo(),
+        new_parent_ids,
+        new_children,
+        &target_commits,
+        &target_roots,
+        rebase_options,
+    )?;
+
     if num_skipped_rebases > 0 {
         writeln!(
             ui.status(),
             "Skipped rebase of {num_skipped_rebases} commits that were already in place"
         )?;
     }
-    if old_commits.is_empty() {
-        return Ok(());
+    if num_rebased_targets > 0 {
+        writeln!(ui.status(), "Rebased {num_rebased_targets} commits")?;
     }
-    for old_commit in old_commits.iter() {
-        check_rebase_destinations(workspace_command.repo(), &new_parents, old_commit)?;
+    if num_rebased_descendants > 0 {
+        writeln!(
+            ui.status(),
+            "Rebased {num_rebased_descendants} descendant commits"
+        )?;
     }
-    let mut tx = workspace_command.start_transaction();
-    let num_rebased =
-        rebase_descendants(&mut tx, settings, new_parents, &old_commits, rebase_options)?;
-    writeln!(ui.status(), "Rebased {num_rebased} commits")?;
-    let tx_message = if old_commits.len() == 1 {
-        format!(
-            "rebase commit {} and descendants",
-            old_commits.first().unwrap().id().hex()
-        )
-    } else {
-        format!("rebase {} commits and their descendants", old_commits.len())
-    };
-    tx.finish(ui, tx_message)?;
-    Ok(())
+    if num_abandoned > 0 {
+        writeln!(
+            ui.status(),
+            "Abandoned {num_abandoned} newly emptied commits"
+        )?;
+    }
+
+    tx.finish(ui, tx_description)
 }
 
-fn rebase_revisions(
-    ui: &mut Ui,
-    settings: &UserSettings,
+/// Computes the new parents and children given the input arguments for
+/// `destination`, `insert_after`, and `insert_before`.
+fn compute_rebase_destination(
     workspace_command: &mut WorkspaceCommandHelper,
-    new_parents: &[Commit],
-    target_commits: &[Commit],
-) -> Result<(), CommandError> {
-    if target_commits.is_empty() {
-        return Ok(());
-    }
-
-    workspace_command.check_rewritable(target_commits.iter().ids())?;
-    for commit in target_commits.iter() {
-        if new_parents.contains(commit) {
-            return Err(user_error(format!(
-                "Cannot rebase {} onto itself",
-                short_commit_hash(commit.id()),
-            )));
+    destination: &[RevisionArg],
+    insert_after: &[RevisionArg],
+    insert_before: &[RevisionArg],
+) -> Result<(Vec<Commit>, Vec<Commit>), CommandError> {
+    let resolve_revisions = |revisions: &[RevisionArg]| -> Result<Vec<Commit>, CommandError> {
+        if revisions.is_empty() {
+            Ok(vec![])
+        } else {
+            Ok(workspace_command
+                .resolve_some_revsets_default_single(revisions)?
+                .into_iter()
+                .collect_vec())
         }
+    };
+    let destination_commits = resolve_revisions(destination)?;
+    let after_commits = resolve_revisions(insert_after)?;
+    let before_commits = resolve_revisions(insert_before)?;
+
+    let (new_parents, new_children) = if !after_commits.is_empty() && !before_commits.is_empty() {
+        (after_commits, before_commits)
+    } else if !after_commits.is_empty() {
+        let new_children: Vec<_> =
+            RevsetExpression::commits(after_commits.iter().ids().cloned().collect_vec())
+                .children()
+                .evaluate_programmatic(workspace_command.repo().as_ref())?
+                .iter()
+                .commits(workspace_command.repo().store())
+                .try_collect()?;
+
+        (after_commits, new_children)
+    } else if !before_commits.is_empty() {
+        // Not using `RevsetExpression::parents` here to persist the order of parents
+        // specified in `before_commits`.
+        let new_parent_ids = before_commits
+            .iter()
+            .flat_map(|commit| commit.parent_ids().iter().cloned().collect_vec())
+            .unique()
+            .collect_vec();
+        let new_parents: Vec<_> = new_parent_ids
+            .iter()
+            .map(|commit_id| workspace_command.repo().store().get_commit(commit_id))
+            .try_collect()?;
+
+        (new_parents, before_commits)
+    } else {
+        (destination_commits, vec![])
+    };
+
+    if !new_children.is_empty() {
+        workspace_command.check_rewritable(new_children.iter().ids())?;
+        ensure_no_commit_loop(
+            workspace_command.repo().as_ref(),
+            &RevsetExpression::commits(new_children.iter().ids().cloned().collect_vec()),
+            &RevsetExpression::commits(new_parents.iter().ids().cloned().collect_vec()),
+        )?;
     }
 
-    move_commits_transaction(
-        ui,
-        settings,
-        workspace_command,
-        &new_parents.iter().ids().cloned().collect_vec(),
-        &[],
-        target_commits,
-    )
+    Ok((new_parents, new_children))
 }
 
-fn rebase_revisions_after(
-    ui: &mut Ui,
-    settings: &UserSettings,
-    workspace_command: &mut WorkspaceCommandHelper,
-    after_commits: &IndexSet<Commit>,
-    target_commits: &[Commit],
-) -> Result<(), CommandError> {
-    workspace_command.check_rewritable(target_commits.iter().ids())?;
-
-    let after_commit_ids = after_commits.iter().ids().cloned().collect_vec();
-    let new_parents_expression = RevsetExpression::commits(after_commit_ids.clone());
-    let new_children_expression = new_parents_expression.children();
-
-    ensure_no_commit_loop(
-        workspace_command.repo().as_ref(),
-        &new_children_expression,
-        &new_parents_expression,
-    )?;
-
-    let new_parent_ids = after_commit_ids;
-    let new_children: Vec<_> = new_children_expression
-        .evaluate_programmatic(workspace_command.repo().as_ref())?
-        .iter()
-        .commits(workspace_command.repo().store())
-        .try_collect()?;
-    workspace_command.check_rewritable(new_children.iter().ids())?;
-
-    move_commits_transaction(
-        ui,
-        settings,
-        workspace_command,
-        &new_parent_ids,
-        &new_children,
-        target_commits,
-    )
-}
-
-fn rebase_revisions_before(
-    ui: &mut Ui,
-    settings: &UserSettings,
-    workspace_command: &mut WorkspaceCommandHelper,
-    before_commits: &IndexSet<Commit>,
-    target_commits: &[Commit],
-) -> Result<(), CommandError> {
-    workspace_command.check_rewritable(target_commits.iter().ids())?;
-    let before_commit_ids = before_commits.iter().ids().cloned().collect_vec();
-    workspace_command.check_rewritable(&before_commit_ids)?;
-
-    let new_children_expression = RevsetExpression::commits(before_commit_ids);
-    let new_parents_expression = new_children_expression.parents();
-
-    ensure_no_commit_loop(
-        workspace_command.repo().as_ref(),
-        &new_children_expression,
-        &new_parents_expression,
-    )?;
-
-    // Not using `new_parents_expression` here to persist the order of parents
-    // specified in `before_commits`.
-    let new_parent_ids: IndexSet<_> = before_commits
-        .iter()
-        .flat_map(|commit| commit.parent_ids().iter().cloned().collect_vec())
-        .collect();
-    let new_parent_ids = new_parent_ids.into_iter().collect_vec();
-    let new_children = before_commits.iter().cloned().collect_vec();
-
-    move_commits_transaction(
-        ui,
-        settings,
-        workspace_command,
-        &new_parent_ids,
-        &new_children,
-        target_commits,
-    )
-}
-
-fn rebase_revisions_after_before(
-    ui: &mut Ui,
-    settings: &UserSettings,
-    workspace_command: &mut WorkspaceCommandHelper,
-    after_commits: &IndexSet<Commit>,
-    before_commits: &IndexSet<Commit>,
-    target_commits: &[Commit],
-) -> Result<(), CommandError> {
-    workspace_command.check_rewritable(target_commits.iter().ids())?;
-    let before_commit_ids = before_commits.iter().ids().cloned().collect_vec();
-    workspace_command.check_rewritable(&before_commit_ids)?;
-
-    let after_commit_ids = after_commits.iter().ids().cloned().collect_vec();
-    let new_children_expression = RevsetExpression::commits(before_commit_ids);
-    let new_parents_expression = RevsetExpression::commits(after_commit_ids.clone());
-
-    ensure_no_commit_loop(
-        workspace_command.repo().as_ref(),
-        &new_children_expression,
-        &new_parents_expression,
-    )?;
-
-    let new_parent_ids = after_commit_ids;
-    let new_children = before_commits.iter().cloned().collect_vec();
-
-    move_commits_transaction(
-        ui,
-        settings,
-        workspace_command,
-        &new_parent_ids,
-        &new_children,
-        target_commits,
-    )
-}
-
-/// Wraps `move_commits` in a transaction.
-fn move_commits_transaction(
+/// Creates a transaction for rebasing revisions.
+#[allow(clippy::too_many_arguments)]
+fn rebase_revisions_transaction(
     ui: &mut Ui,
     settings: &UserSettings,
     workspace_command: &mut WorkspaceCommandHelper,
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
     target_commits: &[Commit],
+    target_roots: &[CommitId],
+    rebase_options: &RebaseOptions,
 ) -> Result<(), CommandError> {
     if target_commits.is_empty() {
         return Ok(());
@@ -602,13 +568,20 @@ fn move_commits_transaction(
         num_rebased_targets,
         num_rebased_descendants,
         num_skipped_rebases,
+        num_abandoned,
     } = move_commits(
         settings,
         tx.mut_repo(),
         new_parent_ids,
         new_children,
         target_commits,
+        target_roots,
+        rebase_options,
     )?;
+    // TODO(ilyagr): Consider making it possible for descendants of the target set
+    // to become emptied, like --skip-empty. This would require writing careful
+    // tests.
+    debug_assert_eq!(num_abandoned, 0);
 
     if let Some(mut fmt) = ui.status_formatter() {
         if num_skipped_rebases > 0 {

cli/tests/cli-reference@.md.snap

@@ -1658,10 +1658,10 @@ commit. This is true in general; it is not specific to this command.
 * `-d`, `--destination <DESTINATION>` — The revision(s) to rebase onto (can be repeated to create a merge commit)
 * `-A`, `--insert-after <INSERT_AFTER>` — The revision(s) to insert after (can be repeated to create a merge commit)
 
-   Only works with `-r`.
+   Only works with `-r` and `-s`.
 * `-B`, `--insert-before <INSERT_BEFORE>` — The revision(s) to insert before (can be repeated to create a merge commit)
 
-   Only works with `-r`.
+   Only works with `-r` and `-s`.
 * `--skip-emptied` — If true, when rebasing would produce an empty commit, the commit is abandoned. It will not be abandoned if it was already empty before the rebase. Will never skip merge commits with multiple non-empty parents
 
 

cli/tests/test_rebase_command.rs

@@ -96,16 +96,6 @@ fn test_rebase_invalid() {
     For more information, try '--help'.
     "###);
 
-    // -s with --after
-    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-s", "a", "--after", "b"]);
-    insta::assert_snapshot!(stderr, @r###"
-    error: the argument '--source <SOURCE>' cannot be used with '--insert-after <INSERT_AFTER>'
-
-    Usage: jj rebase --source <SOURCE> <--destination <DESTINATION>|--insert-after <INSERT_AFTER>|--insert-before <INSERT_BEFORE>>
-
-    For more information, try '--help'.
-    "###);
-
     // -b with --after
     let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--after", "b"]);
     insta::assert_snapshot!(stderr, @r###"
@@ -129,16 +119,6 @@ fn test_rebase_invalid() {
     For more information, try '--help'.
     "###);
 
-    // -s with --before
-    let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-s", "a", "--before", "b"]);
-    insta::assert_snapshot!(stderr, @r###"
-    error: the argument '--source <SOURCE>' cannot be used with '--insert-before <INSERT_BEFORE>'
-
-    Usage: jj rebase --source <SOURCE> <--destination <DESTINATION>|--insert-after <INSERT_AFTER>|--insert-before <INSERT_BEFORE>>
-
-    For more information, try '--help'.
-    "###);
-
     // -b with --before
     let stderr = test_env.jj_cmd_cli_error(&repo_path, &["rebase", "-b", "a", "--before", "b"]);
     insta::assert_snapshot!(stderr, @r###"
@@ -892,12 +872,11 @@ fn test_rebase_with_descendants() {
     Added 0 files, modified 0 files, removed 2 files
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    ○    c: a b
-    ├─╮
+    @  d: a
+    │ ○  c: a b
+    ╭─┤
     │ ○  b: a
     ├─╯
-    │ @  d: a
-    ├─╯
     ○  a
     ◆
     "###);
@@ -921,12 +900,11 @@ fn test_rebase_with_descendants() {
     Added 0 files, modified 0 files, removed 2 files
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
-    ○    c: a b
-    ├─╮
+    @  d: a
+    │ ○  c: a b
+    ╭─┤
     │ ○  b: a
     ├─╯
-    │ @  d: a
-    ├─╯
     ○  a
     ◆
     "###);
@@ -985,7 +963,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 1 commits that were already in place
+    Skipped rebase of 4 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c: b
@@ -1003,7 +982,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 1 commits that were already in place
+    Skipped rebase of 3 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c: b
@@ -1056,7 +1036,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     // The commits in roots(base..c), i.e. commit "a" should be rebased onto "base",
     // which is a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 1 commits that were already in place
+    Skipped rebase of 3 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c: b
@@ -1093,7 +1074,8 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     insta::assert_snapshot!(stdout, @"");
     // This should be a no-op
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 1 commits that were already in place
+    Skipped rebase of 5 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
     @  c: b
@@ -1297,7 +1279,7 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
 }
 
 #[test]
-fn test_rebase_revisions_after() {
+fn test_rebase_after() {
     let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
     let repo_path = test_env.env_root().join("repo");
@@ -1698,6 +1680,62 @@ fn test_rebase_revisions_after() {
     "###);
     test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
 
+    // `rebase -s` of commit "c" and its descendants after itself should be a no-op.
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "c", "--after", "c"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Skipped rebase of 4 commits that were already in place
+    Nothing changed.
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  f: e
+    ○  e: c
+    │ ○  d: c
+    ├─╯
+    ○    c: b2 b4
+    ├─╮
+    │ ○  b4: b3
+    │ ○  b3: a
+    ○ │  b2: b1
+    ○ │  b1: a
+    ├─╯
+    ○  a
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
+    // `rebase -s` of a commit and its descendants after multiple commits.
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &["rebase", "-s", "c", "--after", "b1", "--after", "b3"],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Rebased 4 commits
+    Rebased 2 descendant commits
+    Working copy now at: xznxytkn a4ace41c f | f
+    Parent commit      : nkmrtpmo c7744d08 e | e
+    Added 0 files, modified 0 files, removed 2 files
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○    b4: d f
+    ├─╮
+    │ │ ○  b2: d f
+    ╭─┬─╯
+    │ @  f: e
+    │ ○  e: c
+    ○ │  d: c
+    ├─╯
+    ○    c: b1 b3
+    ├─╮
+    │ ○  b3: a
+    ○ │  b1: a
+    ├─╯
+    ○  a
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
     // Should error if a loop will be created.
     let stderr = test_env.jj_cmd_failure(
         &repo_path,
@@ -1709,7 +1747,7 @@ fn test_rebase_revisions_after() {
 }
 
 #[test]
-fn test_rebase_revisions_before() {
+fn test_rebase_before() {
     let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
     let repo_path = test_env.env_root().join("repo");
@@ -2119,6 +2157,92 @@ fn test_rebase_revisions_before() {
     "###);
     test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
 
+    // Rebase a subgraph before the parents of one of the commits in the subgraph.
+    // "c" had parents "b2" and "b4", but no longer has "b4" as a parent since
+    // "b4" would be a descendant of "c" after the rebase.
+    let (stdout, stderr) =
+        test_env.jj_cmd_ok(&repo_path, &["rebase", "-r", "b2::d", "--before", "a"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Rebased 3 commits onto destination
+    Rebased 6 descendant commits
+    Working copy now at: xznxytkn 308a31e9 f | f
+    Parent commit      : nkmrtpmo 538444a5 e | e
+    Added 1 files, modified 0 files, removed 0 files
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  f: e
+    ○    e: b1 b4
+    ├─╮
+    │ ○  b4: b3
+    │ ○  b3: a
+    ○ │  b1: a
+    ├─╯
+    ○  a: d
+    ○  d: c
+    ○  c: b2
+    ○  b2
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
+    // `rebase -s` of commit "c" and its descendants before itself should be a
+    // no-op.
+    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-s", "c", "--before", "c"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Skipped rebase of 4 commits that were already in place
+    Nothing changed.
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    @  f: e
+    ○  e: c
+    │ ○  d: c
+    ├─╯
+    ○    c: b2 b4
+    ├─╮
+    │ ○  b4: b3
+    │ ○  b3: a
+    ○ │  b2: b1
+    ○ │  b1: a
+    ├─╯
+    ○  a
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
+    // `rebase -s` of a commit and its descendants before multiple commits.
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &["rebase", "-s", "c", "--before", "b2", "--before", "b4"],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Rebased 4 commits
+    Rebased 2 descendant commits
+    Working copy now at: xznxytkn 84704387 f | f
+    Parent commit      : nkmrtpmo cff61821 e | e
+    Added 0 files, modified 0 files, removed 2 files
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○    b4: d f
+    ├─╮
+    │ │ ○  b2: d f
+    ╭─┬─╯
+    │ @  f: e
+    │ ○  e: c
+    ○ │  d: c
+    ├─╯
+    ○    c: b1 b3
+    ├─╮
+    │ ○  b3: a
+    ○ │  b1: a
+    ├─╯
+    ○  a
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
     // Should error if a loop will be created.
     let stderr = test_env.jj_cmd_failure(
         &repo_path,
@@ -2130,7 +2254,7 @@ fn test_rebase_revisions_before() {
 }
 
 #[test]
-fn test_rebase_revisions_after_before() {
+fn test_rebase_after_before() {
     let test_env = TestEnvironment::default();
     test_env.jj_cmd_ok(test_env.env_root(), &["git", "init", "repo"]);
     let repo_path = test_env.env_root().join("repo");
@@ -2285,6 +2409,34 @@ fn test_rebase_revisions_after_before() {
     "###);
     test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
 
+    // `rebase -s` of a commit and its descendants.
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &["rebase", "-s", "c", "--before", "b1", "--after", "b2"],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Rebased 4 commits
+    Rebased 1 descendant commits
+    Working copy now at: lylxulpl 108f0202 f | f
+    Parent commit      : kmkuslsw 52245d71 e | e
+    Added 0 files, modified 0 files, removed 1 files
+    "###);
+    insta::assert_snapshot!(get_log_output(&test_env, &repo_path), @r###"
+    ○      b1: a d f
+    ├─┬─╮
+    │ │ @  f: e
+    │ │ ○  e: c
+    │ ○ │  d: c
+    │ ├─╯
+    │ ○  c: b2
+    │ ○  b2: a
+    ├─╯
+    ○  a
+    ◆
+    "###);
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+
     // Should error if a loop will be created.
     let stderr = test_env.jj_cmd_failure(
         &repo_path,
@@ -2322,7 +2474,8 @@ fn test_rebase_skip_emptied() {
     let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-emptied"]);
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r###"
-    Rebased 3 commits
+    Rebased 2 commits
+    Abandoned 1 newly emptied commits
     Working copy now at: yostqsxw 6b74c840 (empty) also already empty
     Parent commit      : vruxwmqv 48a31526 (empty) already empty
     "###);
@@ -2370,7 +2523,8 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase with -b
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 2 commits that were already in place
+    Skipped rebase of 6 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
     @  f  lylxulpl  88f778c5:  e
@@ -2391,7 +2545,8 @@ fn test_rebase_skip_if_on_destination() {
     insta::assert_snapshot!(stdout, @"");
     // Skip rebase with -s
     insta::assert_snapshot!(stderr, @r###"
-    Skipped rebase of 1 commits that were already in place
+    Skipped rebase of 4 commits that were already in place
+    Nothing changed.
     "###);
     insta::assert_snapshot!(get_long_log_output(&test_env, &repo_path), @r###"
     @  f  lylxulpl  88f778c5:  e

lib/src/rewrite.rs

@@ -460,12 +460,16 @@ pub struct MoveCommitsStats {
     /// The number of commits for which rebase was skipped, due to the commit
     /// already being in place.
     pub num_skipped_rebases: u32,
+    /// The number of commits which were abandoned.
+    pub num_abandoned: u32,
 }
 
 /// Moves `target_commits` from their current location to a new location in the
 /// graph, given by the set of `new_parent_ids` and `new_children`.
-/// The roots of `target_commits` are rebased onto the new parents, while the
+/// Commits in `target_roots` are rebased onto the new parents, while the
 /// new children are rebased onto the heads of `target_commits`.
+/// If `target_roots` is empty, 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.
@@ -475,12 +479,15 @@ pub fn move_commits(
     new_parent_ids: &[CommitId],
     new_children: &[Commit],
     target_commits: &[Commit],
+    target_roots: &[CommitId],
+    options: &RebaseOptions,
 ) -> BackendResult<MoveCommitsStats> {
     if target_commits.is_empty() {
         return Ok(MoveCommitsStats {
             num_rebased_targets: 0,
             num_rebased_descendants: 0,
             num_skipped_rebases: 0,
+            num_abandoned: 0,
         });
     }
 
@@ -521,12 +528,18 @@ pub fn move_commits(
         connected_target_commits_internal_parents.insert(commit.id().clone(), new_parents);
     }
 
-    // Compute the roots of `target_commits`.
-    let target_roots: HashSet<_> = 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();
+    // Compute the roots of `target_commits` if not provided.
+    let target_roots: HashSet<_> = if target_roots.is_empty() {
+        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()
+    } else {
+        target_roots.iter().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
@@ -798,12 +811,10 @@ pub fn move_commits(
     let mut num_rebased_targets = 0;
     let mut num_rebased_descendants = 0;
     let mut num_skipped_rebases = 0;
+    let mut num_abandoned = 0;
 
     // Rebase each commit onto its new parents in the reverse topological order
     // computed above.
-    // TODO(ilyagr): Consider making it possible for descendants of the target set
-    // to become emptied, like --skip-empty. This would require writing careful
-    // tests.
     while let Some(old_commit_id) = to_visit.pop() {
         let old_commit = to_visit_commits.get(&old_commit_id).unwrap();
         let parent_ids = to_visit_commits_new_parents
@@ -813,8 +824,10 @@ pub fn move_commits(
         let new_parent_ids = mut_repo.new_parents(parent_ids);
         let rewriter = CommitRewriter::new(mut_repo, old_commit.clone(), new_parent_ids);
         if rewriter.parents_changed() {
-            rewriter.rebase(settings)?.write()?;
-            if target_commit_ids.contains(&old_commit_id) {
+            let rebased_commit = rebase_commit_with_options(settings, rewriter, options)?;
+            if let RebasedCommit::Abandoned { .. } = rebased_commit {
+                num_abandoned += 1;
+            } else if target_commit_ids.contains(&old_commit_id) {
                 num_rebased_targets += 1;
             } else {
                 num_rebased_descendants += 1;
@@ -829,5 +842,6 @@ pub fn move_commits(
         num_rebased_targets,
         num_rebased_descendants,
         num_skipped_rebases,
+        num_abandoned,
     })
 }