From 5a9a2e24118d165faf24311887374a90985952f5 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sat, 23 Mar 2024 14:49:29 -0700 Subject: [PATCH 01/10] repo: add parents of abandoned commit to parent_mapping By adding the abandoned commit's parents to `parent_mapping`, we can remove a bit more of the special handling of abandoned commitsin `DescendantRebaser`. --- lib/src/repo.rs | 8 +++++++- lib/src/rewrite.rs | 7 ------- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 2d9344fe23..838a65d5b4 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -857,9 +857,15 @@ impl MutableRepo { /// The `rebase_descendants` logic will rebase the descendants of `old_id` /// to become the descendants of parent(s) of `old_id`. Any branches at /// `old_id` would be moved to the parent(s) of `old_id` as well. + // TODO: Propagate errors from commit lookup or take a Commit as argument. pub fn record_abandoned_commit(&mut self, old_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); - self.abandoned.insert(old_id); + self.divergent.remove(&old_id); + self.abandoned.insert(old_id.clone()); + // Descendants should be rebased onto the commit's parents + let old_commit = self.store().get_commit(&old_id).unwrap(); + self.parent_mapping + .insert(old_id, old_commit.parent_ids().to_vec()); } fn clear_descendant_rebaser_plans(&mut self) { diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index b7403a992e..740b807f88 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -540,13 +540,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } let old_parent_ids = old_commit.parent_ids(); let new_parent_ids = self.new_parents(old_parent_ids); - if self.mut_repo.abandoned.contains(&old_commit_id) { - // Update the `new_parents` map so descendants are rebased correctly. - self.mut_repo - .parent_mapping - .insert(old_commit_id.clone(), new_parent_ids.clone()); - return Ok(()); - } if new_parent_ids == old_parent_ids { // The commit is already in place. return Ok(()); From 1a0b0ce7dd0fe1d60e32238ef578076a86df82f9 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 14:52:09 -0700 Subject: [PATCH 02/10] rewrite: make rebase_commit_with_options() mark abandoned commit When `rebase_commit_with_options()` decides to abandons a commit, it records the new parents in the `MutableRepo`, but it's currently the caller's responsibility to remember to mark it as abandoned. Let's move that logic into the function to reduce the risk of future bugs. --- lib/src/repo.rs | 20 +++++++++++++++++--- lib/src/rewrite.rs | 16 +++++----------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 838a65d5b4..4725b1f8a8 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -860,12 +860,26 @@ impl MutableRepo { // TODO: Propagate errors from commit lookup or take a Commit as argument. pub fn record_abandoned_commit(&mut self, old_id: CommitId) { assert_ne!(old_id, *self.store().root_commit_id()); - self.divergent.remove(&old_id); - self.abandoned.insert(old_id.clone()); // Descendants should be rebased onto the commit's parents let old_commit = self.store().get_commit(&old_id).unwrap(); + self.record_abandoned_commit_with_parents(old_id, old_commit.parent_ids().to_vec()); + } + + /// Record a commit as having been abandoned in this transaction. + /// + /// A later `rebase_descendants()` will rebase children of `old_id` onto + /// `new_parent_ids`. A working copy pointing to `old_id` will point to a + /// new commit on top of `new_parent_ids`. + pub fn record_abandoned_commit_with_parents( + &mut self, + old_id: CommitId, + new_parent_ids: impl IntoIterator, + ) { + self.divergent.remove(&old_id); + assert_ne!(old_id, *self.store().root_commit_id()); + self.abandoned.insert(old_id.clone()); self.parent_mapping - .insert(old_id, old_commit.parent_ids().to_vec()); + .insert(old_id, new_parent_ids.into_iter().collect()); } fn clear_descendant_rebaser_plans(&mut self) { diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 740b807f88..65cf7ce0c4 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -188,10 +188,10 @@ pub fn rebase_commit_with_options( EmptyBehaviour::AbandonAllEmpty => *parent.tree_id() == new_tree_id, }; if should_abandon { - // Record old_commit as being succeeded by the parent. - // This ensures that when we stack commits, the second commit knows to - // rebase on top of the parent commit, rather than the abandoned commit. - mut_repo.set_rewritten_commit(old_commit.id().clone(), parent.id().clone()); + mut_repo.record_abandoned_commit_with_parents( + old_commit.id().clone(), + std::iter::once(parent.id().clone()), + ); return Ok(RebasedCommit::Abandoned { parent: parent.clone(), }); @@ -566,13 +566,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { )?; let new_commit = match rebased_commit { RebasedCommit::Rewritten(new_commit) => new_commit, - RebasedCommit::Abandoned { parent } => { - self.mut_repo - .parent_mapping - .insert(old_commit_id.clone(), vec![parent.id().clone()]); - self.mut_repo.abandoned.insert(old_commit.id().clone()); - parent - } + RebasedCommit::Abandoned { parent } => parent, }; self.rebased .insert(old_commit_id.clone(), new_commit.id().clone()); From 8969d792a00b3cd97ffc4269f1f24f2d2f03f748 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 15:27:42 -0700 Subject: [PATCH 03/10] rewrite: move `new_parents()` to `MutableRepo` The function only uses state from `MutableRepo`, so it should be implemented on that type. --- lib/src/repo.rs | 64 +++++++++++++++++++++++++++++++++++++++++++ lib/src/rewrite.rs | 67 ++-------------------------------------------- 2 files changed, 66 insertions(+), 65 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 4725b1f8a8..541972dce1 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -892,6 +892,70 @@ impl MutableRepo { !(self.parent_mapping.is_empty() && self.abandoned.is_empty()) } + /// Calculates new parents for a commit that's currently based on the given + /// parents. It does that by considering how previous commits have been + /// rewritten and abandoned. + /// + /// Panics if `parent_mapping` contains cycles + pub fn new_parents(&self, old_ids: &[CommitId]) -> Vec { + fn single_substitution_round( + parent_mapping: &HashMap>, + divergent: &HashSet, + ids: Vec, + ) -> (Vec, bool) { + let mut made_replacements = false; + let mut new_ids = vec![]; + // TODO(ilyagr): (Maybe?) optimize common case of replacements all + // being singletons. If CommitId-s were Copy. no allocations would be needed in + // that case, but it probably doesn't matter much while they are Vec-s. + for id in ids.into_iter() { + if divergent.contains(&id) { + new_ids.push(id); + continue; + } + match parent_mapping.get(&id) { + None => new_ids.push(id), + Some(replacements) => { + assert!( + // Each commit must have a parent, so a parent can + // not just be mapped to nothing. This assertion + // could be removed if this function is used for + // mapping something other than a commit's parents. + !replacements.is_empty(), + "Found empty value for key {id:?} in the parent mapping", + ); + made_replacements = true; + new_ids.extend(replacements.iter().cloned()) + } + }; + } + (new_ids, made_replacements) + } + + 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(); + loop { + let made_replacements; + (new_ids, made_replacements) = + single_substitution_round(&self.parent_mapping, &self.divergent, new_ids); + if !made_replacements { + break; + } + allowed_iterations + .next() + .expect("cycle detected in the parent mapping"); + } + match new_ids.as_slice() { + // The first two cases are an optimization for the common case of commits with <=2 + // parents + [_singleton] => new_ids, + [a, b] if a != b => new_ids, + _ => new_ids.into_iter().unique().collect(), + } + } + /// After the rebaser returned by this function is dropped, /// self.clear_descendant_rebaser_plans() needs to be called. fn rebase_descendants_return_rebaser<'settings, 'repo>( diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 65cf7ce0c4..aeb30d746b 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -393,69 +393,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.rebased } - /// Panics if `parent_mapping` contains cycles - fn new_parents(&self, old_ids: &[CommitId]) -> Vec { - fn single_substitution_round( - parent_mapping: &HashMap>, - divergent: &HashSet, - ids: Vec, - ) -> (Vec, bool) { - let mut made_replacements = false; - let mut new_ids = vec![]; - // TODO(ilyagr): (Maybe?) optimize common case of replacements all - // being singletons. If CommitId-s were Copy. no allocations would be needed in - // that case, but it probably doesn't matter much while they are Vec-s. - for id in ids.into_iter() { - if divergent.contains(&id) { - new_ids.push(id); - continue; - } - match parent_mapping.get(&id) { - None => new_ids.push(id), - Some(replacements) => { - assert!( - // Each commit must have a parent, so a parent can - // not just be mapped to nothing. This assertion - // could be removed if this function is used for - // mapping something other than a commit's parents. - !replacements.is_empty(), - "Found empty value for key {id:?} in the parent mapping", - ); - made_replacements = true; - new_ids.extend(replacements.iter().cloned()) - } - }; - } - (new_ids, made_replacements) - } - - 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.mut_repo.parent_mapping.len(); - loop { - let made_replacements; - (new_ids, made_replacements) = single_substitution_round( - &self.mut_repo.parent_mapping, - &self.mut_repo.divergent, - new_ids, - ); - if !made_replacements { - break; - } - allowed_iterations - .next() - .expect("cycle detected in the parent mapping"); - } - match new_ids.as_slice() { - // The first two cases are an optimization for the common case of commits with <=2 - // parents - [_singleton] => new_ids, - [a, b] if a != b => new_ids, - _ => new_ids.into_iter().unique().collect(), - } - } - fn ref_target_update(old_id: CommitId, new_ids: Vec) -> (RefTarget, RefTarget) { let old_ids = std::iter::repeat(old_id).take(new_ids.len()); ( @@ -539,7 +476,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 new_parent_ids = self.mut_repo.new_parents(old_parent_ids); if new_parent_ids == old_parent_ids { // The commit is already in place. return Ok(()); @@ -579,7 +516,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { // 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); + let new_parent_ids = self.mut_repo.new_parents(&new_parent_ids); self.update_references(old_parent_id, new_parent_ids)?; } Ok(()) From 8f775822cb3663cd2e6563054b1db323e0ea7f46 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:33:03 -0700 Subject: [PATCH 04/10] rewrite: extract a function for updating heads --- lib/src/rewrite.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index aeb30d746b..673d9f4b84 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -522,11 +522,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { Ok(()) } - pub fn rebase_all(&mut self) -> Result<(), TreeMergeError> { - while let Some(old_commit) = self.to_visit.pop() { - self.rebase_one(old_commit)?; - } - self.update_all_references()?; + fn update_heads(&mut self) { let mut view = self.mut_repo.view().store_view().clone(); for commit_id in &self.heads_to_remove { view.head_ids.remove(commit_id); @@ -537,6 +533,15 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.heads_to_remove.clear(); self.heads_to_add.clear(); self.mut_repo.set_view(view); + } + + pub fn rebase_all(&mut self) -> Result<(), TreeMergeError> { + while let Some(old_commit) = self.to_visit.pop() { + self.rebase_one(old_commit)?; + } + self.update_all_references()?; + self.update_heads(); + Ok(()) } } From 7fc2e08c7d194526218738540f6cea1c5842f87d Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:18:43 -0700 Subject: [PATCH 05/10] rewrite: update heads outside `update_references()` Now that we only call `update_references()` in one place, there's no reason to have it also update `heads_to_add` and `heads_to_remove`. By moving it out of the function, we can consolidate the logic in one place. --- lib/src/rewrite.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 673d9f4b84..ce93656aa5 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -421,17 +421,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } } let (old_target, new_target) = - DescendantRebaser::ref_target_update(old_commit_id.clone(), new_commit_ids); + DescendantRebaser::ref_target_update(old_commit_id, new_commit_ids); for branch_name in &branch_updates { self.mut_repo .merge_local_branch(branch_name, &old_target, &new_target); } } - self.heads_to_add.remove(&old_commit_id); - if !self.new_commits.contains(&old_commit_id) || self.rebased.contains_key(&old_commit_id) { - self.heads_to_remove.push(old_commit_id); - } Ok(()) } @@ -523,6 +519,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } fn update_heads(&mut self) { + for old_parent_id in self.mut_repo.parent_mapping.keys() { + self.heads_to_add.remove(old_parent_id); + if !self.new_commits.contains(old_parent_id) || self.rebased.contains_key(old_parent_id) { + self.heads_to_remove.push(old_parent_id.clone()); + } + } + let mut view = self.mut_repo.view().store_view().clone(); for commit_id in &self.heads_to_remove { view.head_ids.remove(commit_id); From fc6095cd97278f6f93144c0ee1c922114dbf044a Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:39:42 -0700 Subject: [PATCH 06/10] rewrite: calculate `new_commits` later, remove it from state We only use `new_commits` in `update_heads()`, so let's calculate it there. It should also be more correct in case other commits were created after we initialized `DescendantRebaser`. --- lib/src/rewrite.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index ce93656aa5..4c1ca55e89 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -280,7 +280,6 @@ pub(crate) struct DescendantRebaser<'settings, 'repo> { mut_repo: &'repo mut MutableRepo, // In reverse order (parents after children), so we can remove the last one to rebase first. to_visit: Vec, - new_commits: HashSet, rebased: HashMap, // Names of branches where local target includes the commit id in the key. branches: HashMap>, @@ -349,13 +348,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { }, ); - 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. let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); @@ -372,7 +364,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { settings, mut_repo, to_visit, - new_commits, rebased: Default::default(), branches, heads_to_add, @@ -519,9 +510,17 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } fn update_heads(&mut self) { + let new_commits: HashSet<_> = self + .mut_repo + .parent_mapping + .values() + .flatten() + .cloned() + .collect(); + for old_parent_id in self.mut_repo.parent_mapping.keys() { self.heads_to_add.remove(old_parent_id); - if !self.new_commits.contains(old_parent_id) || self.rebased.contains_key(old_parent_id) { + if !new_commits.contains(old_parent_id) || self.rebased.contains_key(old_parent_id) { self.heads_to_remove.push(old_parent_id.clone()); } } From 23250bfe7a8488d88226beacec7cff69c6fdb843 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:47:12 -0700 Subject: [PATCH 07/10] rewrite: calculate `heads_to_remove` later, remove it from state Similar to the previous commit. --- lib/src/rewrite.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 4c1ca55e89..8ea43a8f3a 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -286,7 +286,6 @@ pub(crate) struct DescendantRebaser<'settings, 'repo> { // Parents of rebased/abandoned commit that should become new heads once their descendants // have been rebased. heads_to_add: HashSet, - heads_to_remove: Vec, // Options to apply during a rebase. options: RebaseOptions, @@ -367,7 +366,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { rebased: Default::default(), branches, heads_to_add, - heads_to_remove: Default::default(), options: Default::default(), } } @@ -518,21 +516,21 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .cloned() .collect(); + let mut heads_to_remove = vec![]; for old_parent_id in self.mut_repo.parent_mapping.keys() { self.heads_to_add.remove(old_parent_id); if !new_commits.contains(old_parent_id) || self.rebased.contains_key(old_parent_id) { - self.heads_to_remove.push(old_parent_id.clone()); + heads_to_remove.push(old_parent_id.clone()); } } let mut view = self.mut_repo.view().store_view().clone(); - for commit_id in &self.heads_to_remove { + for commit_id in &heads_to_remove { view.head_ids.remove(commit_id); } for commit_id in &self.heads_to_add { view.head_ids.insert(commit_id.clone()); } - self.heads_to_remove.clear(); self.heads_to_add.clear(); self.mut_repo.set_view(view); } From 89ed0cd04c9355adf707346fb9c6a04e67ffe753 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:50:42 -0700 Subject: [PATCH 08/10] rewrite: calculate `heads_to_add` later, remove it from state Similar to the previous two commits. --- lib/src/rewrite.rs | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 8ea43a8f3a..bd6d169b0f 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -283,9 +283,6 @@ pub(crate) struct DescendantRebaser<'settings, 'repo> { rebased: HashMap, // Names of branches where local target includes the commit id in the key. branches: HashMap>, - // Parents of rebased/abandoned commit that should become new heads once their descendants - // have been rebased. - heads_to_add: HashSet, // Options to apply during a rebase. options: RebaseOptions, @@ -307,15 +304,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { 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); - let heads_to_add = heads_to_add_expression - .evaluate_programmatic(mut_repo) - .unwrap() - .iter() - .collect(); - let to_visit_expression = old_commits_expression.descendants(); let to_visit_revset = to_visit_expression.evaluate_programmatic(mut_repo).unwrap(); let to_visit: Vec<_> = to_visit_revset.iter().commits(store).try_collect().unwrap(); @@ -365,7 +353,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { to_visit, rebased: Default::default(), branches, - heads_to_add, options: Default::default(), } } @@ -516,9 +503,23 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { .cloned() .collect(); + let old_commits_expression = + RevsetExpression::commits(self.mut_repo.parent_mapping.keys().cloned().collect()) + .union(&RevsetExpression::commits( + self.mut_repo.abandoned.iter().cloned().collect(), + )); + let heads_to_add_expression = old_commits_expression + .parents() + .minus(&old_commits_expression); + let mut heads_to_add: HashSet<_> = heads_to_add_expression + .evaluate_programmatic(self.mut_repo) + .unwrap() + .iter() + .collect(); + let mut heads_to_remove = vec![]; for old_parent_id in self.mut_repo.parent_mapping.keys() { - self.heads_to_add.remove(old_parent_id); + heads_to_add.remove(old_parent_id); if !new_commits.contains(old_parent_id) || self.rebased.contains_key(old_parent_id) { heads_to_remove.push(old_parent_id.clone()); } @@ -528,10 +529,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { for commit_id in &heads_to_remove { view.head_ids.remove(commit_id); } - for commit_id in &self.heads_to_add { - view.head_ids.insert(commit_id.clone()); - } - self.heads_to_add.clear(); + view.head_ids.extend(heads_to_add); self.mut_repo.set_view(view); } From d7662c8125d75d74c779231f7a68fc51fcab3e76 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 23:57:21 -0700 Subject: [PATCH 09/10] rewrite: calculate `branches` later, remove it from state --- lib/src/rewrite.rs | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index bd6d169b0f..c594cb27f6 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -281,9 +281,6 @@ pub(crate) struct DescendantRebaser<'settings, 'repo> { // In reverse order (parents after children), so we can remove the last one to rebase first. to_visit: Vec, rebased: HashMap, - // Names of branches where local target includes the commit id in the key. - branches: HashMap>, - // Options to apply during a rebase. options: RebaseOptions, } @@ -335,24 +332,11 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { }, ); - // 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. - let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); - for (branch_name, target) in mut_repo.view().local_branches() { - for commit in target.added_ids() { - branches - .entry(commit.clone()) - .or_default() - .insert(branch_name.to_owned()); - } - } - DescendantRebaser { settings, mut_repo, to_visit, rebased: Default::default(), - branches, options: Default::default(), } } @@ -386,7 +370,21 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { 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() { + // 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. + // TODO: We no longer need to do this now that we update branches for all + // commits at once. + let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); + for (branch_name, target) in self.mut_repo.view().local_branches() { + for commit in target.added_ids() { + branches + .entry(commit.clone()) + .or_default() + .insert(branch_name.to_owned()); + } + } + + if let Some(branch_names) = branches.get(&old_commit_id).cloned() { let mut branch_updates = vec![]; for branch_name in &branch_names { let local_target = self.mut_repo.get_local_branch(branch_name); From 8d141782609d20a2c2927a7f0f39b8d0a4c47c80 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 25 Mar 2024 22:12:09 -0700 Subject: [PATCH 10/10] rewrite: exclude already rewritten commits from set to rebase We currently include the commits in `parent_mapping` and `abandoned` in the set of commits to visit when rebasing descendants. The reason was that we used to update branches and working copies when we visited these commits. Since we started updating refs after rebasing all commits, there's no need to even visit these commits. --- lib/src/rewrite.rs | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index c594cb27f6..7dd63b5ea0 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -301,7 +301,9 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { RevsetExpression::commits(mut_repo.parent_mapping.keys().cloned().collect()).union( &RevsetExpression::commits(mut_repo.abandoned.iter().cloned().collect()), ); - let to_visit_expression = old_commits_expression.descendants(); + let to_visit_expression = old_commits_expression + .descendants() + .minus(&old_commits_expression); let to_visit_revset = to_visit_expression.evaluate_programmatic(mut_repo).unwrap(); let to_visit: Vec<_> = to_visit_revset.iter().commits(store).try_collect().unwrap(); drop(to_visit_revset); @@ -439,12 +441,7 @@ 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 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. - return Ok(()); - } + assert!(!self.mut_repo.parent_mapping.contains_key(&old_commit_id)); let old_parent_ids = old_commit.parent_ids(); let new_parent_ids = self.mut_repo.new_parents(old_parent_ids); if new_parent_ids == old_parent_ids {