From 7bebced5a671500c9ca1ae9d33f7501c75807212 Mon Sep 17 00:00:00 2001
From: Ilya Grigoriev <ilyagr@users.noreply.github.com>
Date: Sun, 26 Nov 2023 16:01:53 -0800
Subject: [PATCH] cli rebase: do not allow `-r --skip-empty`

This follows up on 3967f63 (see that commit's description for more
motivation) and e79c8b6.

In a discussion linked below, it was decided that forbidding `-r --skip-empty`
entirely is preferable to the mixed behavior introduced in 3967f63.

https://github.com/martinvonz/jj/commit/3967f637dc31abdf8ed8817a377defab70821e37#commitcomment-133539911
---
 cli/src/commands/rebase.rs       | 26 ++++++++++++++++----------
 cli/tests/test_rebase_command.rs | 13 +++++++++++++
 lib/src/rewrite.rs               |  4 ++--
 3 files changed, 31 insertions(+), 12 deletions(-)

diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs
index 58bd394a22..7f164471cb 100644
--- a/cli/src/commands/rebase.rs
+++ b/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,26 @@ 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 {
+        // 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.
+        assert_eq!(
+            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 +310,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 +403,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 {
diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs
index 477c6cf363..07b19c8dc3 100644
--- a/cli/tests/test_rebase_command.rs
+++ b/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###"
diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs
index 5c3d0a6d3d..74af33dee5 100644
--- a/lib/src/rewrite.rs
+++ b/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,
 }