From 18faaf72a3e1ec02436157d5e9266371149a9f2e Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Sun, 10 Nov 2024 23:49:25 +0800 Subject: [PATCH] rewrite: remove `DescendantRebaser` Due to the gradual rewrite to use the `MutableRepo::transform_descendants` API, `DescendantRebaser` is no longer used in `MutableRepo::rebase_descendants`, and is only used (indirectly through `MutableRepo::rebase_descendants_return_map`) in `rewrite::squash_commits`. `DescendantRebaser` is removed since it contains a lot of logic similar to `MutableRepo::transform_descendants`. Instead, `MutableRepo::rebase_descendants_with_options_return_map` is rewritten to use `MutableRepo::transform_descendants` directly, and the other `MutableRepo::rebase_descendants_{return_map,with_options}` functions call `MutableRepo::rebase_descendants_with_options_return_map` directly. `MutableRepo::rebase_descendants_return_rebaser` is also removed. --- lib/src/repo.rs | 56 ++++++++++++++---------------------- lib/src/rewrite.rs | 72 ---------------------------------------------- 2 files changed, 22 insertions(+), 106 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index d90e8ce39..f8ec2e981 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -81,9 +81,10 @@ use crate::revset; use crate::revset::RevsetExpression; use crate::revset::RevsetIteratorExt; use crate::rewrite::merge_commit_trees; +use crate::rewrite::rebase_commit_with_options; use crate::rewrite::CommitRewriter; -use crate::rewrite::DescendantRebaser; use crate::rewrite::RebaseOptions; +use crate::rewrite::RebasedCommit; use crate::settings::RepoSettings; use crate::settings::UserSettings; use crate::signing::SignInitError; @@ -1270,26 +1271,6 @@ impl MutableRepo { Ok(()) } - /// After the rebaser returned by this function is dropped, - /// self.parent_mapping needs to be cleared. - fn rebase_descendants_return_rebaser<'settings, 'repo>( - &'repo mut self, - settings: &'settings UserSettings, - options: RebaseOptions, - ) -> BackendResult>> { - if !self.has_rewrites() { - // Optimization - return Ok(None); - } - - let to_visit = - self.find_descendants_to_rebase(self.parent_mapping.keys().cloned().collect())?; - let mut rebaser = DescendantRebaser::new(settings, self, to_visit); - *rebaser.mut_options() = options; - rebaser.rebase_all()?; - Ok(Some(rebaser)) - } - // TODO(ilyagr): Either document that this also moves bookmarks (rename the // function and the related functions?) or change things so that this only // rebases descendants. @@ -1298,11 +1279,10 @@ impl MutableRepo { settings: &UserSettings, options: RebaseOptions, ) -> BackendResult { - let result = self - .rebase_descendants_return_rebaser(settings, options)? - .map_or(0, |rebaser| rebaser.into_map().len()); - self.parent_mapping.clear(); - Ok(result) + let num_rebased = self + .rebase_descendants_with_options_return_map(settings, options)? + .len(); + Ok(num_rebased) } /// This is similar to `rebase_descendants_return_map`, but the return value @@ -1318,20 +1298,28 @@ impl MutableRepo { /// **its parent**. One can tell this case apart since the change ids of the /// key and the value will not match. The parent will inherit the /// descendants and the bookmarks of the abandoned commit. - // TODO: Rewrite this using `transform_descendants()` pub fn rebase_descendants_with_options_return_map( &mut self, settings: &UserSettings, options: RebaseOptions, ) -> BackendResult> { - let result = Ok(self - // We do not set RebaseOptions here, since this function does not currently return - // enough information to describe the results of a rebase if some commits got - // abandoned - .rebase_descendants_return_rebaser(settings, options)? - .map_or(HashMap::new(), |rebaser| rebaser.into_map())); + let mut rebased: HashMap = HashMap::new(); + let roots = self.parent_mapping.keys().cloned().collect_vec(); + self.transform_descendants(settings, roots, |rewriter| { + if rewriter.parents_changed() { + let old_commit_id = rewriter.old_commit().id().clone(); + let rebased_commit: RebasedCommit = + rebase_commit_with_options(settings, rewriter, &options)?; + let new_commit = match rebased_commit { + RebasedCommit::Rewritten(new_commit) => new_commit, + RebasedCommit::Abandoned { parent } => parent, + }; + rebased.insert(old_commit_id, new_commit.id().clone()); + } + Ok(()) + })?; self.parent_mapping.clear(); - result + Ok(rebased) } /// Rebase descendants of the rewritten commits. diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 34d351892..6806159a2 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -387,78 +387,6 @@ pub struct RebaseOptions { pub simplify_ancestor_merge: bool, } -pub(crate) struct DescendantRebaser<'settings, 'repo> { - settings: &'settings UserSettings, - mut_repo: &'repo mut MutableRepo, - // In reverse order (parents after children), so we can remove the last one to rebase first. - to_visit: Vec, - rebased: HashMap, - // Options to apply during a rebase. - options: RebaseOptions, -} - -impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { - /// Panics if any commit is rewritten to its own descendant. - /// - /// There should not be any cycles in the `rewritten` map (e.g. A is - /// rewritten to B, which is rewritten to A). The same commit should not - /// be rewritten and abandoned at the same time. In either case, panics are - /// likely when using the DescendantRebaser. - pub fn new( - settings: &'settings UserSettings, - mut_repo: &'repo mut MutableRepo, - to_visit: Vec, - ) -> DescendantRebaser<'settings, 'repo> { - DescendantRebaser { - settings, - mut_repo, - to_visit, - rebased: Default::default(), - options: Default::default(), - } - } - - /// Returns options that can be set. - pub fn mut_options(&mut self) -> &mut RebaseOptions { - &mut self.options - } - - /// Returns a map from `CommitId` of old commit to new commit. Includes the - /// commits rebase so far. Does not include the inputs passed to - /// `rebase_descendants`. - pub fn into_map(self) -> HashMap { - self.rebased - } - - fn rebase_one(&mut self, old_commit: Commit) -> BackendResult<()> { - let old_commit_id = old_commit.id().clone(); - let old_parent_ids = old_commit.parent_ids(); - let new_parent_ids = self.mut_repo.new_parents(old_parent_ids); - let rewriter = CommitRewriter::new(self.mut_repo, old_commit, new_parent_ids); - if !rewriter.parents_changed() { - // The commit is already in place. - return Ok(()); - } - - let rebased_commit: RebasedCommit = - rebase_commit_with_options(self.settings, rewriter, &self.options)?; - let new_commit = match rebased_commit { - RebasedCommit::Rewritten(new_commit) => new_commit, - RebasedCommit::Abandoned { parent } => parent, - }; - self.rebased - .insert(old_commit_id.clone(), new_commit.id().clone()); - Ok(()) - } - - pub fn rebase_all(&mut self) -> BackendResult<()> { - while let Some(old_commit) = self.to_visit.pop() { - self.rebase_one(old_commit)?; - } - self.mut_repo.update_rewritten_references(self.settings) - } -} - #[derive(Default)] pub struct MoveCommitsStats { /// The number of commits in the target set which were rebased.