From bc88b3cc67c4eac8c83035df42b5663a521a00b7 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 | 60 +++++++++++++++++++++++---------- cli/tests/test_split_command.rs | 54 +++++++++++++++++++++++++++++ 3 files changed, 99 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..aa8e00924c 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,34 @@ 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 mut new_parents = new_siblings.clone(); + new_parents.extend( + child + .parents() + .into_iter() + // Filter out the commit being split. + .filter(|c| c.id() != original_commit.id()), + ); + 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..b219da4ca9 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -377,3 +377,57 @@ 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, + [ + "dump editor1", + "write\nAdd file1", + "next invocation\n", + "dump editor2", + "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 + ├─┬─╮ + │ │ ◉ qpvuntsmwlqt true 1 + │ ◉ │ royxmykxtrkr false Add file2 + │ ├─╯ + ◉ │ kkmpptxzrspx false Add file1 + ├─╯ + ◉ zzzzzzzzzzzz true + "###); +}