From 23ad80c65f053ef18a0d0b834b8ee8372a71402e Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Thu, 11 Apr 2024 09:27:17 -0400 Subject: [PATCH] Fix a bug when the target of `jj split` has merge commit children Ilya reported this in https://github.com/martinvonz/jj/issues/3483. The bug was introduced in https://github.com/martinvonz/jj/commit/976320726d99659936eeab0e71b42d816dffbac0. Before this fix, `jj split` dropped any parents what weren't involved in the split when it rebased the children of the commit being split. This meant that any children which were merge commits lost their other parents unintentionally. Fixes #3483 --- CHANGELOG.md | 3 ++ cli/src/commands/split.rs | 63 +++++++++++++++++++++++---------- cli/tests/test_split_command.rs | 47 ++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 18 deletions(-) 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..3eaf34e9ff 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().into_iter() + } else { + vec![c].into_iter() + } + }) + .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 + "###); +}