diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 56a354ea88..2d9344fe23 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -732,8 +732,17 @@ pub struct MutableRepo { base_repo: Arc, index: Box, view: DirtyCell, - rewritten_commits: HashMap>, - abandoned_commits: HashSet, + // TODO: make these fields private again + // The commit identified by the key has been replaced by all the ones in the value, typically + // because the key commit was abandoned (the value commits are then the abandoned commit's + // parents). A child of the key commit should be rebased onto all the value commits. A branch + // pointing to the key commit should become a conflict pointing to all the value commits. + pub(crate) parent_mapping: HashMap>, + /// Commits with a key in `parent_mapping` that have been divergently + /// rewritten into all the commits indicated by the value. + pub(crate) divergent: HashSet, + // Commits that were abandoned. Their children should be rebased onto their parents. + pub(crate) abandoned: HashSet, } impl MutableRepo { @@ -748,8 +757,9 @@ impl MutableRepo { base_repo, index: mut_index, view: DirtyCell::with_clean(mut_view), - rewritten_commits: Default::default(), - abandoned_commits: Default::default(), + parent_mapping: Default::default(), + divergent: Default::default(), + abandoned: Default::default(), } } @@ -766,8 +776,8 @@ impl MutableRepo { } pub fn has_changes(&self) -> bool { - !(self.abandoned_commits.is_empty() - && self.rewritten_commits.is_empty() + !(self.abandoned.is_empty() + && self.parent_mapping.is_empty() && self.view() == &self.base_repo.view) } @@ -816,7 +826,8 @@ impl MutableRepo { /// docstring for `record_rewritten_commit` for details. pub fn set_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); - self.rewritten_commits.insert(old_id, vec![new_id]); + self.divergent.remove(&old_id); + self.parent_mapping.insert(old_id, vec![new_id]); } /// Record a commit as being rewritten into multiple other commits in this @@ -832,8 +843,9 @@ impl MutableRepo { new_ids: impl IntoIterator, ) { assert_ne!(old_id, *self.store().root_commit_id()); - self.rewritten_commits - .insert(old_id, new_ids.into_iter().collect()); + self.parent_mapping + .insert(old_id.clone(), new_ids.into_iter().collect()); + self.divergent.insert(old_id); } /// Record a commit as having been abandoned in this transaction. @@ -847,16 +859,17 @@ impl MutableRepo { /// `old_id` would be moved to the parent(s) of `old_id` as well. pub fn record_abandoned_commit(&mut self, old_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); - self.abandoned_commits.insert(old_id); + self.abandoned.insert(old_id); } fn clear_descendant_rebaser_plans(&mut self) { - self.rewritten_commits.clear(); - self.abandoned_commits.clear(); + self.parent_mapping.clear(); + self.divergent.clear(); + self.abandoned.clear(); } pub fn has_rewrites(&self) -> bool { - !(self.rewritten_commits.is_empty() && self.abandoned_commits.is_empty()) + !(self.parent_mapping.is_empty() && self.abandoned.is_empty()) } /// After the rebaser returned by this function is dropped, @@ -870,12 +883,7 @@ impl MutableRepo { // Optimization return Ok(None); } - let mut rebaser = DescendantRebaser::new( - settings, - self, - self.rewritten_commits.clone(), - self.abandoned_commits.clone(), - ); + let mut rebaser = DescendantRebaser::new(settings, self); *rebaser.mut_options() = options; rebaser.rebase_all()?; Ok(Some(rebaser)) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index f3973ac9af..b7403a992e 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -278,18 +278,8 @@ pub struct RebaseOptions { pub(crate) struct DescendantRebaser<'settings, 'repo> { settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, - // The commit identified by the key has been replaced by all the ones in the value, typically - // because the key commit was abandoned (the value commits are then the abandoned commit's - // parents). A child of the key commit should be rebased onto all the value commits. A branch - // pointing to the key commit should become a conflict pointing to all the value commits. - parent_mapping: HashMap>, - divergent: HashSet, // In reverse order (parents after children), so we can remove the last one to rebase first. to_visit: Vec, - // 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 `parent_mapping` when we visit them. That - // way, their descendants will be rebased correctly. - abandoned: HashSet, new_commits: HashSet, rebased: HashMap, // Names of branches where local target includes the commit id in the key. @@ -313,17 +303,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { pub fn new( settings: &'settings UserSettings, mut_repo: &'repo mut MutableRepo, - rewritten: HashMap>, - abandoned: HashSet, ) -> DescendantRebaser<'settings, 'repo> { let store = mut_repo.store(); - let root_commit_id = store.root_commit_id(); - assert!(!abandoned.contains(root_commit_id)); - assert!(!rewritten.contains_key(root_commit_id)); - let old_commits_expression = RevsetExpression::commits(rewritten.keys().cloned().collect()) - .union(&RevsetExpression::commits( - abandoned.iter().cloned().collect(), - )); + let old_commits_expression = + RevsetExpression::commits(mut_repo.parent_mapping.keys().cloned().collect()).union( + &RevsetExpression::commits(mut_repo.abandoned.iter().cloned().collect()), + ); let heads_to_add_expression = old_commits_expression .parents() .minus(&old_commits_expression); @@ -349,7 +334,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { visited.insert(commit.id().clone()); let mut dependents = vec![]; for parent in commit.parents() { - if let Some(targets) = rewritten.get(parent.id()) { + if let Some(targets) = mut_repo.parent_mapping.get(parent.id()) { for target in targets { if to_visit_set.contains(target) && !visited.contains(target) { dependents.push(store.get_commit(target).unwrap()); @@ -364,19 +349,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { }, ); - let new_commits = rewritten.values().flatten().cloned().collect(); - - let mut parent_mapping = HashMap::new(); - let mut divergent = HashSet::new(); - for (old_commit, new_commits) in rewritten { - if new_commits.len() == 1 { - parent_mapping.insert(old_commit, vec![new_commits.into_iter().next().unwrap()]); - } else { - let new_commits = mut_repo.index().heads(&mut new_commits.iter()); - parent_mapping.insert(old_commit.clone(), new_commits); - divergent.insert(old_commit); - } - } + let new_commits = mut_repo + .parent_mapping + .values() + .flatten() + .cloned() + .collect(); // Build a map from commit to branches pointing to it, so we don't need to scan // all branches each time we rebase a commit. @@ -393,10 +371,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { DescendantRebaser { settings, mut_repo, - parent_mapping, - divergent, to_visit, - abandoned, new_commits, rebased: Default::default(), branches, @@ -457,11 +432,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { let mut new_ids: Vec = old_ids.to_vec(); // The longest possible non-cycle substitution sequence goes through each key of // parent_mapping once. - let mut allowed_iterations = 0..self.parent_mapping.len(); + let mut allowed_iterations = 0..self.mut_repo.parent_mapping.len(); loop { let made_replacements; - (new_ids, made_replacements) = - single_substitution_round(&self.parent_mapping, &self.divergent, new_ids); + (new_ids, made_replacements) = single_substitution_round( + &self.mut_repo.parent_mapping, + &self.mut_repo.divergent, + new_ids, + ); if !made_replacements { break; } @@ -492,18 +470,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { new_commit_ids: Vec, ) -> Result<(), BackendError> { // We arbitrarily pick a new working-copy commit among the candidates. - let abandoned_old_commit = self.abandoned.contains(&old_commit_id); + let abandoned_old_commit = self.mut_repo.abandoned.contains(&old_commit_id); self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?; if let Some(branch_names) = self.branches.get(&old_commit_id).cloned() { let mut branch_updates = vec![]; for branch_name in &branch_names { - for new_commit_id in &new_commit_ids { - self.branches - .entry(new_commit_id.clone()) - .or_default() - .insert(branch_name.clone()); - } let local_target = self.mut_repo.get_local_branch(branch_name); for old_add in local_target.added_ids() { if *old_add == old_commit_id { @@ -560,26 +532,33 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { fn rebase_one(&mut self, old_commit: Commit) -> Result<(), TreeMergeError> { let old_commit_id = old_commit.id().clone(); - if let Some(new_parent_ids) = self.parent_mapping.get(&old_commit_id).cloned() { + if self.mut_repo.parent_mapping.contains_key(&old_commit_id) { // This is a commit that had already been rebased before `self` was created // (i.e. it's part of the input for this rebase). We don't need - // to rebase it, but we still want to update branches pointing - // to the old commit. - self.update_references(old_commit_id, new_parent_ids)?; + // to rebase it. return Ok(()); } let old_parent_ids = old_commit.parent_ids(); let new_parent_ids = self.new_parents(old_parent_ids); - if self.abandoned.contains(&old_commit_id) { + if self.mut_repo.abandoned.contains(&old_commit_id) { // Update the `new_parents` map so descendants are rebased correctly. - self.parent_mapping + self.mut_repo + .parent_mapping .insert(old_commit_id.clone(), new_parent_ids.clone()); - self.update_references(old_commit_id, new_parent_ids)?; return Ok(()); - } else if new_parent_ids == old_parent_ids { + } + if new_parent_ids == old_parent_ids { // The commit is already in place. return Ok(()); } + assert_eq!( + ( + self.rebased.get(&old_commit_id), + self.mut_repo.parent_mapping.get(&old_commit_id) + ), + (None, None), + "Trying to rebase the same commit {old_commit_id:?} in two different ways", + ); let new_parents: Vec<_> = new_parent_ids .iter() @@ -595,22 +574,27 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { let new_commit = match rebased_commit { RebasedCommit::Rewritten(new_commit) => new_commit, RebasedCommit::Abandoned { parent } => { - self.abandoned.insert(old_commit.id().clone()); + self.mut_repo + .parent_mapping + .insert(old_commit_id.clone(), vec![parent.id().clone()]); + self.mut_repo.abandoned.insert(old_commit.id().clone()); parent } }; - let previous_rebased_value = self - .rebased + self.rebased .insert(old_commit_id.clone(), new_commit.id().clone()); - let previous_mapping_value = self - .parent_mapping - .insert(old_commit_id.clone(), vec![new_commit.id().clone()]); - assert_eq!( - (previous_rebased_value, previous_mapping_value), - (None, None), - "Trying to rebase the same commit {old_commit_id:?} in two different ways", - ); - self.update_references(old_commit_id, vec![new_commit.id().clone()])?; + Ok(()) + } + + fn update_all_references(&mut self) -> Result<(), BackendError> { + for (old_parent_id, new_parent_ids) in self.mut_repo.parent_mapping.clone() { + // Call `new_parents()` here since `parent_mapping` only contains direct + // mappings, not transitive ones. + // TODO: keep parent_mapping updated with transitive mappings so we don't need + // to call `new_parents()` here. + let new_parent_ids = self.new_parents(&new_parent_ids); + self.update_references(old_parent_id, new_parent_ids)?; + } Ok(()) } @@ -618,6 +602,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { while let Some(old_commit) = self.to_visit.pop() { self.rebase_one(old_commit)?; } + self.update_all_references()?; let mut view = self.mut_repo.view().store_view().clone(); for commit_id in &self.heads_to_remove { view.head_ids.remove(commit_id); diff --git a/lib/tests/test_rewrite.rs b/lib/tests/test_rewrite.rs index e811367902..6d883c31a0 100644 --- a/lib/tests/test_rewrite.rs +++ b/lib/tests/test_rewrite.rs @@ -707,12 +707,12 @@ fn test_rebase_descendants_multiple_swap() { .set_rewritten_commit(commit_b.id().clone(), commit_d.id().clone()); tx.mut_repo() .set_rewritten_commit(commit_d.id().clone(), commit_b.id().clone()); - let _ = tx.mut_repo().rebase_descendants_return_map(&settings); // Panics because of the cycle + let _ = tx.mut_repo().rebase_descendants(&settings); // Panics because of + // the cycle } -// Unlike `test_rebase_descendants_multiple_swap`, this does not currently -// panic, but it would probably be OK if it did. #[test] +#[should_panic(expected = "cycle detected")] fn test_rebase_descendants_multiple_no_descendants() { let settings = testutils::user_settings(); let test_repo = TestRepo::init(); @@ -733,19 +733,8 @@ fn test_rebase_descendants_multiple_no_descendants() { .set_rewritten_commit(commit_b.id().clone(), commit_c.id().clone()); tx.mut_repo() .set_rewritten_commit(commit_c.id().clone(), commit_b.id().clone()); - let rebase_map = tx - .mut_repo() - .rebase_descendants_return_map(&settings) - .unwrap(); - assert!(rebase_map.is_empty()); - - assert_eq!( - *tx.mut_repo().view().heads(), - hashset! { - commit_b.id().clone(), - commit_c.id().clone() - } - ); + let _ = tx.mut_repo().rebase_descendants(&settings); // Panics because of + // the cycle } #[test] @@ -1028,11 +1017,13 @@ fn test_rebase_descendants_branch_move_two_steps() { let commit_b2 = tx .mut_repo() .rewrite_commit(&settings, &commit_b) + .set_description("different") .write() .unwrap(); let commit_c2 = tx .mut_repo() .rewrite_commit(&settings, &commit_c) + .set_description("more different") .write() .unwrap(); tx.mut_repo().rebase_descendants(&settings).unwrap();