split: the second commit keeps the change id

[glehmann]

based on: #6458

Checklist

If applicable:

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

[cenviity]

See my other comment at https://github.com/jj-vcs/jj/pull/6458#issuecomment-2860861884 for a suggestion on cleaning up the ‘first commit’ / ‘second commit’ terminology.

[ilyagr]

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]

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]

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]

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]

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]

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.

[ilyagr]

Thanks for updating this; I use it and it makes it easier to use. :)

[glehmann]

Thanks for updating this; I use it and it makes it easier to use. :)

You're welcome!

I'm using the -A/-B/-d flags even more than I thought I would, but when I'm not using them, I'm indeed very pleased to have jj compiled with this change on my computer :) I hope it will make it to the main branch eventually.

In the meantime, I'll continue to refresh this PR (hopefully with all the tests passing!).

[glehmann]

Some time has passed since merging the -d/-A/-B flags, and hopefully, we have had a chance to experiment with the new split behavior. @jj-vcs/maintainers Do you have any opinion on merging this PR?

[martinvonz]

I have personally found the new flags very useful for extracting parts of a commit to an independent commit (-d main). I haven't tried to use them in place of the default behavior, however. My guess is that I would generally want the change id and the bookmark to stay on the parent commit, however. Maybe others have tried -A/-B @?

[dnnr]

I wondered today what I could do to get this exact behavior as my default and I'm glad to see that this PR exists. For what it's worth, I'd appreciate seeing this merged! :pray:

[emilazy]

FWIW, I still regularly feel pain from the lack of this and would very much like to either see it land, or see the default split behaviour dropped entirely. I think if we have a default it must correspond to one of the destination flags, and the -B this change implements is both the closest to the current behaviour, the only option that never results in conflicts, and the option that is consistent with the order we prompt for descriptions in.

Dropping the default and requiring an explicit specification would be an acceptable alternative, but IMO at a high cost to convenience. I won't repeat all the pages of earlier discussion from elsewhere on Discord/GitHub, but I agree with @ilyagr's comment from a while ago that -r X -B X is the "one true split" if you are going to privilege anything at all - as evidenced by the fact that the other destination flag options are built on top of it and correspond to rebasing the resulting new commit away.

Would be great if maintainers could make a decision here.

[martinvonz]

At Google, we do stacked diffs and use a bookmark to identify each review in the stack. In that workflow, it seems clear from internal feedback that most people expect the parent to keep the change id and the bookmark. I'm pretty sure they still expect descendants to be rebased onto the child, though.

[martinvonz]

I just noticed that we have 31 upvotes on the internal tracking bug for making the bookmark end up on the first commit. That's our 3rd most upvoted bug (after "support copies and renames" and "make jj fix format only changed lines").

[glehmann]

I use stacked diffs too—probably not at the google scale though!—with the behavior from this PR.

IMO what is important is to have to bookmark that stays on the same revision, which is not currently the case, but is the case either with split.legacy-bookmark-behavior or with this PR. Am I missing something?

[martinvonz]

I haven't had time to re-review this PR so I'm not sure, but the users at Google seem to want the bookmark to stay on the parent commit.

[matts1]

Speaking for gerrit users, with my new behavior added in jj gerrit upload, it directly maps the jj change ID to a gerrit change.

This means that which of the two commits keeps the change ID matters. And not only does it matter, but sometimes I want the parent commit to be associated with a change, and other times I want the child commit.

Our old internal system at google would seem to match this train of thought. I forget the precise workflow since I haven't used fig in ages, but I think when you ran hg split, it asked "which of the two commits do you want to associate with the exsiting CL uploaded for code review".

My personal thoughts is that I don't really care whether we choose the first or the second commit to associate with the old change by default. However, I strongly advocate that this be a command-line flag because the same user could want different choices at different times.

Also, I strongly believe that whichever commit retains the Change ID should retain the bookmarks.

[martinvonz]

I'm still not sure which commit I think should keep the change id when doing a regular jj split -r (i.e. not -o/-A/-B), but this PR actually seems like an improvement regardless because I think everyone(?) agrees that the bookmark should follow the change id. Any objections? I haven't re-read all the discussion above.