From 23ad80c65f053ef18a0d0b834b8ee8372a71402e Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Thu, 11 Apr 2024 09:27:17 -0400 Subject: [PATCH 1/2] 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 + "###); +} From 0e41da5a13e96521ca0625ea73c50b2b901f9758 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Thu, 11 Apr 2024 11:47:47 -0400 Subject: [PATCH 2/2] EXAMPLE: Rewrite commit to multiple commits --- cli/src/commands/split.rs | 52 +++++---------------------------- cli/tests/test_split_command.rs | 10 +++---- lib/src/repo.rs | 6 ++++ 3 files changed, 19 insertions(+), 49 deletions(-) diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 3eaf34e9ff..8c6fab6207 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -175,18 +175,16 @@ the operation will be aborted. // second commit after the command finishes. This also means that any // branches pointing to the commit being split are moved to the second // commit. - tx.mut_repo() - .set_rewritten_commit(commit.id().clone(), second_commit.id().clone()); - let num_rebased = if args.siblings { - rebase_children_for_siblings_split( - &mut tx, - command.settings(), - &commit, - vec![first_commit.clone(), second_commit.clone()], - )? + if args.siblings { + tx.mut_repo().set_rewritten_commit_multiple( + commit.id().clone(), + vec![first_commit.id().clone(), second_commit.id().clone()], + ); } else { - tx.mut_repo().rebase_descendants(command.settings())? + tx.mut_repo() + .set_rewritten_commit(commit.id().clone(), second_commit.id().clone()); }; + let num_rebased = tx.mut_repo().rebase_descendants(command.settings())?; if let Some(mut formatter) = ui.status_formatter() { if num_rebased > 0 { @@ -201,37 +199,3 @@ 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 81bc878140..ee1f673b31 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -296,9 +296,9 @@ JJ: Lines starting with "JJ: " (like this one) will be removed. ); assert!(!test_env.env_root().join("editor2").exists()); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" - @ zsuskulnrvyr false test_branch - │ ◉ qpvuntsmwlqt false TESTED=TODO - ├─╯ + ◉ zsuskulnrvyr false test_branch?? + │ @ qpvuntsmwlqt false TESTED=TODO + ├─╯ test_branch?? ◉ zzzzzzzzzzzz true "###); } @@ -371,8 +371,8 @@ JJ: Lines starting with "JJ: " (like this one) will be removed. ◉ kkmpptxzrspx false Add file4 ◉ rlvkpnrzqnoo false Add file3 ├─╮ - │ @ yqosqzytrlsw false Add file2 - ◉ │ qpvuntsmwlqt false Add file1 + │ ◉ yqosqzytrlsw false Add file2 + @ │ qpvuntsmwlqt false Add file1 ├─╯ ◉ zzzzzzzzzzzz true "###); diff --git a/lib/src/repo.rs b/lib/src/repo.rs index 422c3f42a5..d3eb92108f 100644 --- a/lib/src/repo.rs +++ b/lib/src/repo.rs @@ -831,6 +831,12 @@ impl MutableRepo { .insert(old_id, (RewriteType::Rewritten, vec![new_id])); } + pub fn set_rewritten_commit_multiple(&mut self, old_id: CommitId, new_ids: Vec) { + assert_ne!(old_id, *self.store().root_commit_id()); + self.parent_mapping + .insert(old_id, (RewriteType::Rewritten, new_ids)); + } + /// Record a commit as being rewritten into multiple other commits in this /// transaction. ///