From 9ac64584ef538884241ae0cb7b2b92be6d877472 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Fri, 29 Mar 2024 21:31:10 +0800 Subject: [PATCH] rebase: extract out some functions from `rebase_revision` --- cli/src/commands/rebase.rs | 172 +++++++++++++++++++++++-------------- 1 file changed, 106 insertions(+), 66 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index cdf13fbc2eb..1a66aa9bde0 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -19,7 +19,7 @@ use std::sync::Arc; use clap::ArgGroup; use indexmap::IndexSet; use itertools::Itertools; -use jj_lib::backend::CommitId; +use jj_lib::backend::{BackendError, CommitId}; use jj_lib::commit::Commit; use jj_lib::object_id::ObjectId; use jj_lib::repo::{ReadonlyRepo, Repo}; @@ -30,7 +30,7 @@ use tracing::instrument; use crate::cli_util::{ self, resolve_multiple_nonempty_revsets_default_single, short_commit_hash, CommandHelper, - RevisionArg, WorkspaceCommandHelper, + RevisionArg, WorkspaceCommandHelper, WorkspaceCommandTransaction, }; use crate::command_error::{user_error, CommandError}; use crate::ui::Ui; @@ -346,64 +346,9 @@ fn rebase_revision( ))); } - let children_expression = RevsetExpression::commit(old_commit.id().clone()).children(); - let child_commits: Vec<_> = children_expression - .evaluate_programmatic(workspace_command.repo().as_ref()) - .unwrap() - .iter() - .commits(workspace_command.repo().store()) - .try_collect()?; - // Currently, immutable commits are defined so that a child of a rewriteable - // commit is always rewriteable. - debug_assert!(workspace_command.check_rewritable(&child_commits).is_ok()); - - // First, rebase the children of `old_commit`. let mut tx = workspace_command.start_transaction(); - let mut rebased_commit_ids = HashMap::new(); - for child_commit in &child_commits { - let new_child_parent_ids: Vec = child_commit - .parents() - .iter() - .flat_map(|c| { - if c == &old_commit { - old_commit - .parents() - .iter() - .map(|c| c.id().clone()) - .collect() - } else { - [c.id().clone()].to_vec() - } - }) - .collect(); - - // Some of the new parents may be ancestors of others as in - // `test_rebase_single_revision`. - let new_child_parents_expression = RevsetExpression::commits(new_child_parent_ids.clone()) - .minus( - &RevsetExpression::commits(new_child_parent_ids.clone()) - .parents() - .ancestors(), - ); - let new_child_parents: Vec = new_child_parents_expression - .evaluate_programmatic(tx.base_repo().as_ref()) - .unwrap() - .iter() - .commits(tx.base_repo().store()) - .try_collect()?; - - rebased_commit_ids.insert( - child_commit.id().clone(), - rebase_commit(settings, tx.mut_repo(), child_commit, &new_child_parents)? - .id() - .clone(), - ); - } - // Now, rebase the descendants of the children. - // TODO(ilyagr): Consider making it possible for these descendants to become - // emptied, like --skip_empty. This would require writing careful tests. - rebased_commit_ids.extend(tx.mut_repo().rebase_descendants_return_map(settings)?); - let num_rebased_descendants = rebased_commit_ids.len(); + let (rebased_commit_ids, num_rebased_descendants) = + extract_commit(&mut tx, settings, &old_commit)?; // We now update `new_parents` to account for the rebase of all of // `old_commit`'s descendants. Even if some of the original `new_parents` @@ -432,13 +377,7 @@ fn rebase_revision( // `old_commit` onto `Q`, new_parents would only account for one of these. let new_parents: Vec<_> = new_parents .iter() - .map(|new_parent| { - rebased_commit_ids - .get(new_parent.id()) - .map_or(Ok(new_parent.clone()), |rebased_new_parent_id| { - tx.repo().store().get_commit(rebased_new_parent_id) - }) - }) + .map(|new_parent| get_possibly_rewritten_commit(&mut tx, &rebased_commit_ids, new_parent)) .try_collect()?; // Finally, it's safe to rebase `old_commit`. We can skip rebasing if it is @@ -477,6 +416,107 @@ fn rebase_revision( } } +/// Extracts `commit` from the graph by rebasing its children on its parents. +/// This assumes that it can be rewritten. +fn extract_commit( + tx: &mut WorkspaceCommandTransaction, + settings: &UserSettings, + commit: &Commit, +) -> Result<(HashMap, usize), CommandError> { + let children_expression = RevsetExpression::commit(commit.id().clone()).children(); + let child_commits: Vec<_> = children_expression + .evaluate_programmatic(tx.base_repo().as_ref()) + .unwrap() + .iter() + .commits(tx.base_repo().store()) + .try_collect()?; + // Currently, immutable commits are defined so that a child of a rewriteable + // commit is always rewriteable. + debug_assert!(tx + .base_workspace_helper() + .check_rewritable(&child_commits) + .is_ok()); + + let commits_to_rebase: Vec<_> = child_commits + .into_iter() + .map(|child_commit| { + let new_child_parent_ids: Vec = child_commit + .parents() + .iter() + .flat_map(|c| { + if c == commit { + commit.parents().iter().map(|c| c.id().clone()).collect() + } else { + [c.id().clone()].to_vec() + } + }) + .collect(); + + // Some of the new parents may be ancestors of others as in + // `test_rebase_single_revision`. + let new_child_parents_expression = + RevsetExpression::commits(new_child_parent_ids.clone()).minus( + &RevsetExpression::commits(new_child_parent_ids.clone()) + .parents() + .ancestors(), + ); + let new_child_parents: Vec = new_child_parents_expression + .evaluate_programmatic(tx.base_repo().as_ref()) + .unwrap() + .iter() + .commits(tx.base_repo().store()) + .try_collect()?; + + Ok::<_, BackendError>((child_commit, new_child_parents)) + }) + .try_collect()?; + + rebase_onto_new_parents(tx, settings, &commits_to_rebase) +} + +/// Rebases each commit in `commits_to_rebase` onto its new parents, and calls +/// `rebase_descendants` after. +/// This assumes that each commit can be rewritten. +fn rebase_onto_new_parents( + tx: &mut WorkspaceCommandTransaction, + settings: &UserSettings, + commits_to_rebase: &[(Commit, Vec)], +) -> Result<(HashMap, usize), CommandError> { + // First, rebase the children of `commit`. + let mut rebased_commit_ids = HashMap::new(); + for (commit, new_parents) in commits_to_rebase { + rebased_commit_ids.insert( + commit.id().clone(), + rebase_commit(settings, tx.mut_repo(), commit, &new_parents)? + .id() + .clone(), + ); + } + // Now, rebase the descendants of `commits_to_rebase`. + // TODO(ilyagr): Consider making it possible for these descendants to become + // emptied, like --skip_empty. This would require writing careful tests. + rebased_commit_ids.extend(tx.mut_repo().rebase_descendants_return_map(settings)?); + let num_rebased_descendants = rebased_commit_ids.len(); + + // Return the map of commit IDs that were rewritten, in case a commit needs to + // be reparented onto a rewritten commit. + Ok((rebased_commit_ids, num_rebased_descendants)) +} + +/// Returns either the `commit` if unchanged, or the rewritten commit +/// corresponding to `commit`. +fn get_possibly_rewritten_commit( + tx: &mut WorkspaceCommandTransaction, + rewritten_commit_ids: &HashMap, + commit: &Commit, +) -> Result { + rewritten_commit_ids + .get(commit.id()) + .map_or(Ok(commit.clone()), |new_commit_id| { + tx.repo().store().get_commit(new_commit_id) + }) +} + fn check_rebase_destinations( repo: &Arc, new_parents: &[Commit],