diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 6cfbfe8be..e0613b26e 100644 --- a/cli/src/commands/rebase.rs +++ b/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 diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 5a14378e3..1c15f05d2 100644 --- a/lib/src/repo.rs +++ b/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>, 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 { + pub fn rebase_descendants_with_options( + &mut self, + settings: &UserSettings, + options: RebaseOptions, + ) -> Result { 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 { + self.rebase_descendants_with_options(settings, Default::default()) + } + pub fn rebase_descendants_return_map( &mut self, settings: &UserSettings, + options: RebaseOptions, ) -> Result, TreeMergeError> { Ok(self - .rebase_descendants_return_rebaser(settings)? + .rebase_descendants_return_rebaser(settings, options)? .map_or(HashMap::new(), |rebaser| rebaser.rebased().clone())) } diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index c68dd2329..bf6538dd2 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -103,6 +103,22 @@ pub fn rebase_commit( mut_repo: &mut MutableRepo, old_commit: &Commit, new_parents: &[Commit], +) -> Result { + 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 { 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, heads_to_remove: Vec, + + // 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 { + // 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)?; diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index de5f44580..c0ec951d1 100644 --- a/lib/tests/test_rewrite.rs +++ b/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, + 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(), + } + ); +}