diff --git a/CHANGELOG.md b/CHANGELOG.md index a703a6dfe2..5dd93ce4ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * Revsets now support `\`-escapes in string literal. +* Fixed a bug with `jj split` introduced in 0.16.0 that caused it to incorrectly + rebase the children of the revision being split if they had other parents + (i.e. if the child was a merge). ## [0.16.0] - 2024-04-03 diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 0cec68fc2d..af83bfda07 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -19,9 +19,10 @@ use jj_lib::object_id::ObjectId; use jj_lib::repo::Repo; use jj_lib::revset::{RevsetExpression, RevsetIteratorExt}; use jj_lib::rewrite::merge_commit_trees; +use jj_lib::settings::UserSettings; use tracing::instrument; -use crate::cli_util::{CommandHelper, RevisionArg}; +use crate::cli_util::{CommandHelper, RevisionArg, WorkspaceCommandTransaction}; use crate::command_error::CommandError; use crate::commands::rebase::rebase_descendants; use crate::description_util::{description_template_for_commit, edit_description}; @@ -176,25 +177,17 @@ the operation will be aborted. // commit. tx.mut_repo() .set_rewritten_commit(commit.id().clone(), second_commit.id().clone()); - // Rebase descendants of the commit being split. - let new_parents = if args.siblings { - vec![first_commit.clone(), second_commit.clone()] + let num_rebased = if args.siblings { + rebase_children_for_siblings_split( + &mut tx, + command.settings(), + &commit, + vec![first_commit.clone(), second_commit.clone()], + )? } else { - vec![second_commit.clone()] + tx.mut_repo().rebase_descendants(command.settings())? }; - let children: Vec = RevsetExpression::commit(commit.id().clone()) - .children() - .evaluate_programmatic(tx.base_repo().as_ref())? - .iter() - .commits(tx.base_repo().store()) - .try_collect()?; - let num_rebased = rebase_descendants( - &mut tx, - command.settings(), - &new_parents, - &children, - Default::default(), - )?; + if let Some(mut formatter) = ui.status_formatter() { if num_rebased > 0 { writeln!(formatter, "Rebased {num_rebased} descendant commits")?; @@ -208,3 +201,37 @@ the operation will be aborted. tx.finish(ui, format!("split commit {}", commit.id().hex()))?; Ok(()) } + +// Rebases the children of `original_commit` by replacing `original_commit` with +// `new_siblings`. Any parents other than `original_commit` will remain after +// the rebase. +fn rebase_children_for_siblings_split( + tx: &mut WorkspaceCommandTransaction, + settings: &UserSettings, + original_commit: &Commit, + new_siblings: Vec, +) -> Result { + let children: Vec = RevsetExpression::commit(original_commit.id().clone()) + .children() + .evaluate_programmatic(tx.base_repo().as_ref())? + .iter() + .commits(tx.base_repo().store()) + .try_collect()?; + let mut num_rebased = 0; + for child in children { + let new_parents = child + .parents() + .into_iter() + .flat_map(|c| { + if c.id() == original_commit.id() { + new_siblings.clone() + } else { + vec![c] + } + }) + .collect_vec(); + num_rebased += + rebase_descendants(tx, settings, &new_parents, &[child], Default::default())?; + } + Ok(num_rebased) +} diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index 35be5f7276..81bc878140 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -377,3 +377,50 @@ JJ: Lines starting with "JJ: " (like this one) will be removed. ◉ zzzzzzzzzzzz true "###); } + +// This test makes sure that the children of the commit being split retain any +// other parents which weren't involved in the split. +#[test] +fn test_split_siblings_with_merge_child() { + let mut test_env = TestEnvironment::default(); + test_env.jj_cmd_ok(test_env.env_root(), &["init", "repo", "--git"]); + let workspace_path = test_env.env_root().join("repo"); + test_env.jj_cmd_ok(&workspace_path, &["describe", "-m=1"]); + test_env.jj_cmd_ok(&workspace_path, &["new", "root()", "-m=a"]); + std::fs::write(workspace_path.join("file1"), "foo\n").unwrap(); + std::fs::write(workspace_path.join("file2"), "bar\n").unwrap(); + test_env.jj_cmd_ok( + &workspace_path, + &["new", "description(1)", "description(a)", "-m=2"], + ); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ zsuskulnrvyr true 2 + ├─╮ + │ ◉ kkmpptxzrspx false a + ◉ │ qpvuntsmwlqt true 1 + ├─╯ + ◉ zzzzzzzzzzzz true + "###); + + // Set up the editor and do the split. + let edit_script = test_env.set_up_fake_editor(); + std::fs::write( + edit_script, + ["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"), + ) + .unwrap(); + test_env.jj_cmd_ok( + &workspace_path, + &["split", "-r", "description(a)", "--siblings", "file1"], + ); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ zsuskulnrvyr true 2 + ├─┬─╮ + │ │ ◉ royxmykxtrkr false Add file2 + │ ◉ │ kkmpptxzrspx false Add file1 + │ ├─╯ + ◉ │ qpvuntsmwlqt true 1 + ├─╯ + ◉ zzzzzzzzzzzz true + "###); +}