From 116761ddcc9571a6b4efc36ded5b77296ffca3d6 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Mon, 25 Mar 2024 23:23:59 -0700 Subject: [PATCH 1/6] rewrite: leverage `root_id()` helper on commit object --- lib/src/rewrite.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 1378e809b8..1d959206d6 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -148,11 +148,11 @@ pub fn rebase_commit_with_options( let old_parents = old_commit.parents(); let old_parent_trees = old_parents .iter() - .map(|parent| parent.store_commit().root_tree.clone()) + .map(|parent| parent.tree_id().clone()) .collect_vec(); let new_parent_trees = new_parents .iter() - .map(|parent| parent.store_commit().root_tree.clone()) + .map(|parent| parent.tree_id().clone()) .collect_vec(); let (old_base_tree_id, new_tree_id) = if new_parent_trees == old_parent_trees { From 9c55fcc6c510d0806bb0bccd9803f67866a7afc0 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 Apr 2024 06:35:03 -0700 Subject: [PATCH 2/6] rewrite: inline and rewrite `ref_target_update()` I rewrote `old_target` and `new_target` to more accurately represent the change; the old target should be a normal (singleton) ref. --- lib/src/rewrite.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 1d959206d6..5df7a775fb 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -349,14 +349,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.rebased } - fn ref_target_update(old_id: CommitId, new_ids: Vec) -> (RefTarget, RefTarget) { - let old_ids = std::iter::repeat(old_id).take(new_ids.len()); - ( - RefTarget::from_legacy_form([], old_ids), - RefTarget::from_legacy_form([], new_ids), - ) - } - fn update_references( &mut self, old_commit_id: CommitId, @@ -393,8 +385,13 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } } } - let (old_target, new_target) = - DescendantRebaser::ref_target_update(old_commit_id, new_commit_ids); + + let old_target = RefTarget::normal(old_commit_id.clone()); + assert!(!new_commit_ids.is_empty()); + let new_target = RefTarget::from_legacy_form( + std::iter::repeat(old_commit_id).take(new_commit_ids.len() - 1), + new_commit_ids, + ); for branch_name in &branch_updates { self.mut_repo .merge_local_branch(branch_name, &old_target, &new_target); From 199c4f9ccd6d81c2646d3e2d0d534bbe3de05483 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 Apr 2024 06:36:23 -0700 Subject: [PATCH 3/6] rewrite: pass UserSettings into `update_all_references()` With this change, `update_all_references()` only uses `self` to get to `mut_repo`. I'll move the function onto `MutableRepo` next. --- lib/src/rewrite.rs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 5df7a775fb..4e60febbc6 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -351,6 +351,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { fn update_references( &mut self, + settings: &UserSettings, old_commit_id: CommitId, new_commit_ids: Vec, ) -> Result<(), BackendError> { @@ -359,7 +360,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.mut_repo.parent_mapping.get(&old_commit_id), Some((RewriteType::Abandoned, _)) ); - self.update_wc_commits(&old_commit_id, &new_commit_ids[0], abandoned_old_commit)?; + self.update_wc_commits( + settings, + &old_commit_id, + &new_commit_ids[0], + abandoned_old_commit, + )?; // 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. @@ -403,6 +409,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { fn update_wc_commits( &mut self, + settings: &UserSettings, old_commit_id: &CommitId, new_commit_id: &CommitId, abandoned_old_commit: bool, @@ -421,7 +428,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { } else { self.mut_repo .new_commit( - self.settings, + settings, vec![new_commit.id().clone()], new_commit.tree_id().clone(), ) @@ -471,14 +478,14 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { Ok(()) } - fn update_all_references(&mut self) -> Result<(), BackendError> { + fn update_all_references(&mut self, settings: &UserSettings) -> 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.mut_repo.new_parents(&new_parent_ids); - self.update_references(old_parent_id, new_parent_ids)?; + self.update_references(settings, old_parent_id, new_parent_ids)?; } Ok(()) } @@ -506,7 +513,7 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { while let Some(old_commit) = self.to_visit.pop() { self.rebase_one(old_commit)?; } - self.update_all_references()?; + self.update_all_references(self.settings)?; self.update_heads(); Ok(()) From aedafb10d4b4df19b7214f55b33a567c0b406202 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 12 Apr 2024 09:31:02 -0700 Subject: [PATCH 4/6] rewrite: remove unnecessary assertions I think the recent refactorings (especially 9c382fd8c656) make it pretty clear that `DescendantRebaser` will not attempt to rebase the same commit twice, so I think we can remove the assertions. This removes some of the places where `DescendantRebaser` reaches into `MutableRepo`'s internals. --- lib/src/rewrite.rs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 4e60febbc6..ac919031a3 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -442,21 +442,12 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { fn rebase_one(&mut self, old_commit: Commit) -> BackendResult<()> { let old_commit_id = old_commit.id().clone(); - assert!(!self.mut_repo.parent_mapping.contains_key(&old_commit_id)); let old_parent_ids = old_commit.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(()); } - 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() From 4e9adf7e1a434002fa8971590a677da35ef70b29 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Thu, 11 Apr 2024 06:46:11 -0700 Subject: [PATCH 5/6] rewrite: move functions for updating refs to `MutableRepo` The functions now depend only on `MutableRepo`, so I think they belong on that type. This gets us closer to being able to make `parent_mapping` private again. --- lib/src/repo.rs | 126 +++++++++++++++++++++++++++++++++++++++++++ lib/src/rewrite.rs | 130 +-------------------------------------------- 2 files changed, 128 insertions(+), 128 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index e0137cf72b..0df7695153 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -50,6 +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::rewrite::{DescendantRebaser, RebaseOptions}; use crate::settings::{RepoSettings, UserSettings}; use crate::signing::{SignInitError, Signer}; @@ -981,6 +982,131 @@ impl MutableRepo { } } + /// Updates branches, working copies, and anonymous heads after rewriting + /// and/or abandoning commits. + pub fn update_rewritten_references(&mut self, settings: &UserSettings) -> BackendResult<()> { + self.update_all_references(settings)?; + self.update_heads(); + Ok(()) + } + + fn update_all_references(&mut self, settings: &UserSettings) -> Result<(), BackendError> { + for (old_parent_id, (_, new_parent_ids)) in self.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(settings, old_parent_id, new_parent_ids)?; + } + Ok(()) + } + + fn update_references( + &mut self, + settings: &UserSettings, + old_commit_id: CommitId, + new_commit_ids: Vec, + ) -> Result<(), BackendError> { + // We arbitrarily pick a new working-copy commit among the candidates. + let abandoned_old_commit = matches!( + self.parent_mapping.get(&old_commit_id), + Some((RewriteType::Abandoned, _)) + ); + self.update_wc_commits( + settings, + &old_commit_id, + &new_commit_ids[0], + abandoned_old_commit, + )?; + + // 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. + // TODO: We no longer need to do this now that we update branches for all + // commits at once. + let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); + for (branch_name, target) in self.view().local_branches() { + for commit in target.added_ids() { + branches + .entry(commit.clone()) + .or_default() + .insert(branch_name.to_owned()); + } + } + + if let Some(branch_names) = branches.get(&old_commit_id).cloned() { + let mut branch_updates = vec![]; + for branch_name in &branch_names { + let local_target = self.get_local_branch(branch_name); + for old_add in local_target.added_ids() { + if *old_add == old_commit_id { + branch_updates.push(branch_name.clone()); + } + } + } + + let old_target = RefTarget::normal(old_commit_id.clone()); + assert!(!new_commit_ids.is_empty()); + let new_target = RefTarget::from_legacy_form( + std::iter::repeat(old_commit_id).take(new_commit_ids.len() - 1), + new_commit_ids, + ); + for branch_name in &branch_updates { + 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); + if workspaces_to_update.is_empty() { + return Ok(()); + } + + let new_commit = self.store().get_commit(new_commit_id)?; + 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()? + }; + for workspace_id in workspaces_to_update.into_iter() { + self.edit(workspace_id, &new_wc_commit).unwrap(); + } + Ok(()) + } + + fn update_heads(&mut self) { + let old_commits_expression = + RevsetExpression::commits(self.parent_mapping.keys().cloned().collect()); + let heads_to_add_expression = old_commits_expression + .parents() + .minus(&old_commits_expression); + let heads_to_add = heads_to_add_expression + .evaluate_programmatic(self) + .unwrap() + .iter(); + + let mut view = self.view().store_view().clone(); + for commit_id in self.parent_mapping.keys() { + view.head_ids.remove(commit_id); + } + view.head_ids.extend(heads_to_add); + self.set_view(view); + } + /// After the rebaser returned by this function is dropped, /// self.parent_mapping needs to be cleared. fn rebase_descendants_return_rebaser<'settings, 'repo>( diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index ac919031a3..8c8b43f8e7 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -29,8 +29,7 @@ use crate::index::Index; use crate::matchers::{Matcher, Visit}; use crate::merged_tree::{MergedTree, MergedTreeBuilder}; use crate::object_id::ObjectId; -use crate::op_store::RefTarget; -use crate::repo::{MutableRepo, Repo, RewriteType}; +use crate::repo::{MutableRepo, Repo}; use crate::repo_path::RepoPath; use crate::revset::{RevsetExpression, RevsetIteratorExt}; use crate::settings::UserSettings; @@ -349,97 +348,6 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { self.rebased } - fn update_references( - &mut self, - settings: &UserSettings, - old_commit_id: CommitId, - new_commit_ids: Vec, - ) -> Result<(), BackendError> { - // We arbitrarily pick a new working-copy commit among the candidates. - let abandoned_old_commit = matches!( - self.mut_repo.parent_mapping.get(&old_commit_id), - Some((RewriteType::Abandoned, _)) - ); - self.update_wc_commits( - settings, - &old_commit_id, - &new_commit_ids[0], - abandoned_old_commit, - )?; - - // 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. - // TODO: We no longer need to do this now that we update branches for all - // commits at once. - let mut branches: HashMap<_, HashSet<_>> = HashMap::new(); - for (branch_name, target) in self.mut_repo.view().local_branches() { - for commit in target.added_ids() { - branches - .entry(commit.clone()) - .or_default() - .insert(branch_name.to_owned()); - } - } - - if let Some(branch_names) = branches.get(&old_commit_id).cloned() { - let mut branch_updates = vec![]; - for branch_name in &branch_names { - 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 { - branch_updates.push(branch_name.clone()); - } - } - } - - let old_target = RefTarget::normal(old_commit_id.clone()); - assert!(!new_commit_ids.is_empty()); - let new_target = RefTarget::from_legacy_form( - std::iter::repeat(old_commit_id).take(new_commit_ids.len() - 1), - new_commit_ids, - ); - for branch_name in &branch_updates { - self.mut_repo - .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 - .mut_repo - .view() - .workspaces_for_wc_commit_id(old_commit_id); - if workspaces_to_update.is_empty() { - return Ok(()); - } - - let new_commit = self.mut_repo.store().get_commit(new_commit_id)?; - let new_wc_commit = if !abandoned_old_commit { - new_commit - } else { - self.mut_repo - .new_commit( - settings, - vec![new_commit.id().clone()], - new_commit.tree_id().clone(), - ) - .write()? - }; - for workspace_id in workspaces_to_update.into_iter() { - self.mut_repo.edit(workspace_id, &new_wc_commit).unwrap(); - } - Ok(()) - } - 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(); @@ -469,44 +377,10 @@ impl<'settings, 'repo> DescendantRebaser<'settings, 'repo> { Ok(()) } - fn update_all_references(&mut self, settings: &UserSettings) -> 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.mut_repo.new_parents(&new_parent_ids); - self.update_references(settings, old_parent_id, new_parent_ids)?; - } - Ok(()) - } - - fn update_heads(&mut self) { - let old_commits_expression = - RevsetExpression::commits(self.mut_repo.parent_mapping.keys().cloned().collect()); - let heads_to_add_expression = old_commits_expression - .parents() - .minus(&old_commits_expression); - let heads_to_add = heads_to_add_expression - .evaluate_programmatic(self.mut_repo) - .unwrap() - .iter(); - - let mut view = self.mut_repo.view().store_view().clone(); - for commit_id in self.mut_repo.parent_mapping.keys() { - view.head_ids.remove(commit_id); - } - view.head_ids.extend(heads_to_add); - self.mut_repo.set_view(view); - } - pub fn rebase_all(&mut self) -> BackendResult<()> { while let Some(old_commit) = self.to_visit.pop() { self.rebase_one(old_commit)?; } - self.update_all_references(self.settings)?; - self.update_heads(); - - Ok(()) + self.mut_repo.update_rewritten_references(self.settings) } } From 9961babdb72d49972b962af998b3928dfbcd3b70 Mon Sep 17 00:00:00 2001 From: Martin von Zweigbergk Date: Fri, 12 Apr 2024 09:49:38 -0700 Subject: [PATCH 6/6] rewrite: move calculation of set to rebase to `MutableRepo` This lets us make `parent_mapping` private again. --- lib/src/repo.rs | 50 ++++++++++++++++++++++++++++++++++++++++++---- lib/src/rewrite.rs | 39 +----------------------------------- 2 files changed, 47 insertions(+), 42 deletions(-) diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 0df7695153..4643312a6a 100644 --- a/lib/src/repo.rs +++ b/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, index: Box, view: DirtyCell, - // 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)>, + parent_mapping: HashMap)>, } impl MutableRepo { @@ -1107,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 { + 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 = + 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>( @@ -1118,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)) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index 8c8b43f8e7..2225ad9175 100644 --- a/lib/src/rewrite.rs +++ b/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, ) -> 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 = - 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,