From 57aed5a43946dd3a8f5ccec47ea6e0fccf9f75b9 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Fri, 7 Feb 2025 13:50:08 -0500 Subject: [PATCH] cli split: Leave bookarks associated with the original change id `jj split` no longer moves bookmarks to the second revision created by the split. Local bookmarks associated with the target revision will point to the first revision (which inherits the target revision's change id) created by the split afterward. Since the first revision inherits the change id of the target revision, this behavior is less surprising than moving the bookmark to the second revision. This no-implicit-move behavior also aligns with how `jj abandon` drops bookmarks instead of moving them to the parent revision. fixes #3419 --- CHANGELOG.md | 5 +++++ cli/src/commands/split.rs | 26 ++++++++++++++++---------- cli/tests/cli-reference@.md.snap | 2 ++ cli/tests/test_split_command.rs | 27 +++++++++++++-------------- 4 files changed, 36 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fc45958b25..601666d146 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,11 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ### Breaking changes +* `jj split` no longer moves bookmarks to the second revision created by the + split. Local bookmarks associated with the target revision will point to the + first revision (which inherits the target revision's change id) created by the + split. [#3419](https://github.com/jj-vcs/jj/issues/3419) + ### Deprecations ### New features diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs index 06e8e57c94..e556a41979 100644 --- a/cli/src/commands/split.rs +++ b/cli/src/commands/split.rs @@ -43,6 +43,9 @@ use crate::ui::Ui; /// description, the second part will not get a description, and you will be /// asked for a description only for the first part. /// +/// The first revision created by the split inherits the change id of the target +/// revision and any local bookmarks associated with it. +/// /// Splitting an empty commit is not supported because the same effect can be /// achieved with `jj new`. #[derive(clap::Args, Clone, Debug)] @@ -83,6 +86,7 @@ pub(crate) fn cmd_split( args: &SplitArgs, ) -> Result<(), CommandError> { let mut workspace_command = command.workspace_helper(ui)?; + let at = workspace_command.resolve_single_rev(ui, &RevisionArg::AT)?; let commit = workspace_command.resolve_single_rev(ui, &args.revision)?; if commit.is_empty(workspace_command.repo().as_ref())? { return Err(user_error_with_hint( @@ -196,23 +200,21 @@ The remainder will be in the second commit. commit_builder.write(tx.repo_mut())? }; - // Mark the commit being split as rewritten to the second commit. As a - // result, if @ points to the commit being split, it will point to the - // second commit after the command finishes. This also means that any - // bookmarks pointing to the commit being split are moved to the second - // commit. + // Mark the first commit as the rewrite of the target commit. Since the + // first commit inherits the change id of the target commit, this ensures + // bookmarks continue pointing to the same change id before and after the + // split. tx.repo_mut() - .set_rewritten_commit(commit.id().clone(), second_commit.id().clone()); + .set_rewritten_commit(commit.id().clone(), first_commit.id().clone()); let mut num_rebased = 0; tx.repo_mut() .transform_descendants(vec![commit.id().clone()], |mut rewriter| { num_rebased += 1; if args.parallel { - rewriter - .replace_parent(second_commit.id(), [first_commit.id(), second_commit.id()]); + rewriter.replace_parent(first_commit.id(), [first_commit.id(), second_commit.id()]); + } else { + rewriter.replace_parent(first_commit.id(), [second_commit.id()]); } - // We don't need to do anything special for the non-parallel case - // since we already marked the original commit as rewritten. rewriter.rebase()?.write()?; Ok(()) })?; @@ -227,6 +229,10 @@ The remainder will be in the second commit. tx.write_commit_summary(formatter.as_mut(), &second_commit)?; writeln!(formatter)?; } + // If @ refers to the target commit, move it to the second commit. + if at.id() == commit.id() { + tx.edit(&second_commit)?; + } tx.finish(ui, format!("split commit {}", commit.id().hex()))?; Ok(()) } diff --git a/cli/tests/cli-reference@.md.snap b/cli/tests/cli-reference@.md.snap index 5791a09ec5..b31c84def8 100644 --- a/cli/tests/cli-reference@.md.snap +++ b/cli/tests/cli-reference@.md.snap @@ -2278,6 +2278,8 @@ Starts a [diff editor] on the changes in the revision. Edit the right side of th If the change you split had a description, you will be asked to enter a change description for each commit. If the change did not have a description, the second part will not get a description, and you will be asked for a description only for the first part. +The first revision created by the split inherits the change id of the target revision and any local bookmarks associated with it. + Splitting an empty commit is not supported because the same effect can be achieved with `jj new`. **Usage:** `jj split [OPTIONS] [FILESETS]...` diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs index dfede90b10..4fb6da8a65 100644 --- a/cli/tests/test_split_command.rs +++ b/cli/tests/test_split_command.rs @@ -221,8 +221,7 @@ fn test_split_with_default_description() { std::fs::write(workspace_path.join("file1"), "foo\n").unwrap(); std::fs::write(workspace_path.join("file2"), "bar\n").unwrap(); - // Create a bookmark pointing to the commit. It will be moved to the second - // commit after the split. + // Create a bookmark pointing to the commit. It will not move during the split. test_env.jj_cmd_ok(&workspace_path, &["bookmark", "create", "test_bookmark"]); let edit_script = test_env.set_up_fake_editor(); @@ -234,10 +233,10 @@ fn test_split_with_default_description() { let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "file1"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - First part: qpvuntsm 48018df6 TESTED=TODO - Second part: kkmpptxz 350b4c13 test_bookmark | (no description set) - Working copy now at: kkmpptxz 350b4c13 test_bookmark | (no description set) - Parent commit : qpvuntsm 48018df6 TESTED=TODO + First part: qpvuntsm 48018df6 test_bookmark | TESTED=TODO + Second part: kkmpptxz 350b4c13 (no description set) + Working copy now at: kkmpptxz 350b4c13 (no description set) + Parent commit : qpvuntsm 48018df6 test_bookmark | TESTED=TODO "###); // Since the commit being split has no description, the user will only be @@ -258,8 +257,9 @@ fn test_split_with_default_description() { "###); assert!(!test_env.env_root().join("editor2").exists()); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" - @ kkmpptxzrspx false test_bookmark + @ kkmpptxzrspx false ○ qpvuntsmwlqt false TESTED=TODO + │ test_bookmark ◆ zzzzzzzzzzzz true "###); } @@ -329,8 +329,7 @@ fn test_split_siblings_no_descendants() { std::fs::write(workspace_path.join("file1"), "foo\n").unwrap(); std::fs::write(workspace_path.join("file2"), "bar\n").unwrap(); - // Create a bookmark pointing to the commit. It will be moved to the second - // commit after the split. + // Create a bookmark pointing to the commit. It will not move during the split. test_env.jj_cmd_ok(&workspace_path, &["bookmark", "create", "test_bookmark"]); insta::assert_snapshot!(get_log_output(&test_env, &workspace_path), @r###" @ qpvuntsmwlqt false test_bookmark @@ -346,16 +345,16 @@ fn test_split_siblings_no_descendants() { let (stdout, stderr) = test_env.jj_cmd_ok(&workspace_path, &["split", "--parallel", "file1"]); insta::assert_snapshot!(stdout, @""); insta::assert_snapshot!(stderr, @r###" - First part: qpvuntsm 0dced07a TESTED=TODO - Second part: zsuskuln 0473f014 test_bookmark | (no description set) - Working copy now at: zsuskuln 0473f014 test_bookmark | (no description set) + First part: qpvuntsm 0dced07a test_bookmark | TESTED=TODO + Second part: zsuskuln 0473f014 (no description set) + Working copy now at: zsuskuln 0473f014 (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_bookmark + @ zsuskulnrvyr false │ ○ qpvuntsmwlqt false TESTED=TODO - ├─╯ + ├─╯ test_bookmark ◆ zzzzzzzzzzzz true "###);