cli split: Add a config option controlling how bookmarks move during splits #5618

emesterhazy · 2025-02-07T19:55:30Z

Currently, jj split moves bookmarks from the target revision to the second
revision created by the split. Since the first revision inherits the change id
of the target revision, moving the bookmarks to the first revision is less
surprising (i.e. the bookmarks stay with the change id). This no-implicit-move
behavior also aligns with how jj abandon drops bookmarks instead of moving
them to the parent revision.

Two releases from now, jj split will no longer move 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. You can also try the new behavior now by setting
split.legacy-bookmark-behavior = false.

Users who have not set the new config option will see a warning when they run
jj split informing them about the change.

The jj split tests for bookmarks are updated to run in all three configurations:

Config setting enabled
Config setting disabled
Config setting unset

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
- N/A: I didn't see any docs describing the bookmark semantics.
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

cli/src/commands/split.rs

PhilipMetzger

Thanks for doing this, lgtm. I will leave the last call to Martin.

cli/src/commands/split.rs

emesterhazy · 2025-02-07T21:45:50Z

it would also be nice if you spelled out "the working copy commit (@)" in the comment

Makes sense; done. Thanks for taking a look :)

CHANGELOG.md

cli/src/commands/split.rs

cli/tests/test_split_command.rs

martinvonz

LGTM, but I'll leave it for a non-googler to approve (EDIT: ah, you already have approval)

cli/tests/test_split_command.rs

We are planning to add a config option that controls how bookmarks and change ids move based on feedback in #5618. I think the tests will be more readable after the config option is added if we move the bookmark testing to its own test.

We are planning to add a config option that controls how bookmarks and change ids move during `jj split` based on feedback in #5618. I think the tests will be more readable after the config option is added if we move the bookmark testing to its own test. #3419

cli/src/commands/split.rs

cli/tests/test_split_command.rs

PhilipMetzger

still looks great.

cli/src/commands/split.rs

martinvonz

I had these two draft comments that I meant to send. (But don't let that change your decision about the warning.)

cli/src/commands/split.rs

…splits Currently, `jj split` moves bookmarks from the target revision to the second revision created by the split. Since the first revision inherits the change id of the target revision, moving the bookmarks to the first revision is less surprising (i.e. the bookmarks stay with the change id). This no-implicit-move behavior also aligns with how `jj abandon` drops bookmarks instead of moving them to the parent revision. Two releases from now, `jj split` will no longer move 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. You can also try the new behavior now by setting `split.legacy-bookmark-behavior = false`. Users who have not opted into the new behavior via the config setting will see a warning when they run `jj split` informing them about the change. The default behavior be changed in the future. The `jj split` tests for bookmarks are updated to run in all three configurations: - Config setting enabled - Config setting disabled - Config setting unset #3419

emesterhazy · 2025-02-13T21:45:06Z

I am going to submit this since at this point we are just going back and forth on how and when the warning should be printed, and there's still time to adjust that.

For what it's worth, I agree with @emilazy that it's not great to force users that want the new behavior to adjust their config to silence the warning. However, I thought the reason we added the warning in the first place was to avoid confusing users by changing the behavior silently (not everyone reads the patch notes).

I think the current plan is:

In the next release n, users will get a warning every time they jj split unless they opt into the new behavior.
In release n+1 the behavior will be enabled by default, and users will only get a warning if they opt out of the new behavior.
In some future release, the opt-out will be removed and users will get an error during jj split, or it will just silently do nothing.

Please open a new PR if anyone thinks we should change the plan.

`jj split --parallel` now sets the working copy revision (`@`) to the first revision created by the split instead of the second. As a result, `@` remains on the same change id before and after the split. There is no good reason to choose the second commit as the working copy commit after a --parallel split. The only reason it works that way today is because doing that didn't require any code changes compared to the non parallel split. Martin proposed this change in the review for #5618.

emesterhazy requested review from martinvonz and PhilipMetzger February 7, 2025 19:56

martinvonz reviewed Feb 7, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

PhilipMetzger approved these changes Feb 7, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

emesterhazy force-pushed the push-sksyopkrnqrs branch from 57aed5a to c92fa61 Compare February 7, 2025 21:45

martinvonz changed the title ~~cli split: Leave bookarks associated with the original change id~~ cli split: Leave bookmarks associated with the original change id Feb 8, 2025

emesterhazy force-pushed the push-sksyopkrnqrs branch 2 times, most recently from b4ee730 to 603208b Compare February 9, 2025 02:35

ilyagr reviewed Feb 9, 2025

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

yuja reviewed Feb 9, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

cli/src/commands/split.rs Outdated Show resolved Hide resolved

emesterhazy force-pushed the push-sksyopkrnqrs branch 2 times, most recently from dce749f to 7936539 Compare February 9, 2025 15:00

martinvonz reviewed Feb 9, 2025

View reviewed changes

emesterhazy force-pushed the push-sksyopkrnqrs branch 2 times, most recently from 68280b0 to a32eeba Compare February 9, 2025 18:38

martinvonz reviewed Feb 9, 2025

View reviewed changes

cli/tests/test_split_command.rs Outdated Show resolved Hide resolved

cli/tests/test_split_command.rs Outdated Show resolved Hide resolved

emesterhazy force-pushed the push-sksyopkrnqrs branch from a32eeba to 212dde7 Compare February 9, 2025 20:17

This was referenced Feb 9, 2025

What should jj split do with bookmarks? #3419

Open

cli split: Test how the working copy commit changes across multiple workspaces #5638

Merged

emesterhazy force-pushed the push-sksyopkrnqrs branch from 212dde7 to d242f08 Compare February 10, 2025 15:38

emesterhazy mentioned this pull request Feb 12, 2025

cli split: Move bookmark testing to a separate test #5665

Merged

emesterhazy force-pushed the push-sksyopkrnqrs branch 2 times, most recently from 648b696 to 62f2caf Compare February 13, 2025 01:29

emesterhazy commented Feb 13, 2025

View reviewed changes

cli/src/commands/split.rs Show resolved Hide resolved

emesterhazy requested a review from martinvonz February 13, 2025 03:03

emesterhazy requested a review from PhilipMetzger February 13, 2025 03:04

emesterhazy force-pushed the push-sksyopkrnqrs branch from 62f2caf to 183a478 Compare February 13, 2025 03:04

emesterhazy changed the title ~~cli split: Leave bookmarks associated with the original change id~~ cli split: Add a config option controlling how bookmarks move during splits Feb 13, 2025

emesterhazy force-pushed the push-sksyopkrnqrs branch from 183a478 to 6f4f1ec Compare February 13, 2025 03:09

yuja reviewed Feb 13, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

cli/tests/test_split_command.rs Outdated Show resolved Hide resolved

emesterhazy force-pushed the push-sksyopkrnqrs branch from 6f4f1ec to bc25d6d Compare February 13, 2025 07:35

PhilipMetzger approved these changes Feb 13, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

martinvonz reviewed Feb 13, 2025

View reviewed changes

cli/src/commands/split.rs Outdated Show resolved Hide resolved

cli/src/commands/split.rs Outdated Show resolved Hide resolved

emesterhazy force-pushed the push-sksyopkrnqrs branch from bc25d6d to e4487d8 Compare February 13, 2025 19:35

emesterhazy force-pushed the push-sksyopkrnqrs branch from e4487d8 to cadaaeb Compare February 13, 2025 19:42

emesterhazy added this pull request to the merge queue Feb 13, 2025

Merged via the queue into main with commit d5d2164 Feb 13, 2025
42 checks passed

emesterhazy deleted the push-sksyopkrnqrs branch February 13, 2025 22:02

emesterhazy mentioned this pull request Feb 13, 2025

cli split: Make jj split --parallel set @ to the first revision #5690

Closed

4 tasks

glehmann mentioned this pull request May 7, 2025

split: the second commit keeps the change id #6466

Open

4 tasks

cli split: Add a config option controlling how bookmarks move during splits #5618

cli split: Add a config option controlling how bookmarks move during splits #5618

Uh oh!

Conversation

emesterhazy commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

PhilipMetzger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

emesterhazy commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

martinvonz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PhilipMetzger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

martinvonz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emesterhazy commented Feb 13, 2025

Uh oh!

Uh oh!

Uh oh!

emesterhazy commented Feb 7, 2025 •

edited

Loading

emesterhazy commented Feb 7, 2025 •

edited

Loading

martinvonz left a comment •

edited

Loading