From 0f7a86d725bfc023402736fd384a7373813a3936 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Sun, 24 Mar 2024 15:27:42 -0700 Subject: [PATCH] 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 4725b1f8a..541972dce 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 65cf7ce0c..aeb30d746 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(())