Make jj rebase --skip-empty keep already empty commits

CHANGELOG.md

@@ -28,6 +28,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Dropped support for the deprecated `:` revset operator. Use `::` instead.
 
+* `jj rebase --skip-empty` no longer abandons commits that were already empty
+  before the rebase.
+
 ### New features
 
 * Added support for commit signing and verification. This comes with 

cli/src/commands/rebase.rs

@@ -155,9 +155,9 @@ pub(crate) struct RebaseArgs {
     destination: Vec<RevisionArg>,
 
     /// If true, when rebasing would produce an empty commit, the commit is
-    /// skipped.
-    /// Will never skip merge commits with multiple non-empty parents.
-    /// Will never skip the working commit.
+    /// 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 = "revision")]
     skip_empty: bool,
 
@@ -181,7 +181,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
 
     let rebase_options = RebaseOptions {
         empty: match args.skip_empty {
-            true => EmptyBehaviour::AbandonAllEmpty,
+            true => EmptyBehaviour::AbandonNewlyEmpty,
             false => EmptyBehaviour::Keep,
         },
         simplify_ancestor_merge: false,

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

@@ -1461,7 +1461,7 @@ J          J
 * `-s`, `--source <SOURCE>` — Rebase specified revision(s) together their tree of descendants (can be repeated)
 * `-r`, `--revision <REVISION>` — Rebase only this revision, rebasing descendants onto this revision's parent(s)
 * `-d`, `--destination <DESTINATION>` — The revision(s) to rebase onto (can be repeated to create a merge commit)
-* `--skip-empty` — If true, when rebasing would produce an empty commit, the commit is skipped. Will never skip merge commits with multiple non-empty parents. Will never skip the working commit
+* `--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
 
   Possible values: `true`, `false`
 

cli/tests/test_rebase_command.rs

@@ -1038,3 +1038,46 @@ fn test_rebase_with_child_and_descendant_bug_2600() {
     ◉
     "###);
 }
+
+#[test]
+fn test_rebase_skip_empty() {
+    let test_env = TestEnvironment::default();
+    test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]);
+    let repo_path = test_env.env_root().join("repo");
+
+    create_commit(&test_env, &repo_path, "a", &[]);
+    create_commit(&test_env, &repo_path, "b", &["a"]);
+    test_env.jj_cmd_ok(&repo_path, &["new", "a", "-m", "will become empty"]);
+    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"]);
+
+    // 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", "-d=b", "--skip-empty"]);
+    insta::assert_snapshot!(stdout, @"");
+    insta::assert_snapshot!(stderr, @r###"
+    Rebased 3 commits
+    Working copy now at: yostqsxw 6b74c840 (empty) also already empty
+    Parent commit      : vruxwmqv 48a31526 (empty) already empty
+    "###);
+
+    // The parent commit became empty and was dropped, but the already empty commits
+    // were kept
+    insta::assert_snapshot!(test_env.jj_cmd_success(&repo_path, &["log", "-T", "description"]), @r###"
+    @  also already empty
+    ◉  already empty
+    ◉  b
+    ◉  a
+    ◉
+    "###);
+}

lib/src/rewrite.rs

@@ -105,13 +105,22 @@ pub fn rebase_commit(
     old_commit: &Commit,
     new_parents: &[Commit],
 ) -> Result<Commit, TreeMergeError> {
-    rebase_commit_with_options(
+    let rebased_commit = rebase_commit_with_options(
         settings,
         mut_repo,
         old_commit,
         new_parents,
         &Default::default(),
-    )
+    )?;
+    match rebased_commit {
+        RebasedCommit::Rewritten(new_commit) => Ok(new_commit),
+        RebasedCommit::Abandoned { parent: _ } => panic!("Commit was unexpectedly abandoned"),
+    }
+}
+
+pub enum RebasedCommit {
+    Rewritten(Commit),
+    Abandoned { parent: Commit },
 }
 
 pub fn rebase_commit_with_options(
@@ -120,7 +129,7 @@ pub fn rebase_commit_with_options(
     old_commit: &Commit,
     new_parents: &[Commit],
     options: &RebaseOptions,
-) -> Result<Commit, TreeMergeError> {
+) -> Result<RebasedCommit, TreeMergeError> {
     let old_parents = old_commit.parents();
     let old_parent_trees = old_parents
         .iter()
@@ -159,28 +168,26 @@ pub fn rebase_commit_with_options(
             }
             EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id,
         };
-        // If the user runs `jj checkout foo`, then `jj rebase -s foo -d bar`, and we
-        // drop the checked out empty commit, then the user will unknowingly
-        // have done the equivalent of `jj edit foo` instead of `jj checkout
-        // foo`. Thus, we never allow dropping the working commit. See #2766 and
-        // #2760 for discussions.
-        if should_abandon && !mut_repo.view().is_wc_commit_id(old_commit.id()) {
+        if should_abandon {
             // Record old_commit as being succeeded by the parent.
             // This ensures that when we stack commits, the second commit knows to
             // rebase on top of the parent commit, rather than the abandoned commit.
             mut_repo.record_rewritten_commit(old_commit.id().clone(), parent.id().clone());
-            return Ok(parent.clone());
+            return Ok(RebasedCommit::Abandoned {
+                parent: parent.clone(),
+            });
         }
     }
     let new_parent_ids = new_parents
         .iter()
         .map(|commit| commit.id().clone())
         .collect();
-    Ok(mut_repo
+    let new_commit = mut_repo
         .rewrite_commit(settings, old_commit)
         .set_parents(new_parent_ids)
         .set_tree_id(new_tree_id)
-        .write()?)
+        .write()?;
+    Ok(RebasedCommit::Rewritten(new_commit))
 }
 
 pub fn rebase_to_dest_parent(
@@ -458,9 +465,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
         &mut self,
         old_commit_id: CommitId,
         new_commit_ids: Vec<CommitId>,
-        abandoned_old_commit: bool,
     ) -> Result<(), BackendError> {
         // We arbitrarily pick a new working-copy commit among the candidates.
+        let abandoned_old_commit = self.abandoned.contains(&old_commit_id);
         self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?;
 
         if let Some(branch_names) = self.branches.get(&old_commit_id).cloned() {
@@ -533,13 +540,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             // (i.e. it's part of the input for this rebase). We don't need
             // to rebase it, but we still want to update branches pointing
             // to the old commit.
-            self.update_references(old_commit_id, new_parent_ids, false)?;
+            self.update_references(old_commit_id, new_parent_ids)?;
             return Ok(());
         }
         if let Some(divergent_ids) = self.divergent.get(&old_commit_id).cloned() {
             // Leave divergent commits in place. Don't update `parent_mapping` since we
             // don't want to rebase descendants either.
-            self.update_references(old_commit_id, divergent_ids, false)?;
+            self.update_references(old_commit_id, divergent_ids)?;
             return Ok(());
         }
         let old_parent_ids = old_commit.parent_ids();
@@ -548,7 +555,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             // Update the `new_parents` map so descendants are rebased correctly.
             self.parent_mapping
                 .insert(old_commit_id.clone(), new_parent_ids.clone());
-            self.update_references(old_commit_id, new_parent_ids, true)?;
+            self.update_references(old_commit_id, new_parent_ids)?;
             return Ok(());
         } else if new_parent_ids == old_parent_ids {
             // The commit is already in place.
@@ -569,13 +576,20 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             .iter()
             .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
             .try_collect()?;
-        let new_commit = rebase_commit_with_options(
+        let rebased_commit: RebasedCommit = rebase_commit_with_options(
             self.settings,
             self.mut_repo,
             &old_commit,
             &new_parents,
             &self.options,
         )?;
+        let new_commit = match rebased_commit {
+            RebasedCommit::Rewritten(new_commit) => new_commit,
+            RebasedCommit::Abandoned { parent } => {
+                self.abandoned.insert(old_commit.id().clone());
+                parent
+            }
+        };
         let previous_rebased_value = self
             .rebased
             .insert(old_commit_id.clone(), new_commit.id().clone());
@@ -587,7 +601,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             (None, None),
             "Trying to rebase the same commit {old_commit_id:?} in two different ways",
         );
-        self.update_references(old_commit_id, vec![new_commit.id().clone()], false)?;
+        self.update_references(old_commit_id, vec![new_commit.id().clone()])?;
         Ok(())
     }
 

lib/tests/test_rewrite.rs

@@ -1663,24 +1663,17 @@ fn test_rebase_abandoning_empty() {
     // Rebase B onto B2, where B2 and B have the same tree, abandoning all empty
     // commits.
     //
-    // We expect B, D, and G to be skipped because they're empty, but not E because
-    // it's the working commit. F also remains as it's not empty.
-    //
-    // F G (empty)                    F'
-    // |/                             |
-    // E (WC, empty)  D (empty)       E' (WC, empty)
-    // |             /                |
+    // We expect B, D, E, and G to be skipped because they're empty. F remains
+    // as it's not empty.
+    // F G (empty)
+    // |/
+    // E (WC, empty)  D (empty)       F' E' (WC, empty)
+    // |             /                |/
     // C-------------                 C'
     // |                           => |
     // B B2                           B2
     // |/                             |
     // A                              A
-    //
-    // TODO(#2869): There is a minor bug here. We'd like the result to be
-    // equivalent to rebasing and then `jj abandon`-ing all the empty commits.
-    // In case of the working copy, this would mean that F' would be a direct
-    // child of C', and the working copy would be a new commit that's also a
-    // direct child of C'.
 
     let mut tx = repo.start_transaction(&settings);
     let commit_a = write_random_commit(tx.mut_repo(), &settings);
@@ -1742,19 +1735,22 @@ fn test_rebase_abandoning_empty() {
     let new_commit_c =
         assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_c, &[commit_b2.id()]);
     assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_d, new_commit_c.id());
-    let new_commit_e =
-        assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_e, &[new_commit_c.id()]);
+    assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_e, new_commit_c.id());
     let new_commit_f =
-        assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[new_commit_e.id()]);
-    assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_e.id());
+        assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_f, &[new_commit_c.id()]);
+    assert_abandoned_with_parent(tx.mut_repo(), &rebase_map, &commit_g, new_commit_c.id());
 
-    assert_eq!(
-        *tx.mut_repo().view().heads(),
-        hashset! {new_commit_f.id().clone()}
-    );
+    let new_wc_commit_id = tx
+        .mut_repo()
+        .view()
+        .get_wc_commit_id(&workspace)
+        .unwrap()
+        .clone();
+    let new_wc_commit = tx.mut_repo().store().get_commit(&new_wc_commit_id).unwrap();
+    assert_eq!(new_wc_commit.parent_ids(), &[new_commit_c.id().clone()]);
 
     assert_eq!(
-        tx.mut_repo().view().get_wc_commit_id(&workspace),
-        Some(new_commit_e.id())
+        *tx.mut_repo().view().heads(),
+        hashset! {new_commit_f.id().clone(), new_wc_commit_id.clone()}
     );
 }