cli rebase: do not allow -r --skip-empty

cli/src/commands/rebase.rs

@@ -147,7 +147,7 @@ pub(crate) struct RebaseArgs {
     /// If true, when rebasing would produce an empty commit, the commit is
     /// skipped.
     /// Will never skip merge commits with multiple non-empty parents.
-    #[arg(long)]
+    #[arg(long, conflicts_with = "revision")]
     skip_empty: bool,
 
     /// Deprecated. Please prefix the revset with `all:` instead.
@@ -179,13 +179,27 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
         .into_iter()
         .collect_vec();
     if let Some(rev_str) = &args.revision {
+        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_revision(
             ui,
             command.settings(),
             &mut workspace_command,
             &new_parents,
             rev_str,
-            &rebase_options,
         )?;
     } else if !args.source.is_empty() {
         let source_commits =
@@ -297,7 +311,6 @@ fn rebase_revision(
     workspace_command: &mut WorkspaceCommandHelper,
     new_parents: &[Commit],
     rev_str: &str,
-    rebase_options: &RebaseOptions,
 ) -> Result<(), CommandError> {
     let old_commit = workspace_command.resolve_single_rev(rev_str, ui)?;
     workspace_command.check_rewritable([&old_commit])?;
@@ -391,13 +404,7 @@ fn rebase_revision(
     // Finally, it's safe to rebase `old_commit`. At this point, it should no longer
     // have any children; they have all been rebased and the originals have been
     // abandoned.
-    rebase_commit_with_options(
-        settings,
-        tx.mut_repo(),
-        &old_commit,
-        &new_parents,
-        rebase_options,
-    )?;
+    rebase_commit(settings, tx.mut_repo(), &old_commit, &new_parents)?;
     debug_assert_eq!(tx.mut_repo().rebase_descendants(settings)?, 0);
 
     if num_rebased_descendants > 0 {

cli/tests/test_rebase_command.rs

@@ -72,6 +72,19 @@ 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 '--revision <REVISION>' cannot be used with '--skip-empty'
+
+    Usage: jj rebase --destination <DESTINATION> --revision <REVISION>
+
+    For more information, try '--help'.
+    "###);
+
     // Rebase onto self with -r
     let stderr = test_env.jj_cmd_failure(&repo_path, &["rebase", "-r", "a", "-d", "a"]);
     insta::assert_snapshot!(stderr, @r###"

lib/src/rewrite.rs

@@ -215,7 +215,7 @@ pub fn back_out_commit(
         .write()?)
 }
 
-#[derive(Clone, Default, PartialEq)]
+#[derive(Clone, Default, PartialEq, Eq, Debug)]
 pub enum EmptyBehaviour {
     /// Always keep empty commits
     #[default]
@@ -236,7 +236,7 @@ pub enum EmptyBehaviour {
 // change the RebaseOptions construction in the CLI, and changing the
 // rebase_commit function to actually use the flag, and ensure we don't need to
 // plumb it in.
-#[derive(Clone, Default)]
+#[derive(Clone, Default, PartialEq, Eq, Debug)]
 pub struct RebaseOptions {
     pub empty: EmptyBehaviour,
 }