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 976320726d.

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

@@ -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
 

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 num_rebased = if args.siblings {
-    let new_parents = if args.siblings {
+        rebase_children_for_siblings_split(
-        vec![first_commit.clone(), second_commit.clone()]
+            &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<Commit> = 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<Commit>,
+) -> Result<usize, CommandError> {
+    let children: Vec<Commit> = 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)
+}

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
+    "###);
+}