Add a --skip-empty flag to rebase

CHANGELOG.md

@@ -25,6 +25,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   usual behavior where commits that became unreachable in the Git repo are
   abandoned ([#2504](https://github.com/martinvonz/jj/pull/2504)).
 
+* `jj rebase` now takes the flag `--skip-empty`, which doesn't copy over commits
+  that would become empty after a rebase.
+
 ### Fixed bugs
 
 

cli/src/commands/rebase.rs

@@ -143,6 +143,12 @@ pub(crate) struct RebaseArgs {
     /// commit)
     #[arg(long, short, required = true)]
     destination: Vec<RevisionArg>,
+
+    /// If true, when rebasing would produce an empty commit, the commit is
+    /// skipped.
+    #[arg(long)]
+    skip_empty: bool,
+
     /// Deprecated. Please prefix the revset with `all:` instead.
     #[arg(long, short = 'L', hide = true)]
     allow_large_revsets: bool,
@@ -162,8 +168,10 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets
     }
 
     let rebase_options = RebaseOptions {
-        // TODO: Add a command-line flag for this.
-        empty: EmptyBehaviour::Keep,
+        empty: match args.skip_empty {
+            true => EmptyBehaviour::AbandonAllEmpty,
+            false => EmptyBehaviour::Keep,
+        },
     };
     let mut workspace_command = command.workspace_helper(ui)?;
     let new_parents = cli_util::resolve_all_revs(&workspace_command, ui, &args.destination)?

lib/src/rewrite.rs

@@ -118,7 +118,7 @@ pub fn rebase_commit_with_options(
     mut_repo: &mut MutableRepo,
     old_commit: &Commit,
     new_parents: &[Commit],
-    _options: &RebaseOptions,
+    options: &RebaseOptions,
 ) -> Result<Commit, TreeMergeError> {
     let old_parents = old_commit.parents();
     let old_parent_trees = old_parents
@@ -137,6 +137,26 @@ pub fn rebase_commit_with_options(
         let new_base_tree = merge_commit_trees(mut_repo, new_parents)?;
         let old_tree = old_commit.tree()?;
         let merged_tree = new_base_tree.merge(&old_base_tree, &old_tree)?;
+        // Ensure we don't abandon commits with multiple parents (merge commits), even
+        // if they're empty.
+        if let [parent] = new_parents {
+            match options.empty {
+                EmptyBehaviour::AbandonNewlyEmpty | EmptyBehaviour::AbandonAllEmpty => {
+                    if new_base_tree.id() == merged_tree.id()
+                        && (options.empty == EmptyBehaviour::AbandonAllEmpty
+                            || old_base_tree.id() != old_tree.id())
+                    {
+                        mut_repo.record_abandoned_commit(old_commit.id().clone());
+                        // Record old_commit as being succeeded by the parent for the purposes of
+                        // the rebase.
+                        // This ensures that when we stack commits, the second commit knows to
+                        // rebase on top of the parent commit, rather than the abandoned commit.
+                        return Ok(parent.clone());
+                    }
+                }
+                EmptyBehaviour::Keep => {}
+            }
+        }
         merged_tree.id()
     };
     let new_parent_ids = new_parents
@@ -352,21 +372,30 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
     }
 
     fn new_parents(&self, old_ids: &[CommitId]) -> Vec<CommitId> {
+        // This should be a set, but performance of a vec is much better since we expect
+        // 99% of commits to have <= 2 parents.
         let mut new_ids = vec![];
+        let mut add_parent = |id: &CommitId| {
+            // This can trigger if we abandon an empty commit, as both the empty commit and
+            // its parent are succeeded by the same commit.
+            if !new_ids.contains(id) {
+                new_ids.push(id.clone());
+            }
+        };
         for old_id in old_ids {
             if let Some(new_parent_ids) = self.new_parents.get(old_id) {
                 for new_parent_id in new_parent_ids {
                     // The new parent may itself have been rebased earlier in the process
                     if let Some(newer_parent_id) = self.rebased.get(new_parent_id) {
-                        new_ids.push(newer_parent_id.clone());
+                        add_parent(newer_parent_id);
                     } else {
-                        new_ids.push(new_parent_id.clone());
+                        add_parent(new_parent_id);
                     }
                 }
             } else if let Some(new_parent_id) = self.rebased.get(old_id) {
-                new_ids.push(new_parent_id.clone());
+                add_parent(new_parent_id);
             } else {
-                new_ids.push(old_id.clone());
+                add_parent(old_id);
             };
         }
         new_ids

lib/tests/test_rewrite.rs

@@ -13,11 +13,15 @@
 // limitations under the License.
 
 use itertools::Itertools as _;
+use jj_lib::commit::Commit;
 use jj_lib::matchers::{EverythingMatcher, FilesMatcher};
+use jj_lib::merged_tree::MergedTree;
 use jj_lib::op_store::{RefTarget, RemoteRef, RemoteRefState, WorkspaceId};
 use jj_lib::repo::Repo;
 use jj_lib::repo_path::RepoPath;
-use jj_lib::rewrite::{restore_tree, DescendantRebaser};
+use jj_lib::rewrite::{
+    restore_tree, DescendantRebaser, EmptyBehaviour, RebaseOptions, RebasedDescendant,
+};
 use maplit::{hashmap, hashset};
 use testutils::{
     assert_rebased, create_random_commit, create_tree, write_random_commit, CommitGraphBuilder,
@@ -66,6 +70,152 @@ fn test_restore_tree() {
     assert_eq!(restored, expected.id());
 }
 
+fn test_empty_commit_option(empty: EmptyBehaviour) {
+    let settings = testutils::user_settings();
+    let test_repo = TestRepo::init();
+    let repo = &test_repo.repo;
+
+    // Rebase a previously empty commit, a newly empty commit, and a commit with
+    // actual changes.
+    //
+    // BD (commit B joined with commit D)
+    // |   G
+    // |   |
+    // |   F (clean merge)
+    // |  /|\
+    // | C D E (empty)
+    // |  \|/
+    // |   B
+    // A__/
+    let mut tx = repo.start_transaction(&settings, "test");
+    let mut_repo = tx.mut_repo();
+    let create_fixed_tree = |paths: &[&str]| {
+        let content_map = paths
+            .iter()
+            .map(|&p| (RepoPath::from_internal_string(p), p))
+            .collect_vec();
+        let content_map_ref = content_map
+            .iter()
+            .map(|(path, content)| (path, *content))
+            .collect_vec();
+        create_tree(repo, &content_map_ref)
+    };
+
+    // The commit_with_parents function generates non-empty merge commits, so it
+    // isn't suitable for this test case.
+    let tree_b = create_fixed_tree(&["B"]);
+    let tree_c = create_fixed_tree(&["B", "C"]);
+    let tree_d = create_fixed_tree(&["B", "D"]);
+    let tree_f = create_fixed_tree(&["B", "C", "D"]);
+    let tree_g = create_fixed_tree(&["B", "C", "D", "G"]);
+
+    let commit_a = create_random_commit(mut_repo, &settings).write().unwrap();
+
+    let mut create_commit = |parents: &[&Commit], tree: &MergedTree| {
+        create_random_commit(mut_repo, &settings)
+            .set_parents(
+                parents
+                    .iter()
+                    .map(|commit| commit.id().clone())
+                    .collect_vec(),
+            )
+            .set_tree_id(tree.id())
+            .write()
+            .unwrap()
+    };
+    let commit_b = create_commit(&[&commit_a], &tree_b);
+    let commit_c = create_commit(&[&commit_b], &tree_c);
+    let commit_d = create_commit(&[&commit_b], &tree_d);
+    let commit_e = create_commit(&[&commit_b], &tree_b);
+    let commit_f = create_commit(&[&commit_c, &commit_d, &commit_e], &tree_f);
+    let commit_g = create_commit(&[&commit_f], &tree_g);
+    let commit_bd = create_commit(&[&commit_a], &tree_d);
+
+    let mut rebaser = DescendantRebaser::new(
+        &settings,
+        tx.mut_repo(),
+        hashmap! {
+            commit_b.id().clone() => hashset!{commit_bd.id().clone()}
+        },
+        hashset! {},
+        RebaseOptions {
+            empty: empty.clone(),
+        },
+    );
+
+    let new_head = match empty {
+        EmptyBehaviour::Keep => {
+            // The commit C isn't empty.
+            let new_commit_c =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+            let new_commit_d =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_d, &[&commit_bd]);
+            let new_commit_e =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]);
+            let new_commit_f = assert_rebased(
+                rebaser.rebase_next().unwrap(),
+                &commit_f,
+                &[&new_commit_c, &new_commit_d, &new_commit_e],
+            );
+            assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f])
+        }
+        EmptyBehaviour::AbandonAllEmpty => {
+            // The commit C isn't empty.
+            let new_commit_c =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+            // D and E are empty, and F is a clean merge with only one child. Thus, F is also
+            // considered empty.
+            assert_abandoned(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
+            assert_abandoned(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd);
+            assert_abandoned(rebaser.rebase_next().unwrap(), &commit_f, &new_commit_c);
+            assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_c])
+        }
+        EmptyBehaviour::AbandonNewlyEmpty => {
+            // The commit C isn't empty.
+            let new_commit_c =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+
+            // The changes in D are included in BD, so D is newly empty.
+            assert_abandoned(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
+            // E was already empty, so F is a merge commit with C and E as parents.
+            // Although it's empty, we still keep it because we don't want to drop merge commits.
+            let new_commit_e =
+                assert_rebased(rebaser.rebase_next().unwrap(), &commit_e, &[&commit_bd]);
+            let new_commit_f = assert_rebased(
+                rebaser.rebase_next().unwrap(),
+                &commit_f,
+                &[&new_commit_c, &new_commit_e],
+            );
+            assert_rebased(rebaser.rebase_next().unwrap(), &commit_g, &[&new_commit_f])
+        }
+    };
+
+    assert!(rebaser.rebase_next().unwrap().is_none());
+    assert_eq!(rebaser.rebased().len(), 5);
+
+    assert_eq!(
+        *tx.mut_repo().view().heads(),
+        hashset! {
+            new_head.id().clone(),
+        }
+    );
+}
+
+#[test]
+fn test_rebase_abandon_all_empty() {
+    test_empty_commit_option(EmptyBehaviour::AbandonAllEmpty)
+}
+
+#[test]
+fn test_rebase_abandon_newly_empty() {
+    test_empty_commit_option(EmptyBehaviour::AbandonNewlyEmpty)
+}
+
+#[test]
+fn test_rebase_keep_empty() {
+    test_empty_commit_option(EmptyBehaviour::Keep)
+}
+
 #[test]
 fn test_rebase_descendants_sideways() {
     let settings = testutils::user_settings();
@@ -1497,3 +1647,22 @@ fn test_rebase_descendants_update_checkout_abandoned_merge() {
     let checkout = repo.store().get_commit(new_checkout_id).unwrap();
     assert_eq!(checkout.parent_ids(), vec![commit_b.id().clone()]);
 }
+
+pub fn assert_abandoned(
+    rebased: Option<RebasedDescendant>,
+    expected_old_commit: &Commit,
+    expected_new_commit: &Commit,
+) {
+    if let Some(RebasedDescendant {
+        old_commit,
+        new_commit,
+    }) = rebased
+    {
+        assert_eq!(old_commit, *expected_old_commit,);
+        assert_eq!(new_commit, *expected_new_commit);
+        // Since it was abandoned, the change ID should be different.
+        assert_ne!(old_commit.change_id(), new_commit.change_id());
+    } else {
+        panic!("expected rebased commit: {rebased:?}");
+    }
+}