feat(rebase): Rename --skip-empty to --skip-emptied.

This is based on @martinvonz's comment in jj-vcs#3830 about the inconsistency between squash --keep-emptied and rebase --skip-empty.
matts1 · Jul 3, 2024 · 93a9a29 · 93a9a29
1 parent ca4eb60
commit 93a9a29
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 55 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -26,9 +26,12 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   move` to ensure that the target branch already exists.
   [#3584](https://github.com/martinvonz/jj/issues/3584)
 
+* `jj rebase --skip-empty` has been renamed to `jj rebase --skip-emptied`
+
 ### Deprecations
 
-* Replacing `-l` shorthand for `--limit` with `-n` in `jj log`, `jj op log` and `jj obslog`.
+* Replacing `-l` shorthand for `--limit` with `-n` in `jj log`, `jj op log`
+  and `jj obslog`.
 
 * `jj split --siblings` is deprecated in favor of `jj split --parallel` (to
   match `jj parallelize`).
@@ -88,7 +91,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   to-be-pushed commit has conflicts, or has no description / committer / author
   set. [#3029](https://github.com/martinvonz/jj/issues/3029)
 
-* `jj` will look for divergent changes outside the short prefix set even if it finds the change id inside the short prefix set. [#2476](https://github.com/martinvonz/jj/issues/2476)
+* `jj` will look for divergent changes outside the short prefix set even if it
+  finds the change id inside the short prefix
+  set. [#2476](https://github.com/martinvonz/jj/issues/2476)
 
 ## [0.18.0] - 2024-06-05
 
@@ -121,7 +126,6 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   were global flags and specifying them once would insert the new commit before/
   after all the specified commits.
 
-
 ### Deprecations
 
 * Attempting to alias a built-in command now gives a warning, rather than being
@@ -233,12 +237,12 @@ Thanks to the people who made this release happen!
 ### Breaking changes
 
 * The default template aliases were replaced as follows:
-  * `builtin_op_log_root(op_id: OperationId)` ->
-    `format_root_operation(root: Operation)`
-  * `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` ->
-    `format_root_commit(root: Commit)`
-  * `builtin_change_id_with_hidden_and_divergent_info` ->
-    `format_short_change_id_with_hidden_and_divergent_info(commit: Commit)`
+    * `builtin_op_log_root(op_id: OperationId)` ->
+      `format_root_operation(root: Operation)`
+    * `builtin_log_root(change_id: ChangeId, commit_id: CommitId)` ->
+      `format_root_commit(root: Commit)`
+    * `builtin_change_id_with_hidden_and_divergent_info` ->
+      `format_short_change_id_with_hidden_and_divergent_info(commit: Commit)`
 
 * The `--revision` option of `jj rebase` is renamed to `--revisions`. The short
   alias `-r` is still supported.
@@ -362,8 +366,8 @@ Thanks to the people who made this release happen!
   default `jj` commands.
 
 * Graph node symbols are now configurable via templates
-  * `templates.log_node`
-  * `templates.op_log_node`
+    * `templates.log_node`
+    * `templates.op_log_node`
 
 * `jj log` now includes synthetic nodes in the graph where some revisions were
   elided.
@@ -465,8 +469,8 @@ No code changes (fixing Rust `Cargo.toml` stuff).
   commits when they are created.
 
   This comes with out-of-the-box support for the following backends:
-  * GnuPG
-  * SSH
+    * GnuPG
+    * SSH
 
   Signature verification and an explicit sign command will hopefully come soon.
 
@@ -812,9 +816,9 @@ Thanks to the people who made this release happen!
   `git.auto-local-branch` setting is applied only to newly fetched remote
   branches. Existing remote branches are migrated as follows:
 
-  * If local branch exists, the corresponding remote branches are considered
-    tracking branches.
-  * Otherwise, the remote branches are non-tracking branches.
+    * If local branch exists, the corresponding remote branches are considered
+      tracking branches.
+    * Otherwise, the remote branches are non-tracking branches.
 
   If the deduced tracking flags are wrong, use `jj branch track`/`untrack`
   commands to fix them up.

diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs
@@ -125,8 +125,8 @@ use crate::ui::Ui;
 /// commit. This is true in general; it is not specific to this command.
 #[derive(clap::Args, Clone, Debug)]
 #[command(verbatim_doc_comment)]
-#[command(group(ArgGroup::new("to_rebase").args(&["branch", "source", "revisions"])))]
-#[command(group(ArgGroup::new("target").args(&["destination", "insert_after", "insert_before"]).multiple(true).required(true)))]
+#[command(group(ArgGroup::new("to_rebase").args(& ["branch", "source", "revisions"])))]
+#[command(group(ArgGroup::new("target").args(& ["destination", "insert_after", "insert_before"]).multiple(true).required(true)))]
 pub(crate) struct RebaseArgs {
     /// Rebase the whole branch relative to destination's ancestors (can be
     /// repeated)
@@ -188,12 +188,16 @@ pub(crate) struct RebaseArgs {
     )]
     insert_before: Vec<RevisionArg>,
 
+    /// Deprecated. Use --skip-emptied instead.
+    #[arg(long, conflicts_with = "revisions")]
+    skip_empty: bool,
+
     /// 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.
     #[arg(long, conflicts_with = "revisions")]
-    skip_empty: bool,
+    skip_emptied: bool,
 
     /// Deprecated. Please prefix the revset with `all:` instead.
     #[arg(long, short = 'L', hide = true)]
@@ -213,8 +217,14 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
         ));
     }
 
+    if args.skip_empty {
+        return Err(user_error(
+            "--skip-empty is deprecated, and has been renamed to --skip-emptied.",
+        ));
+    }
+
     let rebase_options = RebaseOptions {
-        empty: match args.skip_empty {
+        empty: match args.skip_emptied {
             true => EmptyBehaviour::AbandonNewlyEmpty,
             false => EmptyBehaviour::Keep,
         },
@@ -869,42 +879,42 @@ fn move_commits(
         .iter()
         .map(|(commit_id, commit)| {
             let new_parents =
-            // New child of the rebased target commits.
-            if let Some(new_child_parents) = new_children_parents.get(commit_id) {
-                new_child_parents.clone()
-            }
-            // Commits in the target set should persist only rebased parents from the target
-            // sets.
-            else if let Some(target_commit_parents) =
-                target_commits_internal_parents.get(commit_id)
-            {
-                // If the commit does not have any parents in the target set, it is one of the
-                // commits in the root set, and should be rebased onto the new destination.
-                if target_commit_parents.is_empty() {
-                    new_parent_ids.clone()
-                } else {
-                    target_commit_parents.clone()
+                // New child of the rebased target commits.
+                if let Some(new_child_parents) = new_children_parents.get(commit_id) {
+                    new_child_parents.clone()
                 }
-            }
-            // Commits outside the target set should have references to commits inside the set
-            // replaced.
-            else if commit
-                .parent_ids()
-                .iter()
-                .any(|id| target_commits_external_parents.contains_key(id))
-            {
-                let mut new_parents = vec![];
-                for parent in commit.parent_ids() {
-                    if let Some(parents) = target_commits_external_parents.get(parent) {
-                        new_parents.extend(parents.iter().cloned());
+                // Commits in the target set should persist only rebased parents from the target
+                // sets.
+                else if let Some(target_commit_parents) =
+                    target_commits_internal_parents.get(commit_id)
+                {
+                    // If the commit does not have any parents in the target set, it is one of the
+                    // commits in the root set, and should be rebased onto the new destination.
+                    if target_commit_parents.is_empty() {
+                        new_parent_ids.clone()
                     } else {
-                        new_parents.push(parent.clone());
+                        target_commit_parents.clone()
                     }
                 }
-                new_parents
-            } else {
-                commit.parent_ids().iter().cloned().collect_vec()
-            };
+                // Commits outside the target set should have references to commits inside the set
+                // replaced.
+                else if commit
+                    .parent_ids()
+                    .iter()
+                    .any(|id| target_commits_external_parents.contains_key(id))
+                {
+                    let mut new_parents = vec![];
+                    for parent in commit.parent_ids() {
+                        if let Some(parents) = target_commits_external_parents.get(parent) {
+                            new_parents.extend(parents.iter().cloned());
+                        } else {
+                            new_parents.push(parent.clone());
+                        }
+                    }
+                    new_parents
+                } else {
+                    commit.parent_ids().iter().cloned().collect_vec()
+                };
 
             (commit_id.clone(), new_parents)
         })

diff --git a/cli/tests/[email protected] b/cli/tests/[email protected]
@@ -1540,7 +1540,8 @@ commit. This is true in general; it is not specific to this command.
 * `-B`, `--insert-before <INSERT_BEFORE>` — The revision(s) to insert before (can be repeated to create a merge commit)
 
    Only works with `-r`.
-* `--skip-empty` — 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
+* `--skip-empty` — Deprecated. Use --skip-emptied instead
+* `--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
 
 
 

diff --git a/cli/tests/test_rebase_command.rs b/cli/tests/test_rebase_command.rs
@@ -2236,7 +2236,7 @@ fn test_rebase_revisions_after_before() {
 }
 
 #[test]
-fn test_rebase_skip_empty() {
+fn test_rebase_skip_emptied() {
     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");
@@ -2259,7 +2259,7 @@ fn test_rebase_skip_empty() {
     ◉
     "###);
 
-    let (stdout, stderr) = test_env.jj_cmd_ok(&repo_path, &["rebase", "-d=b", "--skip-empty"]);
+    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