cli split: Disable the new bookmark behavior for jj split

The consensus about this change, if one ever existed, seems to have dissolved into two cohorts, one which is happy with the change, and one which thinks we should move both the bookmark and change id to the child commit instead of leaving them on the parent. We may decide to add flags to allow users to choose between the two behaviors, but there are also other concerns such as where @ should go (parent or child). Until we agree on a path forward it seems reasonable to delay the breaking change by disabling it via the config option we added. I don't think it's necessary to fully revert the code and new tests since we aren't announcing the option. #3419
jj-vcs · Feb 28, 2025 · 2af5b60 · 2af5b60
1 parent 8a6bfc9
commit 2af5b60
Show file tree

Hide file tree

Showing 4 changed files with 16 additions and 49 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -33,13 +33,6 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
   of forgetting them, since forgetting a remote bookmark can be unintuitive.
   The old behavior is still available with the new `--include-remotes` flag.
 
-* `jj split` no longer moves bookmarks to the second revision created by the
-  split. Instead, local bookmarks associated with the target revision will move
-  to the first revision created by the split (which inherits the target
-  revision's change id). You can opt out of this change by setting
-  `split.legacy-bookmark-behavior = true`, but this will likely be removed in a
-  future release. [#3419](https://github.com/jj-vcs/jj/issues/3419)
-
 * `jj fix` now always sets the working directory of invoked tools to be the
   workspace root, instead of the working directory of the `jj fix`.
 

diff --git a/cli/src/commands/split.rs b/cli/src/commands/split.rs
@@ -199,7 +199,7 @@ The remainder will be in the second commit.
         commit_builder.write(tx.repo_mut())?
     };
 
-    let legacy_bookmark_behavior = read_legacy_bookmark_behavior_setting(tx.settings(), ui)?;
+    let legacy_bookmark_behavior = tx.settings().get_bool("split.legacy-bookmark-behavior")?;
     if legacy_bookmark_behavior {
         // Mark the commit being split as rewritten to the second commit. This
         // moves any bookmarks pointing to the target commit to the second
@@ -244,28 +244,3 @@ The remainder will be in the second commit.
     tx.finish(ui, format!("split commit {}", target_commit.id().hex()))?;
     Ok(())
 }
-
-/// Reads and returns 'split.legacy-bookmark-behavior' from settings and prints
-/// a warning if it is set to true to alert the user of the deprecation.
-fn read_legacy_bookmark_behavior_setting(
-    settings: &jj_lib::settings::UserSettings,
-    ui: &Ui,
-) -> Result<bool, CommandError> {
-    let use_legacy_behavior = settings.get_bool("split.legacy-bookmark-behavior")?;
-    if use_legacy_behavior {
-        writeln!(
-            ui.warning_default(),
-            "`jj split` will leave bookmarks on the first commit in the next release."
-        )?;
-        writeln!(
-            ui.warning_default(),
-            "Run `jj config set --user split.legacy-bookmark-behavior false` to silence this \
-             message and use the new behavior."
-        )?;
-        writeln!(
-            ui.warning_default(),
-            "See https://github.com/jj-vcs/jj/issues/3419"
-        )?;
-    }
-    Ok(use_legacy_behavior)
-}
diff --git a/cli/src/config/misc.toml b/cli/src/config/misc.toml
@@ -49,5 +49,7 @@ auto-track = "all()"
 auto-update-stale = false
 
 # TODO: https://github.com/jj-vcs/jj/issues/3419 - Remove when fully deprecated.
+# The behavior when this flag is set to false is experimental and may be changed
+# in the future.
 [split]
-legacy-bookmark-behavior = false
+legacy-bookmark-behavior = true
diff --git a/cli/tests/test_split_command.rs b/cli/tests/test_split_command.rs
@@ -881,24 +881,24 @@ fn test_split_with_multiple_workspaces_different_working_copy() {
 
 enum BookmarkBehavior {
     Default,
-    Legacy,
-    Modern,
+    MoveBookmarkToChild,
+    LeaveBookmarkWithTarget,
 }
 
 // TODO: https://github.com/jj-vcs/jj/issues/3419 - Delete params when the config is removed.
 #[test_case(BookmarkBehavior::Default; "default_behavior")]
-#[test_case(BookmarkBehavior::Legacy; "legacy_behavior")]
-#[test_case(BookmarkBehavior::Modern; "modern_behavior")]
+#[test_case(BookmarkBehavior::MoveBookmarkToChild; "move_bookmark_to_child")]
+#[test_case(BookmarkBehavior::LeaveBookmarkWithTarget; "leave_bookmark_with_target")]
 fn test_split_with_bookmarks(bookmark_behavior: BookmarkBehavior) {
     let mut test_env = TestEnvironment::default();
     test_env.run_jj_in(".", ["git", "init", "main"]).success();
     let main_path = test_env.env_root().join("main");
 
     match bookmark_behavior {
-        BookmarkBehavior::Modern => {
+        BookmarkBehavior::LeaveBookmarkWithTarget => {
             test_env.add_config("split.legacy-bookmark-behavior=false");
         }
-        BookmarkBehavior::Legacy => {
+        BookmarkBehavior::MoveBookmarkToChild => {
             test_env.add_config("split.legacy-bookmark-behavior=true");
         }
         BookmarkBehavior::Default => (),
@@ -929,7 +929,7 @@ fn test_split_with_bookmarks(bookmark_behavior: BookmarkBehavior) {
     .unwrap();
     let output = test_env.run_jj_in(&main_path, ["split", "file2"]);
     match bookmark_behavior {
-        BookmarkBehavior::Default | BookmarkBehavior::Modern => {
+        BookmarkBehavior::LeaveBookmarkWithTarget => {
             insta::allow_duplicates! {
             insta::assert_snapshot!(output, @r"
             ------- stderr -------
@@ -949,19 +949,16 @@ fn test_split_with_bookmarks(bookmark_behavior: BookmarkBehavior) {
             ");
             }
         }
-        BookmarkBehavior::Legacy => {
+        BookmarkBehavior::Default | BookmarkBehavior::MoveBookmarkToChild => {
             insta::allow_duplicates! {
-            insta::assert_snapshot!(output, @r"
+            insta::assert_snapshot!(output, @r###"
             ------- stderr -------
-            Warning: `jj split` will leave bookmarks on the first commit in the next release.
-            Warning: Run `jj config set --user split.legacy-bookmark-behavior false` to silence this message and use the new behavior.
-            Warning: See https://github.com/jj-vcs/jj/issues/3419
             First part: qpvuntsm 63d0c5ed first-commit
             Second part: mzvwutvl a9f5665f *le-signet* | second-commit
             Working copy now at: mzvwutvl a9f5665f *le-signet* | second-commit
             Parent commit      : qpvuntsm 63d0c5ed first-commit
             [EOF]
-            ");
+            "###);
             }
             insta::allow_duplicates! {
             insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r"
@@ -985,7 +982,7 @@ fn test_split_with_bookmarks(bookmark_behavior: BookmarkBehavior) {
         .run_jj_in(&main_path, ["split", "file2", "--parallel"])
         .success();
     match bookmark_behavior {
-        BookmarkBehavior::Default | BookmarkBehavior::Modern => {
+        BookmarkBehavior::LeaveBookmarkWithTarget => {
             insta::allow_duplicates! {
             insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r"
             @  vruxwmqvtpmx false second-commit
@@ -996,7 +993,7 @@ fn test_split_with_bookmarks(bookmark_behavior: BookmarkBehavior) {
             ");
             }
         }
-        BookmarkBehavior::Legacy => {
+        BookmarkBehavior::Default | BookmarkBehavior::MoveBookmarkToChild => {
             insta::allow_duplicates! {
             insta::assert_snapshot!(get_log_output(&test_env, &main_path), @r"
             @  vruxwmqvtpmx false *le-signet* second-commit