diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 217f4d4e5..6bbeaef31 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -107,13 +107,13 @@ pub fn back_out_commit( pub struct DescendantRebaser<'settings, 'repo> { settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, - replacements: HashMap>, + new_parents: HashMap>, // In reverse order, so we can remove the last one to rebase first. to_visit: Vec, - // Ancestors of the destinations. These were also in `to_visit` to start with, but we don't + // Commits to visit but skip. These were also in `to_visit` to start with, but we don't // want to rebase them. Instead, we record them in `replacements` when we visit them. That way, // their descendants will be rebased correctly. - ancestors: HashSet, + to_skip: HashSet, rebased: HashMap, } @@ -121,16 +121,18 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { pub fn new( settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, - replacements: HashMap>, + replaced: HashMap, + abandoned: HashSet, ) -> DescendantRebaser<'settings, 'repo> { - let old_commits_expression = - RevsetExpression::commits(replacements.keys().cloned().collect()); + let old_commits_expression = RevsetExpression::commits(replaced.keys().cloned().collect()) + .union(&RevsetExpression::commits( + abandoned.iter().cloned().collect(), + )); let new_commits_expression = - RevsetExpression::commits(replacements.values().flatten().cloned().collect()); + RevsetExpression::commits(replaced.values().cloned().collect()); - let to_visit_expression = old_commits_expression - .descendants(&RevsetExpression::all_non_obsolete_heads()) - .minus(&old_commits_expression); + let to_visit_expression = + old_commits_expression.descendants(&RevsetExpression::all_non_obsolete_heads()); let to_visit_revset = to_visit_expression .evaluate(mut_repo.as_repo_ref()) .unwrap(); @@ -145,18 +147,23 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { let ancestors_revset = ancestors_expression .evaluate(mut_repo.as_repo_ref()) .unwrap(); - let mut ancestors = HashSet::new(); + let mut to_skip = abandoned; for index_entry in ancestors_revset.iter() { - ancestors.insert(index_entry.commit_id()); + to_skip.insert(index_entry.commit_id()); } drop(ancestors_revset); + let mut new_parents = HashMap::new(); + for (old_commit, new_commit) in replaced { + new_parents.insert(old_commit, vec![new_commit]); + } + DescendantRebaser { settings, mut_repo, - replacements, + new_parents, to_visit, - ancestors, + to_skip, rebased: Default::default(), } } @@ -170,11 +177,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { pub fn rebase_next(&mut self) -> Option { while let Some(old_commit_id) = self.to_visit.pop() { + if self.new_parents.contains_key(&old_commit_id) { + continue; + } let old_commit = self.mut_repo.store().get_commit(&old_commit_id).unwrap(); let mut new_parent_ids = vec![]; let old_parent_ids = old_commit.parent_ids(); for old_parent_id in &old_parent_ids { - if let Some(replacements) = self.replacements.get(old_parent_id) { + if let Some(replacements) = self.new_parents.get(old_parent_id) { new_parent_ids.extend(replacements.clone()); } else if let Some(new_parent_id) = self.rebased.get(old_parent_id) { new_parent_ids.push(new_parent_id.clone()); @@ -182,9 +192,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { new_parent_ids.push(old_parent_id.clone()); }; } - if self.ancestors.contains(&old_commit_id) { + if self.to_skip.contains(&old_commit_id) { // Update the `replacements` map so descendants are rebased correctly. - self.replacements.insert(old_commit_id, new_parent_ids); + self.new_parents.insert(old_commit_id, new_parent_ids); continue; } else if new_parent_ids == old_parent_ids { // The commit is already in place. diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index 0da16e276..40d273f89 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -20,7 +20,7 @@ use jujutsu_lib::repo_path::RepoPath; use jujutsu_lib::rewrite::{update_branches_after_rewrite, DescendantRebaser, RebasedDescendant}; use jujutsu_lib::testutils; use jujutsu_lib::testutils::CommitGraphBuilder; -use maplit::hashmap; +use maplit::{hashmap, hashset}; use test_case::test_case; fn assert_rebased( @@ -70,8 +70,9 @@ fn test_rebase_descendants_sideways(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit6.id().clone()] + commit2.id().clone() => commit6.id().clone() }, + hashset! {}, ); let new_commit3 = assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit4, &[new_commit3.id().clone()]); @@ -115,8 +116,9 @@ fn test_rebase_descendants_forward(use_git: bool) { tx.mut_repo(), hashmap! { commit2.id().clone() => - vec![commit6.id().clone()] + commit6.id().clone() }, + hashset! {}, ); assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]); @@ -149,8 +151,9 @@ fn test_rebase_descendants_backward(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit3.id().clone() => vec![commit2.id().clone()] + commit3.id().clone() => commit2.id().clone() }, + hashset! {}, ); assert_rebased(rebaser.rebase_next(), &commit4, &[commit2.id().clone()]); assert!(rebaser.rebase_next().is_none()); @@ -188,8 +191,9 @@ fn test_rebase_descendants_internal_merge(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit6.id().clone()] + commit2.id().clone() => commit6.id().clone() }, + hashset! {}, ); let new_commit3 = assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]); let new_commit4 = assert_rebased(rebaser.rebase_next(), &commit4, &[commit6.id().clone()]); @@ -234,8 +238,9 @@ fn test_rebase_descendants_external_merge(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit3.id().clone() => vec![commit6.id().clone()] + commit3.id().clone() => commit6.id().clone() }, + hashset! {}, ); assert_rebased( rebaser.rebase_next(), @@ -250,12 +255,86 @@ fn test_rebase_descendants_external_merge(use_git: bool) { #[test_case(false ; "local backend")] #[test_case(true ; "git backend")] -fn test_rebase_descendants_degenerate_merge(use_git: bool) { +fn test_rebase_descendants_abandon(use_git: bool) { let settings = testutils::user_settings(); let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); - // Commit 2 was replaced by commit 1 (maybe it was abandoned). Commit 4 should - // get rebased to have only 3 as parent (not 1 and 3). + // Commit 2 and commit 5 were abandoned. Commit 3 and commit 4 should get + // rebased onto commit 1. Commit 6 should get rebased onto the new commit 4. + // + // 6 + // 5 + // 4 3 + // |/ + // 2 + // 1 + let mut tx = repo.start_transaction("test"); + let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); + let commit1 = graph_builder.initial_commit(); + let commit2 = graph_builder.commit_with_parents(&[&commit1]); + let commit3 = graph_builder.commit_with_parents(&[&commit2]); + let commit4 = graph_builder.commit_with_parents(&[&commit2]); + let commit5 = graph_builder.commit_with_parents(&[&commit4]); + let commit6 = graph_builder.commit_with_parents(&[&commit5]); + + let mut rebaser = DescendantRebaser::new( + &settings, + tx.mut_repo(), + hashmap! {}, + hashset! {commit2.id().clone(), commit5.id().clone()}, + ); + assert_rebased(rebaser.rebase_next(), &commit3, &[commit1.id().clone()]); + let new_commit4 = assert_rebased(rebaser.rebase_next(), &commit4, &[commit1.id().clone()]); + assert_rebased(rebaser.rebase_next(), &commit6, &[new_commit4.id().clone()]); + assert!(rebaser.rebase_next().is_none()); + assert_eq!(rebaser.rebased().len(), 3); + + tx.discard(); +} + +#[test_case(false ; "local backend")] +#[test_case(true ; "git backend")] +fn test_rebase_descendants_abandon_and_replace(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + // Commit 2 was replaced by commit 5. Commit 3 was abandoned. Commit 4 should + // get rebased onto commit 5. + // + // 4 + // 3 + // 5 2 + // |/ + // 1 + let mut tx = repo.start_transaction("test"); + let mut graph_builder = CommitGraphBuilder::new(&settings, tx.mut_repo()); + let commit1 = graph_builder.initial_commit(); + let commit2 = graph_builder.commit_with_parents(&[&commit1]); + let commit3 = graph_builder.commit_with_parents(&[&commit2]); + let commit4 = graph_builder.commit_with_parents(&[&commit3]); + let commit5 = graph_builder.commit_with_parents(&[&commit1]); + + let mut rebaser = DescendantRebaser::new( + &settings, + tx.mut_repo(), + hashmap! {commit2.id().clone() => commit5.id().clone()}, + hashset! {commit3.id().clone()}, + ); + assert_rebased(rebaser.rebase_next(), &commit4, &[commit5.id().clone()]); + assert!(rebaser.rebase_next().is_none()); + assert_eq!(rebaser.rebased().len(), 1); + + tx.discard(); +} + +#[test_case(false ; "local backend")] +#[test_case(true ; "git backend")] +fn test_rebase_descendants_abandon_degenerate_merge(use_git: bool) { + let settings = testutils::user_settings(); + let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); + + // Commit 2 was abandoned. Commit 4 should get rebased to have only 3 as parent + // (not 1 and 3). // // 4 // |\ @@ -272,9 +351,8 @@ fn test_rebase_descendants_degenerate_merge(use_git: bool) { let mut rebaser = DescendantRebaser::new( &settings, tx.mut_repo(), - hashmap! { - commit2.id().clone() => vec![commit1.id().clone()] - }, + hashmap! {}, + hashset! {commit2.id().clone()}, ); assert_rebased(rebaser.rebase_next(), &commit4, &[commit3.id().clone()]); assert!(rebaser.rebase_next().is_none()); @@ -285,12 +363,12 @@ fn test_rebase_descendants_degenerate_merge(use_git: bool) { #[test_case(false ; "local backend")] #[test_case(true ; "git backend")] -fn test_rebase_descendants_widen_merge(use_git: bool) { +fn test_rebase_descendants_abandon_widen_merge(use_git: bool) { let settings = testutils::user_settings(); let (_temp_dir, repo) = testutils::init_repo(&settings, use_git); - // Commit 5 was replaced by commits 2 and 3 (maybe 5 was abandoned). Commit 6 - // should get rebased to have 2, 3, and 4 as parents (in that order). + // Commit 5 was abandoned. Commit 6 should get rebased to have 2, 3, and 4 as + // parents (in that order). // // 6 // |\ @@ -311,9 +389,8 @@ fn test_rebase_descendants_widen_merge(use_git: bool) { let mut rebaser = DescendantRebaser::new( &settings, tx.mut_repo(), - hashmap! { - commit5.id().clone() => vec![commit2.id().clone(), commit3.id().clone()] - }, + hashmap! {}, + hashset! {commit5.id().clone()}, ); assert_rebased( rebaser.rebase_next(), @@ -357,9 +434,10 @@ fn test_rebase_descendants_multiple_sideways(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit6.id().clone()], - commit4.id().clone() => vec![commit6.id().clone()], + commit2.id().clone() => commit6.id().clone(), + commit4.id().clone() => commit6.id().clone(), }, + hashset! {}, ); assert_rebased(rebaser.rebase_next(), &commit3, &[commit6.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit5, &[commit6.id().clone()]); @@ -394,9 +472,10 @@ fn test_rebase_descendants_multiple_swap(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit4.id().clone()], - commit4.id().clone() => vec![commit2.id().clone()], + commit2.id().clone() => commit4.id().clone(), + commit4.id().clone() => commit2.id().clone(), }, + hashset! {}, ); assert_rebased(rebaser.rebase_next(), &commit3, &[commit4.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit5, &[commit2.id().clone()]); @@ -441,9 +520,10 @@ fn test_rebase_descendants_multiple_forward_and_backward(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit4.id().clone()], - commit6.id().clone() => vec![commit3.id().clone()], + commit2.id().clone() => commit4.id().clone(), + commit6.id().clone() => commit3.id().clone(), }, + hashset! {}, ); assert_rebased(rebaser.rebase_next(), &commit7, &[commit3.id().clone()]); assert_rebased(rebaser.rebase_next(), &commit8, &[commit4.id().clone()]); @@ -492,8 +572,9 @@ fn test_rebase_descendants_contents(use_git: bool) { &settings, tx.mut_repo(), hashmap! { - commit2.id().clone() => vec![commit4.id().clone()] + commit2.id().clone() => commit4.id().clone() }, + hashset! {}, ); rebaser.rebase_all(); let rebased = rebaser.rebased(); diff --git a/src/commands.rs b/src/commands.rs index bf882720b..49afcc3ac 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -62,7 +62,7 @@ use jujutsu_lib::transaction::Transaction; use jujutsu_lib::tree::{Diff, DiffSummary}; use jujutsu_lib::working_copy::{CheckoutStats, WorkingCopy}; use jujutsu_lib::{conflicts, files, git, revset}; -use maplit::hashmap; +use maplit::{hashmap, hashset}; use pest::Parser; use self::chrono::{FixedOffset, TimeZone, Utc}; @@ -2585,8 +2585,9 @@ fn cmd_rebase( ui.settings(), tx.mut_repo(), hashmap! { - old_commit.id().clone() => vec![new_commit.id().clone()] + old_commit.id().clone() => new_commit.id().clone() }, + hashset! {}, ); rebaser.rebase_all(); let num_rebased = rebaser.rebased().len() + 1; @@ -2602,9 +2603,8 @@ fn cmd_rebase( let mut rebaser = DescendantRebaser::new( ui.settings(), tx.mut_repo(), - hashmap! { - old_commit.id().clone() => old_commit.parent_ids() - }, + hashmap! {}, + hashset! {old_commit.id().clone()}, ); rebaser.rebase_all(); let num_rebased = rebaser.rebased().len();