From b9e4e4fe8a3850e8ab9f2e05a7ce3355fa3553b3 Mon Sep 17 00:00:00 2001 From: Yuya Nishihara Date: Fri, 9 May 2025 11:58:36 +0900 Subject: [PATCH] rewrite: split move_commits() into immutable/mutable parts This makes it clear that the repo state isn't mutated during computation of the destination and descendants. Still compute_move_commits() is big, so we might want to split it further. If needed, maybe we can add variants of apply_move_commits() to fix up intermediate predecessor chains? compute_move_commits() needs (immutable) &MutableRepo because it depends on repo.find_descendants_for_rebase(). --- lib/src/rewrite.rs | 72 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 54 insertions(+), 18 deletions(-) diff --git a/lib/src/rewrite.rs b/lib/src/rewrite.rs index e0097d47d..7068c9f65 100644 --- a/lib/src/rewrite.rs +++ b/lib/src/rewrite.rs @@ -389,7 +389,6 @@ pub struct RewriteRefsOptions { pub delete_abandoned_bookmarks: bool, } -#[derive(Default)] pub struct MoveCommitsStats { /// The number of commits in the target set which were rebased. pub num_rebased_targets: u32, @@ -419,6 +418,23 @@ pub enum MoveCommitsTarget { Roots(Vec), } +#[derive(Clone, Debug)] +struct ComputedMoveCommits { + target_commit_ids: IndexSet, + descendants: Vec, + commit_new_parents_map: HashMap>, +} + +impl ComputedMoveCommits { + fn empty() -> Self { + ComputedMoveCommits { + target_commit_ids: IndexSet::new(), + descendants: vec![], + commit_new_parents_map: HashMap::new(), + } + } +} + /// Moves `loc.target` commits from their current location to a new location in /// the graph. /// @@ -432,6 +448,14 @@ pub fn move_commits( loc: &MoveCommitsLocation, options: &RebaseOptions, ) -> BackendResult { + let commits = compute_move_commits(mut_repo, loc)?; + apply_move_commits(mut_repo, commits, options) +} + +fn compute_move_commits( + repo: &MutableRepo, + loc: &MoveCommitsLocation, +) -> BackendResult { let target_commit_ids: IndexSet; let connected_target_commits: Vec; let connected_target_commits_internal_parents: HashMap>; @@ -440,17 +464,17 @@ pub fn move_commits( match &loc.target { MoveCommitsTarget::Commits(commit_ids) => { if commit_ids.is_empty() { - return Ok(MoveCommitsStats::default()); + return Ok(ComputedMoveCommits::empty()); } target_commit_ids = commit_ids.iter().cloned().collect(); connected_target_commits = RevsetExpression::commits(commit_ids.clone()) .connected() - .evaluate(mut_repo) + .evaluate(repo) .map_err(|err| err.into_backend_error())? .iter() - .commits(mut_repo.store()) + .commits(repo.store()) .try_collect() .map_err(|err| err.into_backend_error())?; connected_target_commits_internal_parents = @@ -466,12 +490,12 @@ pub fn move_commits( } MoveCommitsTarget::Roots(root_ids) => { if root_ids.is_empty() { - return Ok(MoveCommitsStats::default()); + return Ok(ComputedMoveCommits::empty()); } target_commit_ids = RevsetExpression::commits(root_ids.clone()) .descendants() - .evaluate(mut_repo) + .evaluate(repo) .map_err(|err| err.into_backend_error())? .iter() .try_collect() @@ -479,7 +503,7 @@ pub fn move_commits( connected_target_commits = target_commit_ids .iter() - .map(|id| mut_repo.store().get_commit(id)) + .map(|id| repo.store().get_commit(id)) .try_collect()?; // We don't have to compute the internal parents for the connected target set, // since the connected target set is the same as the target set. @@ -493,7 +517,7 @@ pub fn move_commits( // ancestors which are not in the target set as parents. let mut target_commits_external_parents: HashMap> = HashMap::new(); for id in target_commit_ids.iter().rev() { - let commit = mut_repo.store().get_commit(id)?; + let commit = repo.store().get_commit(id)?; let mut new_parents = IndexSet::new(); for old_parent in commit.parent_ids() { if let Some(parents) = target_commits_external_parents.get(old_parent) { @@ -534,10 +558,10 @@ pub fn move_commits( &RevsetExpression::commits(target_commit_ids.iter().cloned().collect_vec()) .children(), ) - .evaluate(mut_repo) + .evaluate(repo) .map_err(|err| err.into_backend_error())? .iter() - .commits(mut_repo.store()) + .commits(repo.store()) .try_collect() .map_err(|err| err.into_backend_error())?; @@ -582,14 +606,14 @@ pub fn move_commits( if let Some(children) = target_commit_external_descendants.get(id) { new_children.extend(children.iter().cloned()); } else { - new_children.push(mut_repo.store().get_commit(id)?); + new_children.push(repo.store().get_commit(id)?); } } new_children } else { loc.new_child_ids .iter() - .map(|id| mut_repo.store().get_commit(id)) + .map(|id| repo.store().get_commit(id)) .try_collect()? }; @@ -645,8 +669,8 @@ pub fn move_commits( let mut roots = target_roots.iter().cloned().collect_vec(); roots.extend(new_children.iter().ids().cloned()); - let descendants = mut_repo.find_descendants_for_rebase(roots.clone())?; - let commit_new_parents_map: HashMap<_, _> = descendants + let descendants = repo.find_descendants_for_rebase(roots.clone())?; + let commit_new_parents_map = descendants .iter() .map(|commit| { let commit_id = commit.id(); @@ -678,7 +702,7 @@ pub fn move_commits( connected_target_commits_internal_parents.get(parent_id) { new_parents.extend(parents.iter().cloned()); } else if !new_children.iter().any(|new_child| { - mut_repo.index().is_ancestor(new_child.id(), parent_id) }) { + repo.index().is_ancestor(new_child.id(), parent_id) }) { new_parents.push(parent_id.clone()); } } @@ -708,6 +732,18 @@ pub fn move_commits( }) .collect(); + Ok(ComputedMoveCommits { + target_commit_ids, + descendants, + commit_new_parents_map, + }) +} + +fn apply_move_commits( + mut_repo: &mut MutableRepo, + commits: ComputedMoveCommits, + options: &RebaseOptions, +) -> BackendResult { let mut num_rebased_targets = 0; let mut num_rebased_descendants = 0; let mut num_skipped_rebases = 0; @@ -721,13 +757,13 @@ pub fn move_commits( }; mut_repo.transform_commits( - descendants, - &commit_new_parents_map, + commits.descendants, + &commits.commit_new_parents_map, &options.rewrite_refs, |rewriter| { let old_commit_id = rewriter.old_commit().id(); if rewriter.parents_changed() { - let is_target_commit = target_commit_ids.contains(old_commit_id); + let is_target_commit = commits.target_commit_ids.contains(old_commit_id); let rebased_commit = rebase_commit_with_options( rewriter, if is_target_commit {