rewrite: move calculation of set to rebase to MutableRepo

This lets us make `parent_mapping` private again.

lib/src/repo.rs

@@ -50,7 +50,7 @@ use crate::operation::Operation;
 use crate::refs::{
     diff_named_ref_targets, diff_named_remote_refs, merge_ref_targets, merge_remote_refs,
 };
-use crate::revset::RevsetExpression;
+use crate::revset::{RevsetExpression, RevsetIteratorExt};
 use crate::rewrite::{DescendantRebaser, RebaseOptions};
 use crate::settings::{RepoSettings, UserSettings};
 use crate::signing::{SignInitError, Signer};
@@ -772,7 +772,6 @@ pub struct MutableRepo {
     base_repo: Arc<ReadonlyRepo>,
     index: Box<dyn MutableIndex>,
     view: DirtyCell<View>,
-    // TODO: make these fields private again
     // The commit identified by the key has been replaced by all the ones in the value.
     // * Branches pointing to the old commit should be updated to the new commit, resulting in a
     //   conflict if there multiple new commits.
@@ -781,7 +780,7 @@ pub struct MutableRepo {
     // * Working copies pointing to the old commit should be updated to the first of the new
     //   commits. However, if the type is `Abandoned`, a new working-copy commit should be created
     //   on top of all of the new commits instead.
-    pub(crate) parent_mapping: HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
+    parent_mapping: HashMap<CommitId, (RewriteType, Vec<CommitId>)>,
 }
 
 impl MutableRepo {
@@ -1052,25 +1051,21 @@ impl MutableRepo {
                 new_commit_ids,
             );
             for branch_name in &branch_updates {
-                self
-                    .merge_local_branch(branch_name, &old_target, &new_target);
+                self.merge_local_branch(branch_name, &old_target, &new_target);
             }
         }
 
         Ok(())
     }
 
-
     fn update_wc_commits(
         &mut self,
         settings: &UserSettings,
         old_commit_id: &CommitId,
         new_commit_id: &CommitId,
         abandoned_old_commit: bool,
     ) -> Result<(), BackendError> {
-        let workspaces_to_update = self
-            .view()
-            .workspaces_for_wc_commit_id(old_commit_id);
+        let workspaces_to_update = self.view().workspaces_for_wc_commit_id(old_commit_id);
         if workspaces_to_update.is_empty() {
             return Ok(());
         }
@@ -1079,13 +1074,12 @@ impl MutableRepo {
         let new_wc_commit = if !abandoned_old_commit {
             new_commit
         } else {
-            self
-                .new_commit(
-                    settings,
-                    vec![new_commit.id().clone()],
-                    new_commit.tree_id().clone(),
-                )
-                .write()?
+            self.new_commit(
+                settings,
+                vec![new_commit.id().clone()],
+                new_commit.tree_id().clone(),
+            )
+            .write()?
         };
         for workspace_id in workspaces_to_update.into_iter() {
             self.edit(workspace_id, &new_wc_commit).unwrap();
@@ -1112,6 +1106,47 @@ impl MutableRepo {
         self.set_view(view);
     }
 
+    /// Find descendants of commits in `parent_mapping` and then return them in
+    /// an order they should be rebased in. The result is in reverse order
+    /// so the next value can be removed from the end.
+    fn find_descendants_to_rebase(&self) -> Vec<Commit> {
+        let store = self.store();
+        let old_commits_expression =
+            RevsetExpression::commits(self.parent_mapping.keys().cloned().collect());
+        let to_visit_expression = old_commits_expression
+            .descendants()
+            .minus(&old_commits_expression);
+        let to_visit_revset = to_visit_expression.evaluate_programmatic(self).unwrap();
+        let to_visit: Vec<_> = to_visit_revset.iter().commits(store).try_collect().unwrap();
+        drop(to_visit_revset);
+        let to_visit_set: HashSet<CommitId> =
+            to_visit.iter().map(|commit| commit.id().clone()).collect();
+        let mut visited = HashSet::new();
+        // Calculate an order where we rebase parents first, but if the parents were
+        // rewritten, make sure we rebase the rewritten parent first.
+        dag_walk::topo_order_reverse(
+            to_visit,
+            |commit| commit.id().clone(),
+            |commit| {
+                visited.insert(commit.id().clone());
+                let mut dependents = vec![];
+                for parent in commit.parents() {
+                    if let Some((_, targets)) = self.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());
+                            }
+                        }
+                    }
+                    if to_visit_set.contains(parent.id()) {
+                        dependents.push(parent);
+                    }
+                }
+                dependents
+            },
+        )
+    }
+
     /// After the rebaser returned by this function is dropped,
     /// self.parent_mapping needs to be cleared.
     fn rebase_descendants_return_rebaser<'settings, 'repo>(
@@ -1123,7 +1158,9 @@ impl MutableRepo {
             // Optimization
             return Ok(None);
         }
-        let mut rebaser = DescendantRebaser::new(settings, self);
+
+        let to_visit = self.find_descendants_to_rebase();
+        let mut rebaser = DescendantRebaser::new(settings, self, to_visit);
         *rebaser.mut_options() = options;
         rebaser.rebase_all()?;
         Ok(Some(rebaser))

lib/src/rewrite.rs

@@ -24,14 +24,12 @@ use tracing::instrument;
 
 use crate::backend::{BackendError, BackendResult, CommitId, MergedTreeId};
 use crate::commit::Commit;
-use crate::dag_walk;
 use crate::index::Index;
 use crate::matchers::{Matcher, Visit};
 use crate::merged_tree::{MergedTree, MergedTreeBuilder};
 use crate::object_id::ObjectId;
 use crate::repo::{MutableRepo, Repo};
 use crate::repo_path::RepoPath;
-use crate::revset::{RevsetExpression, RevsetIteratorExt};
 use crate::settings::UserSettings;
 use crate::store::Store;
 
@@ -290,43 +288,8 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
     pub fn new(
         settings: &'settings UserSettings,
         mut_repo: &'repo mut MutableRepo,
+        to_visit: Vec<Commit>,
     ) -> DescendantRebaser<'settings, 'repo> {
-        let store = mut_repo.store();
-        let old_commits_expression =
-            RevsetExpression::commits(mut_repo.parent_mapping.keys().cloned().collect());
-        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);
-        let to_visit_set: HashSet<CommitId> =
-            to_visit.iter().map(|commit| commit.id().clone()).collect();
-        let mut visited = HashSet::new();
-        // Calculate an order where we rebase parents first, but if the parents were
-        // rewritten, make sure we rebase the rewritten parent first.
-        let to_visit = dag_walk::topo_order_reverse(
-            to_visit,
-            |commit| commit.id().clone(),
-            |commit| {
-                visited.insert(commit.id().clone());
-                let mut dependents = vec![];
-                for parent in commit.parents() {
-                    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());
-                            }
-                        }
-                    }
-                    if to_visit_set.contains(parent.id()) {
-                        dependents.push(parent);
-                    }
-                }
-                dependents
-            },
-        );
-
         DescendantRebaser {
             settings,
             mut_repo,