rebase: allow -r with --skip-emptied

cli/src/commands/rebase.rs

@@ -175,7 +175,7 @@ pub(crate) struct RebaseArgs {
     /// abandoned. It will not be abandoned if it was already empty before the
     /// rebase. Will never skip merge commits with multiple non-empty
     /// parents.
-    #[arg(long, conflicts_with = "revisions")]
+    #[arg(long)]
     skip_emptied: bool,
 }
 
@@ -231,21 +231,6 @@ pub(crate) fn cmd_rebase(
     };
     let mut workspace_command = command.workspace_helper(ui)?;
     if !args.revisions.is_empty() {
-        assert_eq!(
-            // In principle, `-r --skip-empty` could mean to abandon the `-r`
-            // commit if it becomes empty. This seems internally consistent with
-            // the behavior of other commands, but is not very useful.
-            //
-            // It would become even more confusing once `-r --before` is
-            // implemented. If `rebase -r` behaves like `abandon`, the
-            // descendants of the `-r` commits should not be abandoned if
-            // emptied. But it would also make sense for the descendants of the
-            // `--before` commit to be abandoned if emptied. A commit can easily
-            // be in both categories.
-            rebase_options.empty,
-            EmptyBehaviour::Keep,
-            "clap should forbid `-r --skip-empty`"
-        );
         rebase_revisions(
             ui,
             command.settings(),
@@ -569,10 +554,6 @@ fn rebase_revisions_transaction(
         &MoveCommitsTarget::Commits(target_commits),
         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.
-    assert_eq!(num_abandoned, 0);
 
     if let Some(mut fmt) = ui.status_formatter() {
         if num_skipped_rebases > 0 {
@@ -590,6 +571,9 @@ fn rebase_revisions_transaction(
         if num_rebased_descendants > 0 {
             writeln!(fmt, "Rebased {num_rebased_descendants} descendant commits")?;
         }
+        if num_abandoned > 0 {
+            writeln!(fmt, "Abandoned {num_abandoned} newly emptied commits")?;
+        }
     }
 
     tx.finish(ui, tx_description)

cli/tests/test_rebase_command.rs

@@ -78,19 +78,6 @@ fn test_rebase_invalid() {
     For more information, try '--help'.
     "###);
 
-    // Both -r and --skip-empty
-    let stderr = test_env.jj_cmd_cli_error(
-        &repo_path,
-        &["rebase", "-r", "a", "-d", "b", "--skip-empty"],
-    );
-    insta::assert_snapshot!(stderr, @r###"
-    error: the argument '--revisions <REVISIONS>' cannot be used with '--skip-empty'
-
-    Usage: jj rebase --revisions <REVISIONS> <--destination <DESTINATION>|--insert-after <INSERT_AFTER>|--insert-before <INSERT_BEFORE>>
-
-    For more information, try '--help'.
-    "###);
-
     // Both -d and --after
     let stderr = test_env.jj_cmd_cli_error(
         &repo_path,
@@ -2456,6 +2443,7 @@ fn test_rebase_skip_emptied() {
     test_env.jj_cmd_ok(&repo_path, &["restore", "--from=b"]);
     test_env.jj_cmd_ok(&repo_path, &["new", "-m", "already empty"]);
     test_env.jj_cmd_ok(&repo_path, &["new", "-m", "also already empty"]);
+    let setup_opid = test_env.current_operation_id(&repo_path);
 
     // Test the setup
     insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###"
@@ -2470,12 +2458,12 @@ 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###"
+    insta::assert_snapshot!(stderr, @r#"
     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
-    "###);
+    Working copy now at: yostqsxw bc4222f2 (empty) also already empty
+    Parent commit      : vruxwmqv 6b41ecb2 (empty) already empty
+    "#);
 
     // The parent commit became empty and was dropped, but the already empty commits
     // were kept
@@ -2486,6 +2474,47 @@ fn test_rebase_skip_emptied() {
     ○  a
     ◆
     "###);
+
+    test_env.jj_cmd_ok(&repo_path, &["op", "restore", &setup_opid]);
+    // Test the setup
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###"
+    @  also already empty
+    ○  already empty
+    ○  will become empty
+    │ ○  b
+    ├─╯
+    ○  a
+    ◆
+    "###);
+
+    let (stdout, stderr) = test_env.jj_cmd_ok(
+        &repo_path,
+        &[
+            "rebase",
+            "-r=description('will become empty')",
+            "-d=b",
+            "--skip-emptied",
+        ],
+    );
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r#"
+    Rebased 2 descendant commits
+    Abandoned 1 newly emptied commits
+    Working copy now at: yostqsxw 74149b9b (empty) also already empty
+    Parent commit      : vruxwmqv 3bdb2801 (empty) already empty
+    Added 0 files, modified 0 files, removed 1 files
+    "#);
+
+    // Rebasing a single commit which becomes empty abandons that commit, whilst its
+    // already empty descendants were kept
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r#"
+    @  also already empty
+    ○  already empty
+    │ ○  b
+    ├─╯
+    ○  a
+    ◆
+    "#);
 }
 
 #[test]
@@ -2513,10 +2542,9 @@ fn test_rebase_skip_emptied_descendants() {
     ◆
     "#);
 
-    // TODO: Use `-r` instead of `-s` once `-r --skip-emptied` is supported.
     let (stdout, stderr) = test_env.jj_cmd_ok(
         &repo_path,
-        &["rebase", "-s", "b", "--before", "c", "--skip-emptied"],
+        &["rebase", "-r", "b", "--before", "c", "--skip-emptied"],
     );
     insta::assert_snapshot!(stdout, @"");
     insta::assert_snapshot!(stderr, @r#"