From 6204ebc042f3ce8ddbeb5dd88527421503a564f8 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Sat, 20 Apr 2024 12:09:27 -0400 Subject: [PATCH 1/2] Add checks for stderr and stdout to `jj split` tests This would have caught a bug in https://github.com/martinvonz/jj/pull/3550 that otherwise might have slipped through. --- cli/tests/test_split_command.rs | 100 +++++++++++++++++++++++++------- 1 file changed, 78 insertions(+), 22 deletions(-) diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index a4b19be6f0..0d961e1df1 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -173,7 +173,14 @@ fn test_split_with_non_empty_description() { .join("\0"), ) .unwrap(); - test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + First part: qpvuntsm 41e04d04 part 1 + Second part: kkmpptxz 093b6c0d part 2 + Working copy now at: kkmpptxz 093b6c0d part 2 + Parent commit : qpvuntsm 41e04d04 part 1 + "###); assert_eq!( std::fs::read_to_string(test_env.env_root().join("editor1")).unwrap(), @@ -224,7 +231,14 @@ fn test_split_with_default_description() { ["dump editor1", "next invocation\n", "dump editor2"].join("\0"), ) .unwrap(); - test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + First part: qpvuntsm 5afe936c TESTED=TODO + Second part: kkmpptxz 0e09a2df test_branch | (no description set) + Working copy now at: kkmpptxz 0e09a2df test_branch | (no description set) + Parent commit : qpvuntsm 5afe936c TESTED=TODO + "###); // Since the commit being split has no description, the user will only be // prompted to add a description to the first commit, which will use the @@ -281,7 +295,17 @@ fn test_split_with_merge_child() { ["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"), ) .unwrap(); - test_env.jj_cmd_ok(&workspace_path, &["split", "-r", "description(a)", "file1"]); + let (stdout, stderr) = + test_env.jj_cmd_ok(&workspace_path, &["split", "-r", "description(a)", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 descendant commits + First part: kkmpptxz e8006b47 Add file1 + Second part: royxmykx 5e1b793d Add file2 + Working copy now at: zsuskuln 0315e471 (empty) 2 + Parent commit : qpvuntsm dc0e5d61 (empty) 1 + Parent commit : royxmykx 5e1b793d Add file2 + "###); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" @ zsuskulnrvyr true 2 ├─╮ @@ -319,7 +343,21 @@ fn test_split_siblings_no_descendants() { ["dump editor1", "next invocation\n", "dump editor2"].join("\0"), ) .unwrap(); - test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + First part: qpvuntsm?? 8d2b7558 TESTED=TODO + Second part: zsuskuln acd41528 (no description set) + Working copy now at: zsuskuln acd41528 test_branch | (no description set) + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + @ zsuskulnrvyr false test_branch + │ ◉ qpvuntsmwlqt false TESTED=TODO + ├─╯ + ◉ zzzzzzzzzzzz true + "###); // Since the commit being split has no description, the user will only be // prompted to add a description to the first commit, which will use the @@ -338,12 +376,6 @@ 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 - ├─╯ - ◉ zzzzzzzzzzzz true - "###); } #[test] @@ -368,6 +400,12 @@ fn test_split_siblings_with_descendants() { // to the split command. test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]); test_env.jj_cmd_ok(&workspace_path, &["prev", "--edit"]); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + ◉ kkmpptxzrspx false Add file4 + ◉ rlvkpnrzqnoo false Add file3 + @ qpvuntsmwlqt false Add file1 & file2 + ◉ zzzzzzzzzzzz true + "###); // Set up the editor and do the split. let edit_script = test_env.set_up_fake_editor(); @@ -383,7 +421,25 @@ fn test_split_siblings_with_descendants() { .join("\0"), ) .unwrap(); - test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]); + let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 2 descendant commits + First part: qpvuntsm 27b151c3 Add file1 + Second part: vruxwmqv c0857cfb Add file2 + Working copy now at: vruxwmqv c0857cfb Add file2 + Parent commit : zzzzzzzz 00000000 (empty) (no description set) + Added 0 files, modified 0 files, removed 1 files + "###); + insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" + ◉ kkmpptxzrspx false Add file4 + ◉ rlvkpnrzqnoo false Add file3 + ├─╮ + │ @ vruxwmqvtpmx false Add file2 + ◉ │ qpvuntsmwlqt false Add file1 + ├─╯ + ◉ zzzzzzzzzzzz true + "###); // The commit we're splitting has a description, so the user will be // prompted to enter a description for each of the sibling commits. @@ -409,16 +465,6 @@ JJ: A file2 JJ: Lines starting with "JJ: " (like this one) will be removed. "# ); - - insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" - ◉ kkmpptxzrspx false Add file4 - ◉ rlvkpnrzqnoo false Add file3 - ├─╮ - │ @ yqosqzytrlsw false Add file2 - ◉ │ qpvuntsmwlqt false Add file1 - ├─╯ - ◉ zzzzzzzzzzzz true - "###); } // This test makes sure that the children of the commit being split retain any @@ -452,10 +498,20 @@ fn test_split_siblings_with_merge_child() { ["write\nAdd file1", "next invocation\n", "write\nAdd file2"].join("\0"), ) .unwrap(); - test_env.jj_cmd_ok( + let (stdout, stderr) = test_env.jj_cmd_ok( &workspace_path, &["split", "-r", "description(a)", "--siblings", "file1"], ); + insta::assert_snapshot!(stdout, @""); + insta::assert_snapshot!(stderr, @r###" + Rebased 1 descendant commits + First part: kkmpptxz e8006b47 Add file1 + Second part: royxmykx 2cc60f3d Add file2 + Working copy now at: zsuskuln 2f04d1d1 (empty) 2 + Parent commit : qpvuntsm dc0e5d61 (empty) 1 + Parent commit : kkmpptxz e8006b47 Add file1 + Parent commit : royxmykx 2cc60f3d Add file2 + "###); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" @ zsuskulnrvyr true 2 ├─┬─╮ From dd643c949cb415529da310970354c6cc6d53b2eb Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Sat, 20 Apr 2024 10:48:52 -0400 Subject: [PATCH 2/2] Rewrite rebasing in `jj split` using `transform_descendants()` This is following on the rewrite for `parallelize`. - https://github.com/martinvonz/jj/pull/3521 Since rebase_descendants from rebase.rs is no longer used outside of that file, it can be made private again. --- cli/src/commands/rebase.rs | 2 +- cli/src/commands/split.rs | 68 +++++++++------------------------ cli/tests/test_split_command.rs | 4 +- 3 files changed, 22 insertions(+), 52 deletions(-) diff --git a/cli/src/commands/rebase.rs b/cli/src/commands/rebase.rs index 8b6ffc68cd..bd50dcd243 100644 --- a/cli/src/commands/rebase.rs +++ b/cli/src/commands/rebase.rs @@ -286,7 +286,7 @@ fn rebase_branch( } /// Rebases `old_commits` onto `new_parents`. -pub fn rebase_descendants( +fn rebase_descendants( tx: &mut WorkspaceCommandTransaction, settings: &UserSettings, new_parents: Vec, diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index dc5608c669..c4298d23a1 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -13,18 +13,13 @@ // limitations under the License. use std::io::Write; -use itertools::Itertools; -use jj_lib::commit::Commit; 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, WorkspaceCommandTransaction}; +use crate::cli_util::{CommandHelper, RevisionArg}; use crate::command_error::CommandError; -use crate::commands::rebase::rebase_descendants; use crate::description_util::{description_template_for_commit, edit_description}; use crate::ui::Ui; @@ -179,16 +174,24 @@ the operation will be aborted. // 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()], - )? - } else { - tx.mut_repo().rebase_descendants(command.settings())? - }; + let mut num_rebased = 0; + tx.mut_repo().transform_descendants( + command.settings(), + vec![commit.id().clone()], + |mut rewriter| { + num_rebased += 1; + if args.siblings { + rewriter.replace_parent( + second_commit.id(), + &[first_commit.id().clone(), second_commit.id().clone()], + ); + } + // We don't need to do anything special for the non-siblings case + // since we already marked the original commit as rewritten. + rewriter.rebase(command.settings())?.write()?; + Ok(()) + }, + )?; if let Some(mut formatter) = ui.status_formatter() { if num_rebased > 0 { @@ -203,36 +206,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() - } 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 0d961e1df1..051fe24f72 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -346,8 +346,8 @@ fn test_split_siblings_no_descendants() { let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--siblings", "file1"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - First part: qpvuntsm?? 8d2b7558 TESTED=TODO - Second part: zsuskuln acd41528 (no description set) + First part: qpvuntsm 8d2b7558 TESTED=TODO + Second part: zsuskuln acd41528 test_branch | (no description set) Working copy now at: zsuskuln acd41528 test_branch | (no description set) Parent commit : zzzzzzzz 00000000 (empty) (no description set) Added 0 files, modified 0 files, removed 1 files