jj icon indicating copy to clipboard operation
jj copied to clipboard

What should `jj split` do with bookmarks?

Open emesterhazy opened this issue 1 year ago • 42 comments
trafficstars

I'm forking this off of https://github.com/martinvonz/jj/issues/2274#issuecomment-2028818012 where Martin wrote:

I don't think that checking out the second commit is an issue (presumably we have to pick one), but perhaps we should leave any branches in a conflicted state. Thoughts?

Good question. I think it would make more sense to leave the branch in a conflicted state, but it seems like it's practically quite useful to leave it on one of the commits instead. That way it requires action from the user in some of the cases instead of all of the cases. I'd say we should leave it that way for now and change it later if we hear that it's confusing - or worse, that it leads people to push the wrong commit to a remote.

By the way, we have had a discussion internally at Google about where regular (parent/child) jj split should put the branch. It currently leaves it on the child, but it lets the parent keep the change id. We use branches for tracking code reviews at Google (I know you're at Google, @emesterhazy; I'm just providing context for others). For that use case, it's confusing that the branch doesn't follow the change id. It makes sense for the branch to move to the child in more Git-like workflows, since branches typically point to the tip of a chain of commits in Git. However, I don't want us to design a behavior that makes less sense just because it better matches Git. What do people think?

I'm going to think about this a bit more before giving an opinion, but I wanted to give this its own issue for discussion.

emesterhazy avatar Apr 01 '24 23:04 emesterhazy

I've been thinking about this a bit more and would like to propose that we change the current behavior.

Currently, the ~branch~ bookmark follows the child revision, moving forward the way branches usually do in a git workflow.

  • This doesn't make a lot of sense from the jj centric viewpoint though where bookmarks are aliases for a revision / change id, and it causes issues for workflows where each change id is reviewed separately (stacked PRs and Google's internal workflow).

Instead of moving the bookmark to the child revision, I think split should leave bookmarks in place and attached to the same change id before and after the split. If users want to move the bookmark, they can. This no-implicit-move behavior aligns the change that makes abandon drop bookmarks instead of moving them to the parent revision.

@PhilipMetzger, @martinvonz What do you think?

emesterhazy avatar Feb 06 '25 16:02 emesterhazy

I also think it makes sense, since it aligns with the proposed model for jj gerrit send (#2845). It also explicitly requires a jj bookmark move which I also find great.

PhilipMetzger avatar Feb 06 '25 16:02 PhilipMetzger

I agree. I think many people think of bookmarks as pointing to a change id (even though that's not how it's actually implemented), so I think it makes sense if split leaves the bookmark pointing to the first commit. If we ever add a flag for having the second commit keep the change id instead, then the bookmark should point there instead, IMO.

martinvonz avatar Feb 06 '25 16:02 martinvonz

It would also be consistent with the idea of writing jj split -r X as jj squash -i --from X -A X (i.e. taking part of X and putting it in a new commit inserted after X).

martinvonz avatar Feb 06 '25 16:02 martinvonz

Is there a issue open for the jj squash -A flag? I'm having trouble finding it and it doesn't look like that flag exists currently. The symmetry is definitely nice though.

I think making this change to jj split should be pretty simple. I'll take a look.

emesterhazy avatar Feb 06 '25 16:02 emesterhazy

Is there a issue open for the jj squash -A flag? I'm having trouble finding it and it doesn't look like that flag exists currently. The symmetry is definitely nice though.

I don't think we have an issue specifically for that, but #4708 is closely related. It might be that it's better to add those flags for jj split instead since jj squash doesn't currently create new changes (it might have been more natural with the old name jj move).

I think making this change to jj split should be pretty simple. I'll take a look.

Thanks!

martinvonz avatar Feb 07 '25 18:02 martinvonz

FYI for readers, there's a substantial amount of additional discussion about the merits of this change in the pull request:

  • https://github.com/jj-vcs/jj/pull/5618#discussion_r1948029105

emesterhazy avatar Feb 09 '25 20:02 emesterhazy

Here is an example of someone enjoying the current behavior: https://github.com/jj-vcs/jj/discussions/2425#discussioncomment-12117924

steveklabnik avatar Feb 10 '25 21:02 steveklabnik

Use jj split -i instead of jj commit.

It seems pretty clear this this is a workaround for the fact that there's no good way to add commits and move the bookmark at the same time. I don't think we should optimize for jj split being used in this way, but it drives home the point that we need to provide better solutions for users.

I suspect this person doesn't know about experimental-advance-branches since it would make jj commit to behave the way they want it to.

emesterhazy avatar Feb 10 '25 21:02 emesterhazy

I don't think we should optimize for jj split being used in this way, but it drives home the point that we need to provide better solutions for users.

Yeah, just to be clear, I'm not advocating for a specific decision here, just like, both of these things appeared in my feed next to each other and wanted to make sure they were linked up :)

steveklabnik avatar Feb 10 '25 21:02 steveklabnik

The proposed behaviour can be better even for a PR workflow: it lets you split out changes that shouldn’t be in that PR (because they should be postponed to a later PR, or kept in your local stack of changes, or whatever). That is something I actually do sometimes! So I feel like the PR workflow is a bit of a red herring here; I’m not convinced there’s an obvious choice based solely on it.

Given that, I think that it makes sense to keep bookmarks on the change that keeps the original change ID, based on the general principle of what keeping the original change ID means and the fact that it makes more sense with the branch names jj git push -c creates. I think having an option to let the “other side” keep the change ID could make sense, and in that case that commit should get the bookmarks instead. But the current behaviour seems generally wrong to me and I support changing it.

emilazy avatar Feb 10 '25 22:02 emilazy

Based on the discussion in https://github.com/jj-vcs/jj/pull/5618, we are going to do the following:

  1. Change the default behavior so that the change id moves to the second commit. This will keep bookmark associated with the same change id before and after the split.
  2. Add a config options that control whether the change id and bookmark move to the first commit or second commit created by the split.
  3. Add a warning that prints on jj split when the config option is missing to tell users that the default will change from second commit to first commit in the next release.
  4. Wait one release cycle and then change the default.
  5. Remove the config option in ~6 months unless there is strong opposition at that time.

We came to this decision in light of the fact that some users find the current behavior helpful for their Git branches workflows. Hopefully we will have a more elegant solution in 6 months.

emesterhazy avatar Feb 12 '25 13:02 emesterhazy

The configuration option should probably restore the current behaviour of mismatching change ID and bookmarks target, right? I don't like it, but @martinvonz's concern was about workflow compatibility and it seems like there wouldn't be a value of the option that isn't a breaking change otherwise.

emilazy avatar Feb 12 '25 14:02 emilazy

Yes, my concern is that people are used to the current behavior and I don't want to surprise them. I think you're saying that you think there are few enough people who depend on the current behavior that it's okay to make it a breaking change. Is that correct? If that seems to be the consensus, then I'm fine with doing that. What do the other maintainers think?

martinvonz avatar Feb 12 '25 18:02 martinvonz

I don’t have a strong opinion on that. My guess is that most people have no real expectation for where the bookmarks go, that people with experience with Jujutsu’s model would expect them to go with the change ID and find the current behaviour surprising, and that some people (as evidenced by comments elsewhere) have noticed the current behaviour and use it to do a bit less bookmark management than they otherwise would.

What I was saying is that if we add a temporary option as a compatibility mechanism, it should restore the current behaviour wholesale – the change ID goes to the first commit, but the bookmarks to the second. In my understanding, @emesterhazy’s proposal doesn’t do that; it would choose where the change ID goes, and move the bookmarks to that. That’s more sensible behaviour – and what I think we’d want if we add a flag to jj split to decide where the change ID goes – but it’s not a compatibility mechanism for the existing (weird) behaviour.

emilazy avatar Feb 12 '25 19:02 emilazy

Oh, I see. Yes, what I had in mind was the compatibility option you were talking about. I hope that's the only config option we need. I can see a flag being useful for making the change id and bookmarks go to either the first or second commit (but what what about others when we add support for multi-way splits?), but I hope we won't need a config option for it because it seems like it shouldn't be about personal preference.

martinvonz avatar Feb 12 '25 19:02 martinvonz

So we just want a config that restores the current behavior where the bookmark moves off of the change id?

I guess that's simpler to implement, but it's still bad because it breaks jj git push -c xyz. I think what people really care about is that the bookmark moves forward, but I'm guessing they won't mind if we fix the jj git push -c case as well.

Fine with me to make the config option strictly backwards compatible, at least at first, but I also think moving the change id as well will be more useful for users.

emesterhazy avatar Feb 12 '25 19:02 emesterhazy

Yes, the config option I'm proposing would exist to make the transition smoother for users. It would also be useful for giving users a way to go back to the old behavior once we've turned it on by default, in case there are problems we had not considered (and that didn't affect users who opted in before it was turned on by default).

You can even call the config option split.legacy-bookmark-behavior or something to make it very clear what it's about.

martinvonz avatar Feb 12 '25 19:02 martinvonz

I tried the new behavior out, and I'm surprised the "new" behavior does not keep the working copy with the first commit. In other words, for jj split --config split.legacy-bookmark-behavior=false -r @, even if there are no bookmarks, if we are serious about making the parent commit into the proper inheritor of all of the original commit's properties, the working copy should stay there. What do people think?

Also, I noticed that the warning is printed even if there are no bookmarks at the commit being split. I think that's a bit confusing, though if we make the change I allude to above, we'd also want the warning to print on jj split -r @.

(My belief is still that it's also useful and somewhat important to have a flag to move everything -- change id, working copy, bookmars -- to the child commits, but that's a separate point. I'm repeating it mainly because I seemed to say "option" accidentally before where I meant "flag").


Update:

As an illustration, if we start with a single commit with a bookmark that is the working copy:

A @ bm

Currently jj split --config split.legacy-bookmark-behavior=false @ results in

B @
^
|
A branch

whereas

B
^
|
A @ branch

might make more sense. If we go with this, I think we're very likely to need a flag that results in

A @ branch
^
|
C

ilyagr avatar Feb 14 '25 02:02 ilyagr

Emily on Discord pointed out that keeping the working copy at the parent commit would lead to change in the working copy tree. So, perhaps we finally have a reason why it might be better to move everything over to the second commit.

Or, perhaps, moving the working copy separately from bookmarks is not very confusing. Emily also pointed out that this is what jj new does.

ilyagr avatar Feb 14 '25 02:02 ilyagr

Stream of consciousness from Discord:

I see two possible resolutions

  1. actually jj split should specifically advance @ for the same reason jj new does
  2. actually we should have been making the second commit get the change ID and bookmarks all along

[…]

(I think that the rationale that convinces me about bookmarks – "if bookmarks can go to one of two commits and one of them shares the change ID, it should be that one" – doesn't convince me about @. I can try to rationalize why, but I'm just recording that when I s/bookmarks/the working copy/, I don't find myself convinced.)

(something like "moving around the working copy is normal and multiple commands do it" / "we are very explicit about bookmark updates and it's a much bigger deal what commit a bookmark points to than what the working copy points to")

[in reply to @ilyagr preferring option 2] FWIW, I am sympathetic to 2, because I usually find I'm splitting "some extra stuff I shouldn't have done in this change" earlier in the stack rather than vice versa

[…]

"an option to keep everything at the parent (keeping the working copy)" is interesting to me, because it would work well with the "wait, this shouldn't be in this stack" use case that I pointed out can be useful for PR workflows just like the opposite can be

I still feel weird about (non-parallel; --parallel is weird and I have no strong intuitions about it) jj split changing the working copy, though

emilazy avatar Feb 14 '25 02:02 emilazy

Emily on Discord pointed out that keeping the working copy at the parent commit would lead to change in the working copy tree. So, perhaps we finally have a reason why it might be better to move everything over to the second commit.

I think this is also a pretty strong justification for keeping bookmarks on the second commit by default actually, since it would mean that jj split never changes the tree of any bookmark, which seems consistent.

Also, if we had support for splitting into N commits in the future (instead of only 2), it would seem weird for the bookmark to include only the changes from the first split commit, but not the other (N-1) split commits. So taking the case of N=2, it makes sense that all of the properties would remain on the last commit in this case as well.

(I actually had originally thought that it would be better to keep all of the properties the first commit, but this way of thinking about it changed my mind.)

scott2000 avatar Feb 14 '25 02:02 scott2000

So taking the case of N=2, it makes sense that all of the properties would remain on the last commit in this case as well.

So, this same logic would apply to duplicate, when it has a bookmark?

joyously avatar Feb 14 '25 02:02 joyously

So taking the case of N=2, it makes sense that all of the properties would remain on the last commit in this case as well.

So, this same logic would apply to duplicate, when it has a bookmark?

I wouldn't think so, because duplicate creates duplicate commits with brand new change IDs, so it wouldn't make sense to move existing bookmarks onto the duplicate commits.

scott2000 avatar Feb 14 '25 02:02 scott2000

I think this is also a pretty strong justification for keeping bookmarks on the second commit by default actually, since it would mean that jj split never changes the tree of any bookmark, which seems consistent.

I disagree with this line of thinking. There's nothing inherently good about preserving the tree of the target change id, so I think this devolves to a discussion of where you prefer the bookmark to end up.

Usually when I do a split I think of it as moving changes out of the target commit. I want the tree to change, that's why I'm doing the split. Sometimes the changes are useful and I'll include them in a current or future PR, but often they're garbage that I'm planning to abandon. I don't want the target change id and bookmark to move to the commit with the garbage in it ;)

emesterhazy avatar Feb 14 '25 06:02 emesterhazy

There is a long continuation of the discussion starting here: https://github.com/jj-vcs/jj/pull/5828#issuecomment-2690975041

I might try to move those comments here.

ilyagr avatar Mar 05 '25 04:03 ilyagr

I'll repost Emily's great summary in https://github.com/jj-vcs/jj/pull/5828#issuecomment-2691132725, since I want to reference parts of it (mostly item 8 below, a small subset of the post).

Here's @emilazy 's original post:


Summary of the Discord discussion, necessarily coloured by my POV but I will try to weigh all the different positions expressed:

  1. We’ve had a hard time communicating about all of this and a lot of the agreement on the previous PR involved being confused about what the actual state after it would be or not having fully thought things through.

  2. Everyone agrees that the behaviour as of the current stable release is bad and we need to fix it. I don’t think anyone likes the status quo.

  3. The current state of trunk is that the change ID and bookmarks go to the first commit in a split, but @ moves to the second. @ilyagr, @scott2000, and I all believe that this is a problematic inconsistency: for the same reason that bookmarks should move with the change ID, so should the working copy. My understanding of @emesterhazy’s position is that he doesn’t necessarily agree, but is open to discussion about it. I consider this inconsistency bad enough that it’s a signal we need to work out the actual desired semantics rather than going forward with a breaking change we have conflicting intuitions about and lack a shared coherent model to justify.

  4. The behaviour at trunk – ignoring the @ issue – is well‐suited to a workflow where you want to evict some changes from a change; say if you got a review saying that some extraneous change on your CL/PR should be split out for later review. In that case, you want the change ID and bookmarks to go with the commit that lost the split‐out diff hunks.

  5. There is another legitimate and common workflow: splitting preliminary work out of a change: say, moving a simple preparatory refactor or rename as a new parent for a commit that implements a new feature, for the new feature to be stacked on top of. In this case, you want the change ID and bookmarks to go to the second commit. I believe that @ilyagr, @scott2000, and I all personally end up doing these kinds of splits more than (4). The behaviour of the stable release is closer to this workflow (modulo our unanimous agreement that the change ID should certainly go to the second commit in this case).

  6. I understand @emesterhazy usually does (4). @ilyagr, @scott2000, and I all think that for both of these workflows, it makes more sense for @ to move with the change ID. I believe @emesterhazy is not as convinced.

  7. There are “Git compatibility” workflows that dovetail better with the stable release behaviour of (5): “jj split as git commit -p”, adding new commits while sliding a pull request bookmark before, etc.; none of the people who consider (5) a legitimate and important workflow do so because of these compatibility concerns. It’s not a matter of constraining Jujutsu’s design based on GitHub; all the people who value (5) do so invariant of such concerns. In fact, Gerrit‐style change‐based code review where change IDs are semantically meaningful and reviews are attached to them helps motivate our position on how the change ID should move for (5). People who use jj split for these compatibility properties still must be taken into account when considering compatibility shims and breaking changes, but none of the motivations here are about that.

  8. One proposal was to make (5) the default behaviour (so jj split would behave like the stable release except the change ID would move to the second commit for consistency), and add a flag like --evict/--away/--yeet/… that implements (4). I feel that we were moving in the direction of something vaguely like consensus on this, but the default mode was still a point of contention, and we had a jumble of confusing and conflicting intuitions about how @ should move and how descendants should be rebased. We were also uncertain about whether you should be selecting changes to keep or changes to split out (i.e., which side goes where).

  9. As it seemed like we will not be able to get a consensus on the exact behaviour jj split should have by the release, and there are significant concerns about the behaviour in trunk that we did not fully realize when the original PR was merged, I think we all uneasily agreed that it’s sensible to roll back the default behaviour for now to take the time pressure of the upcoming release off. Restoring the default behaviour of the previous stable release is a conservative change that avoids committing to a breaking change that we don’t seem to have full consensus for. It’s not an endorsement of the stable behaviour, though, which again everyone agrees is bad.

  10. At that point, I brought up the idea that had come up before of giving jj split arguments for -d/-B/-A, a la new/rebase/duplicate. Some subsequent discussion revealed that this seems like a promising avenue to resolve these confusions and conflicts:

    1. It provides a simple model for answering questions about change IDs, bookmarks, @, and descendants: the -r commit keeps its change ID and bookmarks in all cases, just like those other commands; @ remains there if it was already there; descendants are rebased according to what destination option you use. With these destination flags, no other behaviour really makes sense.

    2. -r X -B X supports the (5) workflow, and is almost identical to the stable behaviour modulo change IDs.

    3. -r X -A X supports the (4) workflow. In this case, you select which changes would be split out into the parent, which differs from the behaviour at trunk, but is more consistent with the model implied by these flags (and personally I think this is usually how you would expect to select hunks given this interface). Unlike the --evict we were discussing, this also has reasonable behaviour with the squash workflow: -r @ -A @ leaves the working copy without the split‐out changes, while -r @- -A 'next()' (where next() is @ilyagr’s name for @-+ ~ @, the commit jj next would move to) achieves the same for a commit you are on top of but not directly editing.

    4. -r X -d X- supports a workflow we were calling --evict-new, where you want to split out some work from a stack entirely, and don’t want it to be incorporated into the chain sof descendants at all. This also helped resolved some of our confusion about descendant rebasing.

    5. It also unlocks some nice new workflows: jj split --after 'trunk()' --before @- can add a new commit as a parent to a mega‐merge, jj split -r X --before Y can slot a change in place elsewhere in a stack or into a specific stack below a mega‐merge, and so on.

    6. It seems to extend naturally to the n‐way split case.

    7. It helps us explain --parallel’s behaviour: -r X --parallel is equivalent to -r X --after X- --before X+ (in the single‐commit case).

    8. jj split and jj squash would become exact inverses that can undo each other. (Actually, they would become almost the same command: if --into was a destination argument like -d/-B/-A are, they could be unified entirely. I think this is fascinating and promising, but also a rabbit hole that we shouldn’t block fixing jj split on.)

    9. There is a risk that operations like -r X -A X would create conflicts in @ when selecting overlapping changes, in the same way that -r X --parallel can today. This is equivalent to the problem jj squash --from ‹parent› has. It is not yet clear how much of a workflow problem this would be, which is why we’d need to experiment with how it feels before committing. @scott2000 points out that this is not avoidable in the general case:

      I don't think there's any easy way to solve this in general actually, since diff editors can produce arbitrary trees (like they don't even have to select whole lines). It becomes especially obvious when you look at -d @- (or --parallel). Let's say that @- has tree X, @ has tree Y, and the user selects tree Z in the diff editor. The 2 resulting commits must have trees (Y-Z+X) and Z respectively. It's clear that (Y-Z+X) can have conflicts depending on what the user selected. The only thing that changes depending on whether we have the user select changes to keep or changes to remove is which of these two commits gets the conflict.

      but also proposes a potential solution that could solve this for both jj split and jj squash:

      Yeah, I'm thinking adding destination arguments and saying "select what you want to put in the new commit" would work in most cases, and it would put jj split in the same spot as jj squash is currently. Maybe in the future we could solve the conflict problems with both of them at the same time (e.g. by adding a --keep-selected flag to both of them which keeps selected changes in the source commit instead of moving them to the destination commit).

    10. We still do not have consensus on what the default behaviour should be if we add these flags. I believe the conservative option would be for -r X to imply -B X by default, since it is so close to the stable behaviour and therefore minimally‐breaking. -r X -B X is also a case where conflicts will never be created, as with the current jj split. (It’s not wholly unbreaking; someone could be depending on the current change ID behaviour. My sense from experience is that when you care about change IDs, you’d prefer the new, more consistent behaviour, and when you don’t, it doesn’t matter. It’s less breaking than the change in trunk, in any case.)

      I also personally believe that -B X is usually what I (and @ilyagr, and @scott2000) want independent of compatibility concerns, per what I said about the frequency of these relative workflows in (5). However, there is not agreement on this. I know that @emesterhazy thinks that the preference of (5) over (4) is because of the (7) use case of bookmarks for Git compatibility, which I disagree with – I would want (4) more often even if I never used bookmarks and was doing things in a Jujutsu‐native environment with a custom Gerrit‐style forge. We will have to make some decision on what default we should pick for the long term at some point. We could even decide to make a destination argument mandatory if we can’t agree on what workflow to prioritize. It may be that we have to introduce a setting for this, if we can’t hash out the disagreement here.

Although we do not have consensus on what mode/workflow should be preferred by default, or how important the matter of @ is, given the conversation, I believe that we all ended the discussion at least relatively happy with the idea of “restore the default behaviour for the upcoming release, then implement -d/-B/-A options for jj split and see how it feels in practice without immediately committing to that path”. I hope that this comment can serve as a reasonable summary of everyone’s positions in that discussion and that we can continue discussing from this. Please do correct me if you think I have misrepresented your position in this comment, especially @emesterhazy.

In terms of this PR, I am fine merging it; given all the above considerations, I think it is better to ship no change to the default behaviour in the next release than it is to commit to the breaking change in trunk. I personally think that the option would ideally be renamed, because although (10) would support the same workflow turning off the “legacy” behaviour supports, it would not behave in exactly the same way, and therefore the “non‐legacy” behaviour here is something that I think we wouldn’t want to have if (10) goes well. However I don’t want to block on that; it is undocumented, after all, so we’re not really offering any stability guarantees about it or enticing users to set it, and this has taken a lot of all of our energy already so I have no desire to pick further nits :)

I also hope that we can resolve this soon in a way that everyone can be happy with, and my hope is that, despite the remaining disagreements, (10) offers a potential path forward that everyone who participated in the discussion seems to consider at least a promising avenue of experimentation.

ilyagr avatar Mar 11 '25 04:03 ilyagr

I have a few points in mind, in no particular order. Let me know if something is unclear, at some point I decided I should stop editing my answer and just post it.

Summary: I try to make a clear argument for moving everything to a second commit by default. I also explain why I think we might want something like --evict (likely in addition to -A/-B, not instead of).

One true split

I think the "one true split" that should be the default is the one that moves the working copy, the change id, the bookmarks to the child commit, and rebases all the children of the original commit to the child commit.

I think there is a better answer than the one I previously gave to @martinvonz 's question in https://github.com/jj-vcs/jj/pull/5618#discussion_r1948490921,

if we ignore PRs/MRs, do you think the bookmark should end up on the first or second commit? IMO, it's at least clear that it should end up on the commit that inherited the change id. It's less clear if that should be the first or the second commit.

Moving everything to the second commit simply the most consistent option that preserves the most invariants, and is thus easiest to reason about. Importantly, in addition to preserving what I listed above, moving the change id to the second commit preserves the snapshot of the change with that change id. If the original change is the working copy, it preserces the file contents of the working copy directory. There is no need to reload any editors, nor to reason about how the working copy changed before continuing to edit it (if that's what you want to do).

With this behavior of split, the answer to pretty much any question of the form "does the split seem to preserve X" seems to be yes, even when the user is thinking in terms of snapshots rather than patches (Update: See following comments for more on the text in italics). Let me know if I missed something.

As another example, @krobelus made a UI that depends on the split preserving the working copy files.

Another UI to keep in mind, and why -A/-B does not seem to me to be a full solution

(By "another UI", I mean "in addition to UIs like @krobelus's I mentioned above)

This is also a reply to the remark from the discussion:

We were also uncertain about whether you should be selecting changes to keep or changes to split out (i.e., which side goes where).

I think an important UI that I'd like to continue working well is jj split --tool meld-3. I am a big fan of it, though I'm not sure we advertise it enough (nor am I sure how to advertise it better)

With this UI, if you jj split --tool meld-3 -r A file, you get 3 panes, where you can edit the middle one:

Version of file at A- Version of file that will end up in the parent commit (Initially same as right side) Version of file at A

This makes sense with the current UI, the UI Evan implemented, and the --evict UI discussed in item 8 of Emily's comment above. It makes less sense with the rebase-like UI. It's valuable because it essentially shows you the trees (well, one file in the tree) of the piece of jj log that will appear after the split. In fact, I've been wishing for a UI to split one commit into many in one operation that would look similar. Facebook people (and likely others) have been experimenting with UIs that show many versions of a file arranged horizontally like that as well, I think.

OTOH, the rebase-like split -A/-B UI might make more sense when you select diffs in scm-diff-editor-like UI.

I'm not saying that the -A/-B UI is a bad idea, nor am I saying that the --evict UI is urgent to have (though I think it'd be very helpful), just that this seems to me to be a reason to consider having a --evict UI even if we also do -A/-B.

Some details about --evict UI and a reply to @scott2000

Previously, it wasn't fully clear to me how --evict UI should behave. Quoting Emily's idem 8 again,

One proposal was to make (5) the default behaviour (so jj split would behave like the stable release except the change ID would move to the second commit for consistency), and add a flag like --evict/--away/--yeet/… that implements (4). ... a jumble of confusing and conflicting intuitions about how @ should move and how descendants should be rebased.

I think I'm now convinced by @scott2000's argument on Discord that for the --evict behavior, the changes should be rebased onto the same commit where the bookmarks and the change id go. If we want to be consistent, let's be fully consistent. I previously thought this was less convenient than rebasing the children onto the child commit, but Scott had a good argument about how it makes the new workflow with the edit workflow. Since Scott's suggestion is more convenient sometimes, and easier to reason about and remember all the time, I'd try that.

We could always create additional options as needed, but I currently think that this is the version of --evict that makes sense.

(I realize this section is very abbreviated, feel free to ask me to expand on it)

ilyagr avatar Mar 11 '25 04:03 ilyagr

A couple of clarifications for the "Another UI to keep in mind" section, based on a quick discussion with Emily:

What worries me about having only jj split -A @ instead of --evict is that it is not obvious to me what the three panes of a diff editor will mean for -A @ worries me. If we find a way to make it obvious, that might be better than --evict, but I'm not sure how.

This is also related to the fact that the rebase in jj split -A @ could introduce conflicts in theory (unless we hack around it, which we probably would, but that's not my point). There is no possibility of conflicts if the middle pane becomes the snapshot of the file in the parent commit (whether or not it inherits the change id).

This is very much a patches-vs-snapshots mindset. When thinking about patches, the UI of :builtin diff editor makes sense, as do -A/-B. However, these make little sense when thinking of snapshots, which is also useful for people to be able to do. In that case, the 3-pane-diff UI is wonderful, as is a version of split that's optimized for it (as it currently is). jj should be helpful both when people think in terms of patches and when a problem is more easily reasoned about in terms of snapshots.


In actuality, I think my suggestion for jj split --evict is something like jj split -A @ --invert-selection in the language of #5828. However, the problem is that this is not obvious[^still]; -A/-B might be the flexible solution, but it can be hard to reason through, especially for snapshots. We can make it obvious in some way other than what I suggested, but implementing something like --evict seems easiest to me.

[^still]: I'm still not sure whether --invert-selection is needed there or for -B to work like jj split without --evict. Hopefully, after I do my TODO homework, it'll be clear.


TODO: Walk through an example of what -A would look like in 3-pane view. (And with different behaviors)

I think an example should bring some substance to my possibly overly theoretical comments, but I don't think I'll do it today.

ilyagr avatar Mar 11 '25 04:03 ilyagr

One solution to this that I've been thinking about is having the flag change the default destination. Basically there would be two modes:

  1. The default mode is "move selected changes". In this mode, the patch selected by the user using the diff editor is moved out of the target commit into a new commit. If no destination is given, the "natural" destination of -B target is used by default. This is the most natural destination because it means that the snapshot selected in the diff editor will be reflected in a commit tree (the parent commit, which is the newly-created commit), and it is not possible for there to be conflicts.
  2. An alternative mode is "keep selected changes", which can be specified by a flag (e.g. --evict, --keep-selected, --invert-selection). In this mode, the patch selected by the user using the diff editor is kept in the target commit, and any other remaining changes are moved into a new commit. If no destination is given, the "natural" destination of -A target is used by default. This is the most natural destination because it means that the snapshot selected in the diff editor will be reflected in a commit tree (the parent commit, which is the target commit), and it is not possible for there to be conflicts.

I think it gives a consistent behavior, since it will not be possible to cause conflicts unless a destination is passed, and it does the same thing by default in both cases (it puts the selected changes in the parent commit, and the unselected changes in the child commit). The only thing that really changes is which commit inherits the identity of the target commit.

This could also apply to jj squash, where the analogous behavior would be to change the meaning of -r X to be equivalent to --from X --into X+ when --evict is passed (instead of the usual --from X --into X-), making jj squash -r X --evict have the same behavior as the old jj unsquash -r X I believe (edit: or maybe it was jj unsquash -r X+?).

Since jj squash is basically the inverse of jj split, I think it's important that whatever solution we find for jj split also works analogously for jj squash.

The hard part of this would be to pick a name that makes sense whether the user passes a destination or not. We could also say that when --evict is passed, the user isn't allowed to pass destination arguments, which would probably make it easier to explain the behavior (I don't think jj unsquash allowed --from and --to arguments either since it would be unintuitive?). I think it could be useful to allow both --evict and destination flags at the same time though.

scott2000 avatar Mar 11 '25 23:03 scott2000