Implement skipping of empty commits.

lib/src/rewrite.rs

@@ -122,6 +122,25 @@ pub fn rebase_commit(
         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.
+                        return Ok(parent.clone());
+                    }
+                }
+                EmptyBehaviour::Keep => {}
+            }
+        }
         merged_tree.id()
     };
     let new_parent_ids = new_parents
@@ -334,21 +353,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

@@ -66,6 +66,127 @@ 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 graph_builder = CommitGraphBuilder::new(&settings, 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 = graph_builder.initial_commit();
+    let commit_b = graph_builder.commit_with_parents_and_tree(&[&commit_a], &tree_b);
+    let commit_c = graph_builder.commit_with_parents_and_tree(&[&commit_b], &tree_c);
+    let commit_d = graph_builder.commit_with_parents_and_tree(&[&commit_b], &tree_d);
+    let commit_e = graph_builder.commit_with_parents_and_tree(&[&commit_b], &tree_b);
+    let commit_f =
+        graph_builder.commit_with_parents_and_tree(&[&commit_c, &commit_d, &commit_e], &tree_f);
+    let commit_g = graph_builder.commit_with_parents_and_tree(&[&commit_f], &tree_g);
+    let commit_bd = graph_builder.commit_with_parents_and_tree(&[&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(),
+            ..Default::default()
+        },
+    );
+
+    let new_commit_c = assert_rebased(rebaser.rebase_next().unwrap(), &commit_c, &[&commit_bd]);
+
+    let new_head = match empty {
+        EmptyBehaviour::Keep => {
+            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 => {
+            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 => {
+            assert_abandoned(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_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();