split: the second commit keeps the change id #6466

glehmann · 2025-05-06T11:26:15Z

based on: #6458

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
I have updated the config schema (cli/src/config-schema.json)
I have added tests to cover my changes

cli/src/commands/split.rs

cli/tests/test_split_command.rs

yuja · 2025-05-07T05:46:07Z

cli/src/commands/split.rs

@@ -300,6 +309,9 @@ pub(crate) fn cmd_split(
    if legacy_bookmark_behavior {
        tx.repo_mut()
            .set_rewritten_commit(target.commit.id().clone(), second_commit.id().clone());
+    } else {
+        tx.repo_mut()
+            .set_rewritten_commit(target.commit.id().clone(), first_commit.id().clone());


I don't closely follow the -A/-B/-d discussion, but it seems odd that we need to add this workaround to non-"legacy" code path. Suppose our goal is to remove special case like this, I think we should either make the first commit inherit the original change id (so descendants will be rebased onto it), or make the second commit inherit both change id and descendants.

I think it's more the other way around: we don't need it for the “legacy” code.
At first I completely removed the legacy bookmark config and used this call to change the parent in the rewrite. Then I reintroduced the legacy bookmark config and had to set the parent explicitly in the rewrite, making this call useless.

But I wonder if I understood the intent of the legacy bookmark config properly. Was it to

keep the bookmark on the same change id;

or to have the bookmark on the first commit?

I assumed 2. If it's 1., then this PR already does that, and legacy-bookmark-behavior=false should only have an effect when used with legacy-change-id=true.

from #5618, which introduced this behavior

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.

so the intent was clearly option 1.

@PhilipMetzger would that work for you if the only way to have the bookmark on the first commit is to set both legacy-bookmark-behavior=false and legacy-change-id-behavior=true?

@PhilipMetzger would that work for you if the only way to have the bookmark on the first commit is to set both legacy-bookmark-behavior=false and legacy-change-id-behavior=true?

In my opinion, this change shouldn't introduce a flag since it is controversial by itself and we ideally only should support one variant of jj split -r X -B X or jj split -r X -A X as a default, this PR makes jj split -r X -B X the default. See Martins comment in your other PR #6458 (comment). I'd also hold off on landing this until we have a consensus on #3419.

I don't think it's about jj split -r X -B X vs jj split -r X -A X, but rather which commit keeps the change id: the one with the selected changes or the other one?

With jj split -A/-B/-d and the selected changes that may be moved anywhere, the commit with the non-selected changes is the only one that makes sense (to me) to keep the change id.
That's why I originally included it in the jj split -A/-B/-d PR.

We could also discuss if the commit with the selected changes should go before or after the other commit by default, but that's not the point of this PR.

Anyway, you seem quite opposed to that change, and I don't remember why. I'll see if I find that info in #3419, and maybe continue the discussion there.

With jj split -A/-B/-d and the selected changes that may be moved anywhere, the commit with the non-selected changes is the only one that makes sense (to me) to keep the change id.

That makes sense. Specifying -A/-B/-d effectively means that the "legacy" option is turned off. I think it's similar to how we implemented jj duplicate -A/-B/-d.

With jj split -A/-B/-d and the selected changes that may be moved anywhere, the commit with the non-selected changes is the only one that makes sense (to me) to keep the change id.
That's why I originally included it in the jj split -A/-B/-d PR.

I still agree with that.

We could also discuss if the commit with the selected changes should go before or after the other commit by default, but that's not the point of this PR.

Anyway, you seem quite opposed to that change, and I don't remember why. I'll see if I find that info in #3419, and maybe continue the discussion there.

I currently don't think that rebase -r X -B X should be default which happens to be the current implementation, although this opinion is a minority but that's talk for #3419.

I think most of these points were already covered by others, but I thought I'd summarize my opinions.

It seems that Yuya's original comment in this thread is already addressed. Re-iterating #3419 (comment), my opinion was that the change id, the bookmarks, and the descendants should all go to the second commit by default (so, with legacy-split-behavior=false, presumably). This seems to agree with Yuya's comment and also seems to match what this PR does right now, so yay!

If we decide to not go with this PR, Philip's suggestion (as I understood it) to have no default and to rely fully on -A/-B/-d seems reasonable to me. IMO (as a bit of a minor aside), that would be blocked on significantly improving¹ the docs for -A/-B/-d, which would be good regardless, but would become urgent and crucial if we remove the default.

However, that does not stop people in the know from experimenting, seeing how convenient that UI is, and how common/useful split -r X -B X (which does seem to match my preferred behavior) is.

Footnotes

The main reason I'm complaining about the docs is that I couldn't tell from them whether the behavior I want is split -r X -B X or split -r X -A X; I had to experiment. Again, I think this is not a huge problem unless we remove the default and people are thus required to learn these flags. ↩

If we decide to not go with this PR, Philip's suggestion (as I understood it) to have no default and to rely fully on -A/-B/-d seems reasonable to me. IMO (as a bit of a minor aside), that would be blocked on significantly improving1 the docs for -A/-B/-d, which would be good regardless, but would become urgent and crucial if we remove the default.

That's correct, but as I mentioned in the issue it should be a temporary stopgap before the users (people who are less involved in the decision process) decide on which behavior they want, this can take up to at least 6 months but that should be no issue (it is extremely likely that -r X -B X will win, since -r X -A X is a minority position).

Currently, with split.legacy-bookmark-behavior set to true, jj split -r X (i.e. the default) and jj split -r X -B X both insert a new commit before X, but the change ID behaviour is different. So it would make sense to me either to a) remove the default and warn if none of -A/-B/-d are supplied or b) keep the default and merge this PR to make the change ID behaviour consistent.

And with split.legacy-bookmark-behavior set to false, it is even harder to reconcile the behaviour of jj split -r X vs jj split -r X -B X. With jj split -r X, it "appears" as though a new commit was inserted after "X", not before, since the selected changes and bookmarks have moved to the newly created commit now with change ID X.

cli/src/commands/split.rs

cenviity · 2025-05-08T00:26:44Z

cli/tests/test_split_command.rs

+    work_dir.write_file("file2", "bar\n");
+    work_dir.run_jj(["describe", "-m", "my feature"]).success();
+
+    let output = work_dir.run_jj([


We need to snapshot the output and log output before running this command to demonstrate the legacy behaviour of the revision with the selected changes (i.e. the 'first commit') keeping the change ID.

cenviity · 2025-05-08T00:34:44Z

cli/tests/test_split_command.rs

+    let output = work_dir.run_jj([
+        "split",
+        "-m",
+        "file1",
+        "-r",
+        "qpvuntsmwlqt",
+        "--insert-before",
+        "qpvuntsmwlqt",
+        "file1",
+    ]);


(not actionable)

See my comment at #6458 (comment) comparing the same set of tests here with differing log outputs for why I think the revision with remaining changes (i.e. the 'second commit') keeping the change ID makes more sense. So I am in favour of this PR.

Here, the command indicates we are inserting a revision before qpvuntsmwlqt, and that is what the log output below shows – vruxwmqvtpmx is the newly inserted revision with the changes that have been split out.

I haven’t been following the latest discussions, so just to reiterate my quite strong view at this point: jj split -r X -d/-B/-A … is creating a new commit from extracted parts of X. The commit that remains behind, containing the parts that were not split out, should be the one to keep the change ID and bookmarks. So in jj split -r X -B X, the second commit should inherit the change ID; in jj split -r X -A X, the first commit should. (I’d rather not speak of “the first/second commit” at all, for this reason, but rather the original commit and the split‐out commit, or similar.)

I think we have pretty strong consensus around this at this point, when operating within the -d/-B/-A framework (but if anyone disagrees they should say so). I believe the remaining disagreement is only about what the default action should be.

I haven’t read the PR or played with the implementation to check if it agrees with my intuitions on how the flags should work, so I don’t know if this comment is useful.

I haven’t been following the latest discussions, so just to reiterate my quite strong view at this point: jj split -r X -d/-B/-A … is creating a new commit from extracted parts of X. The commit that remains behind, containing the parts that were not split out, should be the one to keep the change ID and bookmarks.

In case it weren't apparent, I agree and am in favour of this PR. I was pointing out another way of reasoning that arrives at the same conclusion.

So in jj split -r X -B X, the second commit should inherit the change ID; in jj split -r X -A X, the first commit should.

For the benefit of others following this thread, IIUC I think you meant to say 'the second commit' in both cases, in the context of this PR and the previous paragraph where 'the second commit' is the revision with the parts that weren't split out (the remaining changes). Maybe here you meant 'the second commit' as in the child commit after jj split -r X -B X and 'the first commit' as in the parent commit after jj split -r X -A X.

(I’d rather not speak of “the first/second commit” at all, for this reason, but rather the original commit and the split‐out commit, or similar.)

Agreed. See my comment at #6458 (comment) for cleaning up the terminology now that jj split -A/-B/-d is possible and the 'order' of the 'first' and 'second' commits is less clear and becomes rather arbitrary.

Maybe here you meant 'the second commit' as in the child commit after jj split -r X -B X and 'the first commit' as in the parent commit after jj split -r X -A X.

Right. (A good example of why the terminology becomes inadequate with the new flags.)

cenviity · 2025-05-08T01:57:50Z

See my other comment at #6458 (comment) for a suggestion on cleaning up the ‘first commit’ / ‘second commit’ terminology.

ilyagr

Thanks, @glehmann for working on this and on #6458! Even though I missed most of the discussion, I really appreciate it!

Upon a quick test, unless I missed something, this PR's behavior at the moment is exactly what I thought the default behavior should be. So, it SGTM, but it sounds like we need to decide between this and Philip's suggestion of having no default behavior (see my other comment; perhaps there are also other suggestions I am less aware of), and there's no rush on that.

Whatever we decide in the end, having the option to play with all the behaviors we were discussing is really nice!

ilyagr · 2025-05-17T00:15:39Z

CHANGELOG.md

+* `jj split` assigns the change id and the bookmarks of the source revision
+  to the second revision. 
+  You can opt out of this change by setting `split.legacy-bookmark-behavior = true`,
+  but this will likely be removed in a future release.


Reminder: let's make sure the commit description, this changelog entry, and the docs are all consistent before merging (if we get to that point).

Currently, the commit description still says that split.legacy-bookmark-behavior defaults to true, but it actually defaults to false. It seems this might change, but whatever we do, we should double-check that we are consistent before merging.

ilyagr · 2025-05-17T00:29:13Z

cli/src/commands/split.rs

@@ -300,6 +309,9 @@ pub(crate) fn cmd_split(
    if legacy_bookmark_behavior {
        tx.repo_mut()
            .set_rewritten_commit(target.commit.id().clone(), second_commit.id().clone());
+    } else {
+        tx.repo_mut()
+            .set_rewritten_commit(target.commit.id().clone(), first_commit.id().clone());


I think most of these points were already covered by others, but I thought I'd summarize my opinions.

It seems that Yuya's original comment in this thread is already addressed. Re-iterating #3419 (comment), my opinion was that the change id, the bookmarks, and the descendants should all go to the second commit by default (so, with legacy-split-behavior=false, presumably). This seems to agree with Yuya's comment and also seems to match what this PR does right now, so yay!

If we decide to not go with this PR, Philip's suggestion (as I understood it) to have no default and to rely fully on -A/-B/-d seems reasonable to me. IMO (as a bit of a minor aside), that would be blocked on significantly improving¹ the docs for -A/-B/-d, which would be good regardless, but would become urgent and crucial if we remove the default.

However, that does not stop people in the know from experimenting, seeing how convenient that UI is, and how common/useful split -r X -B X (which does seem to match my preferred behavior) is.

Footnotes

The main reason I'm complaining about the docs is that I couldn't tell from them whether the behavior I want is split -r X -B X or split -r X -A X; I had to experiment. Again, I think this is not a huge problem unless we remove the default and people are thus required to learn these flags. ↩

The first commit contains the split out changes. The second commit usually keeps most of the changes, the change id and the attached bookmarks. This no-implicit-move behavior aligns with other jj commands. With the -A/-B/-d options, the split-out commit may be moved anywhere. With commit tree @ pmpklomk (empty) (no description set) ○ pzskstlk *letters* fileB ○ kmvzrqtu fileA │ ○ lyzvpymy *numbers* file3&C │ ○ kpmxxkwk file2 │ ○ xzvtlrrt file1 ├─╯ ◆ zzzzzzzz root() when splitting `fileC` out of `lyzvpymy` to the other branch with jj split -r lyzvpymy -d pzskstlk the change id and the bookmark stay in the branch with the source commit. @ pmpklomk (empty) (no description set) ○ vowztxqo fileC ○ pzskstlk *letters* fileB ○ kmvzrqtu fileA │ ○ lyzvpymy *numbers* file3 │ ○ kpmxxkwk file2 │ ○ xzvtlrrt file1 ├─╯ ◆ zzzzzzzz root()

cenviity

With the -A/-B/-d options, the split-out commit may be moved anywhere.
With commit tree
@  pmpklomk (empty) (no description set)
○  pzskstlk *letters* fileB
○  kmvzrqtu fileA
│ ○  lyzvpymy *numbers* file3&C
│ ○  kpmxxkwk file2
│ ○  xzvtlrrt file1
├─╯
◆  zzzzzzzz root()
when splitting fileC out of lyzvpymy to the other branch with
jj split -r lyzvpymy -d pzskstlk
the change id and the bookmark stay in the branch with the source commit.
@  pmpklomk (empty) (no description set)
○  vowztxqo fileC
○  pzskstlk *letters* fileB
○  kmvzrqtu fileA
│ ○  lyzvpymy *numbers* file3
│ ○  kpmxxkwk file2
│ ○  xzvtlrrt file1
├─╯
◆  zzzzzzzz root()

I noticed that in the current commit message the command says -d pzskstlk but the resulting tree looks like -A pzskstlk was applied instead (the new change vowztxqo is inserted between pzskstlk and its child. So, either the command or the resulting tree needs to be updated.

Another nit – perhaps use diff summaries rather than descriptions to show the before and after states? The description for lyzvpymy should remain the same after the split.

ilyagr · 2025-05-20T00:56:27Z

Is this issue with -d related to this PR (beyond its commit description) as it is now, or should it be a separate issue?

Perhaps we should remove the discussion of -d/-A/-B from the commit description, assuming that this PR does not change what -d/-A/-B do?

cenviity · 2025-05-20T01:29:48Z

Is this issue with -d related to this PR (beyond its commit description) as it is now, or should it be a separate issue?

Perhaps we should remove the discussion of -d/-A/-B from the commit description, assuming that this PR does not change what -d/-A/-B do?

I think the example is useful to demonstrate that the change ID lyzvpymy stays in place rather than get taken by the newly created commit, which is what this PR changes. It's just that the argument used in the command there doesn't match the output tree.

yuja · 2025-05-20T01:57:55Z

Is this PR still relevant for review? or are we just discussing a potential default to be introduced later? iirc, the idea was to try out the new -A/-B/-d flags for a couple of weeks, and pick one as the default.

glehmann · 2025-05-20T10:56:17Z

iirc, the idea was to try out the new -A/-B/-d flags for a couple of weeks, and pick one as the default.

yes, no hurry to review this PR

glehmann · 2025-05-20T11:02:34Z

I noticed that in the current commit message the command says -d pzskstlk but the resulting tree looks like -A pzskstlk was applied instead (the new change vowztxqo is inserted between pzskstlk and its child. So, either the command or the resulting tree needs to be updated.

Good catch!
On the other hand, I think that @ilyagr is right: this PR is not about the behavior with -A/-B/-d. This behavior is already this one when using these flags.
I'll use another example that doesn't use the -A/-B/d flags

cenviity · 2025-05-21T03:59:23Z

On the other hand, I think that @ilyagr is right: this PR is not about the behavior with -A/-B/-d. This behavior is already this one when using these flags.

Ah yes, I'd forgotten that the new flags already exhibit the behaviour in this PR.

glehmann requested a review from a team as a code owner May 6, 2025 11:26

PhilipMetzger reviewed May 6, 2025

View reviewed changes

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

glehmann force-pushed the gln/split-command-flags-uoov branch 2 times, most recently from 96bca11 to 252108e Compare May 6, 2025 11:49

cenviity reviewed May 6, 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

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

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

glehmann force-pushed the gln/split-command-flags-uoov branch 3 times, most recently from b742f09 to 9f1da45 Compare May 6, 2025 20:38

yuja reviewed May 7, 2025

View reviewed changes

glehmann force-pushed the gln/split-command-flags-uoov branch 2 times, most recently from 4c6b342 to ac0bb82 Compare May 7, 2025 21:23

cenviity mentioned this pull request May 8, 2025

split: implement -A/-B/-d flags #6458

Merged

4 tasks

cenviity reviewed May 8, 2025

View reviewed changes

glehmann force-pushed the gln/split-command-flags-uoov branch 4 times, most recently from 30db138 to 9a8c1c0 Compare May 11, 2025 12:48

PhilipMetzger mentioned this pull request May 11, 2025

split: use "selected/remaining changes" instead of "first/second commit" #6509

Merged

4 tasks

glehmann force-pushed the gln/split-command-flags-uoov branch 7 times, most recently from dca26ed to 7d50be8 Compare May 16, 2025 09:36

ilyagr reviewed May 17, 2025

View reviewed changes

glehmann force-pushed the gln/split-command-flags-uoov branch from 7d50be8 to e58b7ad Compare May 17, 2025 21:33

cenviity reviewed May 19, 2025

View reviewed changes

split: the second commit keeps the change id #6466

Are you sure you want to change the base?

split: the second commit keeps the change id #6466

Uh oh!

Conversation

glehmann commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PhilipMetzger May 7, 2025 • edited by cenviity Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuja May 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

ilyagr May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cenviity May 20, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

cenviity May 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

cenviity May 8, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

cenviity commented May 8, 2025

Uh oh!

ilyagr 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!

Choose a reason for hiding this comment

Uh oh!

ilyagr May 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

cenviity left a comment

Choose a reason for hiding this comment

Uh oh!

ilyagr commented May 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cenviity commented May 20, 2025

glehmann commented May 6, 2025 •

edited

Loading

PhilipMetzger May 7, 2025 •

edited by cenviity

Loading

yuja May 8, 2025 •

edited

Loading

ilyagr May 17, 2025 •

edited

Loading

cenviity May 20, 2025 •

edited

Loading

cenviity May 8, 2025 •

edited

Loading

cenviity May 8, 2025 •

edited

Loading

ilyagr left a comment •

edited

Loading

ilyagr May 17, 2025 •

edited

Loading

ilyagr commented May 20, 2025 •

edited

Loading