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

@@ -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<Option<DescendantRebaser<'settings, 'repo>>> {
-        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<usize> {
-        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<HashMap<CommitId, CommitId>> {
-        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<CommitId, CommitId> = 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.

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<Commit>,
-    rebased: HashMap<CommitId, CommitId>,
-    // 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<Commit>,
-    ) -> 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<CommitId, CommitId> {
-        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.