From 7fe864061f4027627c0d581eea3316d3e92e5057 Mon Sep 17 00:00:00 2001 From: Matt Stark Date: Tue, 21 Nov 2023 16:49:31 +1100 Subject: [PATCH] Implement skipping of empty commits. --- lib/src/rewrite.rs | 36 ++++++++++-- lib/tests/test_rewrite.rs | 121 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 153 insertions(+), 4 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index ef1611e9c8..916df3444e 100644 --- a/lib/src/rewrite.rs +++ b/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 { + // 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 diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 988715d9ba..51e95d200c 100644 --- a/lib/tests/test_rewrite.rs +++ b/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();