From f74618f41d24f769ce2babc0663e9342faa880c4 Mon Sep 17 00:00:00 2001 From: Benjamin Tan Date: Fri, 31 May 2024 18:56:05 +0800 Subject: [PATCH] new: refactor creation of new commit into common code path --- cli/src/commands/new.rs | 137 +++++++++++++++++----------------- cli/tests/test_new_command.rs | 12 +-- 2 files changed, 76 insertions(+), 73 deletions(-) diff --git a/cli/src/commands/new.rs b/cli/src/commands/new.rs index d6caccba4..d1b3935e8 100644 --- a/cli/src/commands/new.rs +++ b/cli/src/commands/new.rs @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashSet; use std::io::Write; use clap::ArgGroup; use itertools::Itertools; -use jj_lib::commit::{Commit, CommitIteratorExt}; +use jj_lib::backend::CommitId; +use jj_lib::commit::CommitIteratorExt; use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use jj_lib::rewrite::{merge_commit_trees, rebase_commit}; @@ -111,20 +113,20 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", (None, Vec::new()) }; - let mut tx = workspace_command.start_transaction(); - let mut num_rebased; - let new_commit; + let parent_commits; + let parent_commit_ids; + let children_commits; + if args.insert_before { // Instead of having the new commit as a child of the changes given on the // command line, add it between the changes' parents and the changes. // The parents of the new commit will be the parents of the target commits // which are not descendants of other target commits. - tx.base_workspace_helper().check_rewritable(&target_ids)?; - let new_children = RevsetExpression::commits(target_ids.clone()); - let new_parents = new_children.parents(); - if let Some(commit_id) = new_children - .dag_range_to(&new_parents) - .evaluate_programmatic(tx.repo())? + let children_expression = RevsetExpression::commits(target_ids); + let parents_expression = children_expression.parents(); + if let Some(commit_id) = children_expression + .dag_range_to(&parents_expression) + .evaluate_programmatic(workspace_command.repo().as_ref())? .iter() .next() { @@ -134,68 +136,69 @@ Please use `jj new 'all:x|y'` instead of `jj new --allow-large-revsets x y`.", short_commit_hash(&commit_id), ))); } - let new_parents_commits: Vec = new_parents - .evaluate_programmatic(tx.repo())? + // Manually collect the parent commit IDs to preserve the order of parents. + parent_commit_ids = target_commits .iter() - .commits(tx.repo().store()) + .flat_map(|commit| commit.parent_ids()) + .unique() + .cloned() + .collect_vec(); + parent_commits = parent_commit_ids + .iter() + .map(|commit_id| workspace_command.repo().store().get_commit(commit_id)) + .try_collect()?; + children_commits = target_commits; + } else if args.insert_after { + parent_commits = target_commits; + parent_commit_ids = parent_commits.iter().ids().cloned().collect(); + let parents_expression = RevsetExpression::commits(target_ids); + // Each child of the targets will be rebased: its set of parents will be updated + // so that the targets are replaced by the new commit. + // Exclude children that are ancestors of the new commit + let children_expression = parents_expression + .children() + .minus(&parents_expression.ancestors()); + children_commits = children_expression + .evaluate_programmatic(workspace_command.repo().as_ref())? + .iter() + .commits(workspace_command.repo().store()) .try_collect()?; - let merged_tree = merge_commit_trees(tx.repo(), &new_parents_commits)?; - let new_parents_commit_ids = new_parents_commits.iter().ids().cloned().collect(); - new_commit = tx - .mut_repo() - .new_commit(command.settings(), new_parents_commit_ids, merged_tree.id()) - .set_description(join_message_paragraphs(&args.message_paragraphs)) - .write()?; - num_rebased = target_ids.len(); - for child_commit in target_commits { - rebase_commit( - command.settings(), - tx.mut_repo(), - child_commit, - vec![new_commit.id().clone()], - )?; - } } else { - let old_parents = RevsetExpression::commits(target_ids.clone()); - let commits_to_rebase: Vec = if args.insert_after { - // Each child of the targets will be rebased: its set of parents will be updated - // so that the targets are replaced by the new commit. - // Exclude children that are ancestors of the new commit - let to_rebase = old_parents.children().minus(&old_parents.ancestors()); - to_rebase - .evaluate_programmatic(tx.base_repo().as_ref())? - .iter() - .commits(tx.base_repo().store()) - .try_collect()? - } else { - vec![] - }; - tx.base_workspace_helper() - .check_rewritable(commits_to_rebase.iter().ids())?; - let merged_tree = merge_commit_trees(tx.repo(), &target_commits)?; - new_commit = tx - .mut_repo() - .new_commit(command.settings(), target_ids.clone(), merged_tree.id()) - .set_description(join_message_paragraphs(&args.message_paragraphs)) - .write()?; - num_rebased = commits_to_rebase.len(); - for child_commit in commits_to_rebase { - let commit_parents = RevsetExpression::commits(child_commit.parent_ids().to_owned()); - let new_parents = commit_parents.minus(&old_parents); - let mut new_parent_ids = new_parents - .evaluate_programmatic(tx.base_repo().as_ref())? - .iter() - .collect_vec(); - new_parent_ids.push(new_commit.id().clone()); - rebase_commit( - command.settings(), - tx.mut_repo(), - child_commit, - new_parent_ids, - )?; - } + parent_commits = target_commits; + parent_commit_ids = parent_commits.iter().ids().cloned().collect(); + children_commits = vec![]; + }; + workspace_command.check_rewritable(children_commits.iter().ids())?; + + let parent_commit_ids_set: HashSet = parent_commit_ids.iter().cloned().collect(); + + let mut tx = workspace_command.start_transaction(); + let merged_tree = merge_commit_trees(tx.repo(), &parent_commits)?; + let new_commit = tx + .mut_repo() + .new_commit(command.settings(), parent_commit_ids, merged_tree.id()) + .set_description(join_message_paragraphs(&args.message_paragraphs)) + .write()?; + + let mut num_rebased = 0; + for child_commit in children_commits { + let new_parent_ids = child_commit + .parent_ids() + .iter() + .filter(|id| !parent_commit_ids_set.contains(id)) + .cloned() + .chain(std::iter::once(new_commit.id().clone())) + .collect_vec(); + rebase_commit( + command.settings(), + tx.mut_repo(), + child_commit, + new_parent_ids, + )?; + num_rebased += 1; } num_rebased += tx.mut_repo().rebase_descendants(command.settings())?; + if args.no_edit { if let Some(mut formatter) = ui.status_formatter() { write!(formatter, "Created new commit ")?; diff --git a/cli/tests/test_new_command.rs b/cli/tests/test_new_command.rs index 4eace5492..4b1c82f50 100644 --- a/cli/tests/test_new_command.rs +++ b/cli/tests/test_new_command.rs @@ -314,10 +314,10 @@ fn test_new_insert_before() { insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" Rebased 2 descendant commits - Working copy now at: kxryzmor ff6bbbc7 (empty) G - Parent commit : znkkpsqq 41a89ffc E | (empty) E - Parent commit : vruxwmqv c9257eff D | (empty) D + Working copy now at: kxryzmor 19e53931 (empty) G Parent commit : kkmpptxz 6041917c B | (empty) B + Parent commit : vruxwmqv c9257eff D | (empty) D + Parent commit : znkkpsqq 41a89ffc E | (empty) E "###); insta::assert_snapshot!(get_short_log_output(&test_env, &repo_path), @r###" ◉ F @@ -325,11 +325,11 @@ fn test_new_insert_before() { ├─╯ @ G ├─┬─╮ - │ │ ◉ B - │ │ ◉ A + │ │ ◉ E │ ◉ │ D │ ├─╯ - ◉ │ E + ◉ │ B + ◉ │ A ├─╯ ◉ root "###);