Make DescendantRebaser use rewrite state from MutableRepo

lib/src/repo.rs

@@ -732,8 +732,17 @@ pub struct MutableRepo {
     base_repo: Arc<ReadonlyRepo>,
     index: Box<dyn MutableIndex>,
     view: DirtyCell<View>,
-    rewritten_commits: HashMap<CommitId, Vec<CommitId>>,
-    abandoned_commits: HashSet<CommitId>,
+    // TODO: make these fields private again
+    // The commit identified by the key has been replaced by all the ones in the value, typically
+    // because the key commit was abandoned (the value commits are then the abandoned commit's
+    // parents). A child of the key commit should be rebased onto all the value commits. A branch
+    // pointing to the key commit should become a conflict pointing to all the value commits.
+    pub(crate) parent_mapping: HashMap<CommitId, Vec<CommitId>>,
+    /// Commits with a key in `parent_mapping` that have been divergently
+    /// rewritten into all the commits indicated by the value.
+    pub(crate) divergent: HashSet<CommitId>,
+    // Commits that were abandoned. Their children should be rebased onto their parents.
+    pub(crate) abandoned: HashSet<CommitId>,
 }
 
 impl MutableRepo {
@@ -748,8 +757,9 @@ impl MutableRepo {
             base_repo,
             index: mut_index,
             view: DirtyCell::with_clean(mut_view),
-            rewritten_commits: Default::default(),
-            abandoned_commits: Default::default(),
+            parent_mapping: Default::default(),
+            divergent: Default::default(),
+            abandoned: Default::default(),
         }
     }
 
@@ -766,8 +776,8 @@ impl MutableRepo {
     }
 
     pub fn has_changes(&self) -> bool {
-        !(self.abandoned_commits.is_empty()
-            && self.rewritten_commits.is_empty()
+        !(self.abandoned.is_empty()
+            && self.parent_mapping.is_empty()
             && self.view() == &self.base_repo.view)
     }
 
@@ -816,7 +826,8 @@ impl MutableRepo {
     /// docstring for `record_rewritten_commit` for details.
     pub fn set_rewritten_commit(&mut self, old_id: CommitId, new_id: CommitId) {
         assert_ne!(old_id, *self.store().root_commit_id());
-        self.rewritten_commits.insert(old_id, vec![new_id]);
+        self.divergent.remove(&old_id);
+        self.parent_mapping.insert(old_id, vec![new_id]);
     }
 
     /// Record a commit as being rewritten into multiple other commits in this
@@ -832,8 +843,9 @@ impl MutableRepo {
         new_ids: impl IntoIterator<Item = CommitId>,
     ) {
         assert_ne!(old_id, *self.store().root_commit_id());
-        self.rewritten_commits
-            .insert(old_id, new_ids.into_iter().collect());
+        self.parent_mapping
+            .insert(old_id.clone(), new_ids.into_iter().collect());
+        self.divergent.insert(old_id);
     }
 
     /// Record a commit as having been abandoned in this transaction.
@@ -847,16 +859,17 @@ impl MutableRepo {
     /// `old_id` would be moved to the parent(s) of `old_id` as well.
     pub fn record_abandoned_commit(&mut self, old_id: CommitId) {
         assert_ne!(old_id, *self.store().root_commit_id());
-        self.abandoned_commits.insert(old_id);
+        self.abandoned.insert(old_id);
     }
 
     fn clear_descendant_rebaser_plans(&mut self) {
-        self.rewritten_commits.clear();
-        self.abandoned_commits.clear();
+        self.parent_mapping.clear();
+        self.divergent.clear();
+        self.abandoned.clear();
     }
 
     pub fn has_rewrites(&self) -> bool {
-        !(self.rewritten_commits.is_empty() && self.abandoned_commits.is_empty())
+        !(self.parent_mapping.is_empty() && self.abandoned.is_empty())
     }
 
     /// After the rebaser returned by this function is dropped,
@@ -870,12 +883,7 @@ impl MutableRepo {
             // Optimization
             return Ok(None);
         }
-        let mut rebaser = DescendantRebaser::new(
-            settings,
-            self,
-            self.rewritten_commits.clone(),
-            self.abandoned_commits.clone(),
-        );
+        let mut rebaser = DescendantRebaser::new(settings, self);
         *rebaser.mut_options() = options;
         rebaser.rebase_all()?;
         Ok(Some(rebaser))

lib/src/rewrite.rs

@@ -278,18 +278,8 @@ pub struct RebaseOptions {
 pub(crate) struct DescendantRebaser<'settings, 'repo> {
     settings: &'settings UserSettings,
     mut_repo: &'repo mut MutableRepo,
-    // The commit identified by the key has been replaced by all the ones in the value, typically
-    // because the key commit was abandoned (the value commits are then the abandoned commit's
-    // parents). A child of the key commit should be rebased onto all the value commits. A branch
-    // pointing to the key commit should become a conflict pointing to all the value commits.
-    parent_mapping: HashMap<CommitId, Vec<CommitId>>,
-    divergent: HashSet<CommitId>,
     // In reverse order (parents after children), so we can remove the last one to rebase first.
     to_visit: Vec<Commit>,
-    // Commits to visit but skip. These were also in `to_visit` to start with, but we don't
-    // want to rebase them. Instead, we record them in `parent_mapping` when we visit them. That
-    // way, their descendants will be rebased correctly.
-    abandoned: HashSet<CommitId>,
     new_commits: HashSet<CommitId>,
     rebased: HashMap<CommitId, CommitId>,
     // Names of branches where local target includes the commit id in the key.
@@ -313,17 +303,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
     pub fn new(
         settings: &'settings UserSettings,
         mut_repo: &'repo mut MutableRepo,
-        rewritten: HashMap<CommitId, Vec<CommitId>>,
-        abandoned: HashSet<CommitId>,
     ) -> DescendantRebaser<'settings, 'repo> {
         let store = mut_repo.store();
-        let root_commit_id = store.root_commit_id();
-        assert!(!abandoned.contains(root_commit_id));
-        assert!(!rewritten.contains_key(root_commit_id));
-        let old_commits_expression = RevsetExpression::commits(rewritten.keys().cloned().collect())
-            .union(&RevsetExpression::commits(
-                abandoned.iter().cloned().collect(),
-            ));
+        let old_commits_expression =
+            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);
@@ -349,7 +334,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
                 visited.insert(commit.id().clone());
                 let mut dependents = vec![];
                 for parent in commit.parents() {
-                    if let Some(targets) = rewritten.get(parent.id()) {
+                    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());
@@ -364,19 +349,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
             },
         );
 
-        let new_commits = rewritten.values().flatten().cloned().collect();
-
-        let mut parent_mapping = HashMap::new();
-        let mut divergent = HashSet::new();
-        for (old_commit, new_commits) in rewritten {
-            if new_commits.len() == 1 {
-                parent_mapping.insert(old_commit, vec![new_commits.into_iter().next().unwrap()]);
-            } else {
-                let new_commits = mut_repo.index().heads(&mut new_commits.iter());
-                parent_mapping.insert(old_commit.clone(), new_commits);
-                divergent.insert(old_commit);
-            }
-        }
+        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.
@@ -393,10 +371,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
         DescendantRebaser {
             settings,
             mut_repo,
-            parent_mapping,
-            divergent,
             to_visit,
-            abandoned,
             new_commits,
             rebased: Default::default(),
             branches,
@@ -457,11 +432,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
         let mut new_ids: Vec<CommitId> = 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();
+        let mut allowed_iterations = 0..self.mut_repo.parent_mapping.len();
         loop {
             let made_replacements;
-            (new_ids, made_replacements) =
-                single_substitution_round(&self.parent_mapping, &self.divergent, new_ids);
+            (new_ids, made_replacements) = single_substitution_round(
+                &self.mut_repo.parent_mapping,
+                &self.mut_repo.divergent,
+                new_ids,
+            );
             if !made_replacements {
                 break;
             }
@@ -492,18 +470,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
         new_commit_ids: Vec<CommitId>,
     ) -> Result<(), BackendError> {
         // We arbitrarily pick a new working-copy commit among the candidates.
-        let abandoned_old_commit = self.abandoned.contains(&old_commit_id);
+        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() {
             let mut branch_updates = vec![];
             for branch_name in &branch_names {
-                for new_commit_id in &new_commit_ids {
-                    self.branches
-                        .entry(new_commit_id.clone())
-                        .or_default()
-                        .insert(branch_name.clone());
-                }
                 let local_target = self.mut_repo.get_local_branch(branch_name);
                 for old_add in local_target.added_ids() {
                     if *old_add == old_commit_id {
@@ -560,26 +532,33 @@ 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 let Some(new_parent_ids) = self.parent_mapping.get(&old_commit_id).cloned() {
+        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, but we still want to update branches pointing
-            // to the old commit.
-            self.update_references(old_commit_id, new_parent_ids)?;
+            // to rebase it.
             return Ok(());
         }
         let old_parent_ids = old_commit.parent_ids();
         let new_parent_ids = self.new_parents(old_parent_ids);
-        if self.abandoned.contains(&old_commit_id) {
+        if self.mut_repo.abandoned.contains(&old_commit_id) {
             // Update the `new_parents` map so descendants are rebased correctly.
-            self.parent_mapping
+            self.mut_repo
+                .parent_mapping
                 .insert(old_commit_id.clone(), new_parent_ids.clone());
-            self.update_references(old_commit_id, new_parent_ids)?;
             return Ok(());
-        } else if new_parent_ids == old_parent_ids {
+        }
+        if new_parent_ids == old_parent_ids {
             // The commit is already in place.
             return Ok(());
         }
+        assert_eq!(
+            (
+                self.rebased.get(&old_commit_id),
+                self.mut_repo.parent_mapping.get(&old_commit_id)
+            ),
+            (None, None),
+            "Trying to rebase the same commit {old_commit_id:?} in two different ways",
+        );
 
         let new_parents: Vec<_> = new_parent_ids
             .iter()
@@ -595,29 +574,35 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> {
         let new_commit = match rebased_commit {
             RebasedCommit::Rewritten(new_commit) => new_commit,
             RebasedCommit::Abandoned { parent } => {
-                self.abandoned.insert(old_commit.id().clone());
+                self.mut_repo
+                    .parent_mapping
+                    .insert(old_commit_id.clone(), vec![parent.id().clone()]);
+                self.mut_repo.abandoned.insert(old_commit.id().clone());
                 parent
             }
         };
-        let previous_rebased_value = self
-            .rebased
+        self.rebased
             .insert(old_commit_id.clone(), new_commit.id().clone());
-        let previous_mapping_value = self
-            .parent_mapping
-            .insert(old_commit_id.clone(), vec![new_commit.id().clone()]);
-        assert_eq!(
-            (previous_rebased_value, previous_mapping_value),
-            (None, None),
-            "Trying to rebase the same commit {old_commit_id:?} in two different ways",
-        );
-        self.update_references(old_commit_id, vec![new_commit.id().clone()])?;
+        Ok(())
+    }
+
+    fn update_all_references(&mut self) -> Result<(), BackendError> {
+        for (old_parent_id, new_parent_ids) in self.mut_repo.parent_mapping.clone() {
+            // Call `new_parents()` here since `parent_mapping` only contains direct
+            // 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);
+            self.update_references(old_parent_id, new_parent_ids)?;
+        }
         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()?;
         let mut view = self.mut_repo.view().store_view().clone();
         for commit_id in &self.heads_to_remove {
             view.head_ids.remove(commit_id);

lib/tests/test_rewrite.rs

@@ -707,12 +707,12 @@ fn test_rebase_descendants_multiple_swap() {
         .set_rewritten_commit(commit_b.id().clone(), commit_d.id().clone());
     tx.mut_repo()
         .set_rewritten_commit(commit_d.id().clone(), commit_b.id().clone());
-    let _ = tx.mut_repo().rebase_descendants_return_map(&settings); // Panics because of the cycle
+    let _ = tx.mut_repo().rebase_descendants(&settings); // Panics because of
+                                                         // the cycle
 }
 
-// Unlike `test_rebase_descendants_multiple_swap`, this does not currently
-// panic, but it would probably be OK if it did.
 #[test]
+#[should_panic(expected = "cycle detected")]
 fn test_rebase_descendants_multiple_no_descendants() {
     let settings = testutils::user_settings();
     let test_repo = TestRepo::init();
@@ -733,19 +733,8 @@ fn test_rebase_descendants_multiple_no_descendants() {
         .set_rewritten_commit(commit_b.id().clone(), commit_c.id().clone());
     tx.mut_repo()
         .set_rewritten_commit(commit_c.id().clone(), commit_b.id().clone());
-    let rebase_map = tx
-        .mut_repo()
-        .rebase_descendants_return_map(&settings)
-        .unwrap();
-    assert!(rebase_map.is_empty());
-
-    assert_eq!(
-        *tx.mut_repo().view().heads(),
-        hashset! {
-            commit_b.id().clone(),
-            commit_c.id().clone()
-        }
-    );
+    let _ = tx.mut_repo().rebase_descendants(&settings); // Panics because of
+                                                         // the cycle
 }
 
 #[test]
@@ -1028,11 +1017,13 @@ fn test_rebase_descendants_branch_move_two_steps() {
     let commit_b2 = tx
         .mut_repo()
         .rewrite_commit(&settings, &commit_b)
+        .set_description("different")
         .write()
         .unwrap();
     let commit_c2 = tx
         .mut_repo()
         .rewrite_commit(&settings, &commit_c)
+        .set_description("more different")
         .write()
         .unwrap();
     tx.mut_repo().rebase_descendants(&settings).unwrap();