diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index dabda33989..4618d6a7a5 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -184,6 +184,7 @@ Please use `jj rebase -d 'all:x|y'` instead of `jj rebase --allow-large-revsets true => EmptyBehaviour::AbandonAllEmpty, false => EmptyBehaviour::Keep, }, + simplify_ancestor_merge: true, }; let mut workspace_command = command.workspace_helper(ui)?; let new_parents = cli_util::resolve_all_revs(&workspace_command, &args.destination)? diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 00f72c835a..c12d9877a6 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -241,9 +241,21 @@ pub enum EmptyBehaviour { // change the RebaseOptions construction in the CLI, and changing the // rebase_commit function to actually use the flag, and ensure we don't need to // plumb it in. -#[derive(Clone, Default, PartialEq, Eq, Debug)] +#[derive(Clone, PartialEq, Eq, Debug)] pub struct RebaseOptions { pub empty: EmptyBehaviour, + /// If a merge commit would end up with one parent being an ancestor of the + /// other, then filter out the ancestor. + pub simplify_ancestor_merge: bool, +} + +impl Default for RebaseOptions { + fn default() -> Self { + Self { + empty: Default::default(), + simplify_ancestor_merge: true, + } + } } /// Rebases descendants of a commit onto a new commit (or several). @@ -541,7 +553,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { return Ok(()); } let old_parent_ids = old_commit.parent_ids(); - let new_parent_ids = self.new_parents(old_parent_ids); + let mut new_parent_ids = self.new_parents(old_parent_ids); if self.abandoned.contains(&old_commit_id) { // Update the `new_parents` map so descendants are rebased correctly. self.parent_mapping @@ -553,16 +565,18 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { return Ok(()); } - // Don't create commit where one parent is an ancestor of another. - let head_set: HashSet<_> = self - .mut_repo - .index() - .heads(&mut new_parent_ids.iter()) - .into_iter() - .collect(); + // If specified, don't create commit where one parent is an ancestor of another. + if self.options.simplify_ancestor_merge { + let head_set: HashSet<_> = self + .mut_repo + .index() + .heads(&mut new_parent_ids.iter()) + .into_iter() + .collect(); + new_parent_ids.retain(|new_parent| head_set.contains(new_parent)); + } let new_parents: Vec<_> = new_parent_ids .iter() - .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_with_options( diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 5a2b29367c..d405e31eb4 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -506,9 +506,8 @@ fn test_rebase_descendants_abandon_and_replace() { ); } -// TODO(#2600): This behavior may need to change #[test] -fn test_rebase_descendants_abandon_degenerate_merge() { +fn test_rebase_descendants_abandon_degenerate_merge_simplify() { let settings = testutils::user_settings(); let test_repo = TestRepo::init(); let repo = &test_repo.repo; @@ -531,7 +530,13 @@ fn test_rebase_descendants_abandon_degenerate_merge() { tx.mut_repo().record_abandoned_commit(commit_b.id().clone()); let rebase_map = tx .mut_repo() - .rebase_descendants_return_map(&settings) + .rebase_descendants_with_options_return_map( + &settings, + RebaseOptions { + simplify_ancestor_merge: true, + ..Default::default() + }, + ) .unwrap(); let new_commit_d = assert_rebased_onto(tx.mut_repo(), &rebase_map, &commit_d, &[commit_c.id()]); assert_eq!(rebase_map.len(), 1); @@ -542,6 +547,52 @@ fn test_rebase_descendants_abandon_degenerate_merge() { ); } +#[test] +fn test_rebase_descendants_abandon_degenerate_merge_preserve() { + let settings = testutils::user_settings(); + let test_repo = TestRepo::init(); + let repo = &test_repo.repo; + + // Commit B was abandoned. Commit D should get rebased to have A and C as + // parents. + // + // D + // |\ + // B C + // |/ + // A + let mut tx = repo.start_transaction(&settings); + let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); + let commit_a = graph_builder.initial_commit(); + let commit_b = graph_builder.commit_with_parents(&[&commit_a]); + let commit_c = graph_builder.commit_with_parents(&[&commit_a]); + let commit_d = graph_builder.commit_with_parents(&[&commit_b, &commit_c]); + + tx.mut_repo().record_abandoned_commit(commit_b.id().clone()); + let rebase_map = tx + .mut_repo() + .rebase_descendants_with_options_return_map( + &settings, + RebaseOptions { + simplify_ancestor_merge: false, + ..Default::default() + }, + ) + .unwrap(); + let new_commit_d = assert_rebased_onto( + tx.mut_repo(), + &rebase_map, + &commit_d, + &[&commit_a.id(), &commit_c.id()], + ); + assert_eq!(rebase_map.len(), 1); + + assert_eq!( + *tx.mut_repo().view().heads(), + hashset! {new_commit_d.id().clone()} + ); +} + #[test] fn test_rebase_descendants_abandon_widen_merge() { let settings = testutils::user_settings(); @@ -1532,6 +1583,7 @@ fn test_empty_commit_option(empty_behavior: EmptyBehaviour) { &settings, RebaseOptions { empty: empty_behavior.clone(), + simplify_ancestor_merge: true, }, ) .unwrap(); @@ -1672,6 +1724,7 @@ fn test_rebase_abandoning_empty() { let rebase_options = RebaseOptions { empty: EmptyBehaviour::AbandonAllEmpty, + simplify_ancestor_merge: true, }; rebase_commit_with_options( &settings,