lib: Implement skipping of empty commits

cli/src/commands/rebase.rs

@@ -334,7 +334,10 @@ fn rebase_revision(
         );
     }
     // Now, rebase the descendants of the children.
-    rebased_commit_ids.extend(tx.mut_repo().rebase_descendants_return_map(settings)?);
+    rebased_commit_ids.extend(
+        tx.mut_repo()
+            .rebase_descendants_return_map(settings, Default::default())?,
+    );
     let num_rebased_descendants = rebased_commit_ids.len();
 
     // We now update `new_parents` to account for the rebase of all of

lib/src/repo.rs

@@ -50,7 +50,7 @@ use crate::refs::{
     diff_named_ref_targets, diff_named_remote_refs, merge_ref_targets, merge_remote_refs,
 };
 use crate::revset::{self, ChangeIdIndex, Revset, RevsetExpression};
-use crate::rewrite::DescendantRebaser;
+use crate::rewrite::{DescendantRebaser, RebaseOptions};
 use crate::settings::{RepoSettings, UserSettings};
 use crate::simple_op_heads_store::SimpleOpHeadsStore;
 use crate::simple_op_store::SimpleOpStore;
@@ -855,28 +855,39 @@ impl MutableRepo {
     fn rebase_descendants_return_rebaser<'settings, 'repo>(
         &'repo mut self,
         settings: &'settings UserSettings,
+        options: RebaseOptions,
     ) -> Result<Option<DescendantRebaser<'settings, 'repo>>, TreeMergeError> {
         if !self.has_rewrites() {
             // Optimization
             return Ok(None);
         }
         let mut rebaser = self.create_descendant_rebaser(settings);
+        *rebaser.mut_options() = options;
         rebaser.rebase_all()?;
         Ok(Some(rebaser))
     }
 
-    pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> {
+    pub fn rebase_descendants_with_options(
+        &mut self,
+        settings: &UserSettings,
+        options: RebaseOptions,
+    ) -> Result<usize, TreeMergeError> {
         Ok(self
-            .rebase_descendants_return_rebaser(settings)?
+            .rebase_descendants_return_rebaser(settings, options)?
             .map_or(0, |rebaser| rebaser.rebased().len()))
     }
 
+    pub fn rebase_descendants(&mut self, settings: &UserSettings) -> Result<usize, TreeMergeError> {
+        self.rebase_descendants_with_options(settings, Default::default())
+    }
+
     pub fn rebase_descendants_return_map(
         &mut self,
         settings: &UserSettings,
+        options: RebaseOptions,
     ) -> Result<HashMap<CommitId, CommitId>, TreeMergeError> {
         Ok(self
-            .rebase_descendants_return_rebaser(settings)?
+            .rebase_descendants_return_rebaser(settings, options)?
             .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone()))
     }
 

lib/src/rewrite.rs

@@ -103,6 +103,22 @@ pub fn rebase_commit(
     mut_repo: &mut MutableRepo,
     old_commit: &Commit,
     new_parents: &[Commit],
+) -> Result<Commit, TreeMergeError> {
+    rebase_commit_with_options(
+        settings,
+        mut_repo,
+        old_commit,
+        new_parents,
+        &Default::default(),
+    )
+}
+
+pub fn rebase_commit_with_options(
+    settings: &UserSettings,
+    mut_repo: &mut MutableRepo,
+    old_commit: &Commit,
+    new_parents: &[Commit],
+    options: &RebaseOptions,
 ) -> Result<Commit, TreeMergeError> {
     let old_parents = old_commit.parents();
     let old_parent_trees = old_parents
@@ -113,16 +129,44 @@ pub fn rebase_commit(
         .iter()
         .map(|parent| parent.store_commit().root_tree.clone())
         .collect_vec();
-    let new_tree_id = if new_parent_trees == old_parent_trees {
-        // Optimization
-        old_commit.tree_id().clone()
+
+    let (old_base_tree_id, new_tree_id) = if new_parent_trees == old_parent_trees {
+        (
+            // Optimization: old_base_tree_id is only used for newly empty, but when the parents
+            // haven't changed it can't be newly empty.
+            None,
+            // Optimization: Skip merging.
+            old_commit.tree_id().clone(),
+        )
     } else {
         let old_base_tree = merge_commit_trees(mut_repo, &old_parents)?;
         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)?;
-        merged_tree.id()
+        (
+            Some(old_base_tree.id()),
+            new_base_tree.merge(&old_base_tree, &old_tree)?.id(),
+        )
     };
+    // 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 *parent.tree_id() == new_tree_id
+                    && (options.empty == EmptyBehaviour::AbandonAllEmpty
+                        || old_base_tree_id != Some(old_commit.tree_id().clone()))
+                {
+                    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 => {}
+        }
+    }
     let new_parent_ids = new_parents
         .iter()
         .map(|commit| commit.id().clone())
@@ -221,6 +265,9 @@ pub struct DescendantRebaser<'settings, 'repo> {
     // have been rebased.
     heads_to_add: HashSet<CommitId>,
     heads_to_remove: Vec<CommitId>,
+
+    // Options to apply during a rebase.
+    options: RebaseOptions,
 }
 
 impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
@@ -322,9 +369,15 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             branches,
             heads_to_add,
             heads_to_remove: Default::default(),
+            options: Default::default(),
         }
     }
 
+    /// Returns options that can be set.
+    pub fn mut_options(&mut self) -> &mut RebaseOptions {
+        &mut self.options
+    }
+
     /// Returns a map from `CommitId` of old commit to new commit. Includes the
     /// commits rebase so far. Does not include the inputs passed to
     /// `rebase_descendants`.
@@ -333,21 +386,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
@@ -477,8 +539,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
                 .filter(|new_parent| head_set.contains(new_parent))
                 .map(|new_parent_id| self.mut_repo.store().get_commit(new_parent_id))
                 .try_collect()?;
-            let new_commit =
-                rebase_commit(self.settings, self.mut_repo, &old_commit, &new_parents)?;
+            let new_commit = rebase_commit_with_options(
+                self.settings,
+                self.mut_repo,
+                &old_commit,
+                &new_parents,
+                &self.options,
+            )?;
             self.rebased
                 .insert(old_commit_id.clone(), new_commit.id().clone());
             self.update_references(old_commit_id, vec![new_commit.id().clone()], true)?;

lib/tests/test_rewrite.rs

@@ -13,12 +13,17 @@
 // 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 test_case::test_case;
 use testutils::{
     assert_rebased, create_random_commit, create_tree, write_random_commit, CommitGraphBuilder,
     TestRepo,
@@ -1480,3 +1485,157 @@ 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()]);
 }
+
+fn assert_rebase_skipped(
+    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:?}");
+    }
+}
+
+#[test_case(EmptyBehaviour::Keep; "keep all commits")]
+#[test_case(EmptyBehaviour::AbandonNewlyEmpty; "abandon newly empty commits")]
+#[test_case(EmptyBehaviour::AbandonAllEmpty ; "abandon all empty commits")]
+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! {},
+    );
+    *rebaser.mut_options() = 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_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_d, &commit_bd);
+            assert_rebase_skipped(rebaser.rebase_next().unwrap(), &commit_e, &commit_bd);
+            assert_rebase_skipped(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_rebase_skipped(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(),
+        }
+    );
+}