Feedback for the experimental advance-branches feature

[emesterhazy]

Please leave any feedback that you have for the experimental advance-branches config setting added in

• Implement FR #2338 for current branches / automatically advancing branches #3129

This feature should be available in the next (0.17) release or today if you build from source at head.

[emesterhazy]

There were a few open questions from a discussion on the PR.

1. Should we do nothing if there are multiple branches eligible to advance?
2. Should we only advance a branch if there isn't already a branch pointing to the target commit?
3. Should we add a special case for new commits with multiple parents (aka merges) at moves a branch to the merge commit?

Feel free to share your thoughts on these as well.

[Cretezy]

Are docs going to be ready for the 0.17 release? I could take up writing or reviewing them.

[emesterhazy]

I wasn't planning to include this in the release notes since it's an experimental feature, but perhaps it's not a bad idea. There might be some additional polish we want to do first though, such as the split issue raised below.

[BatmanAoD]

One other issue I've noticed: split should probably move the branch forward, so that it's still at @- after the operation is done.

[bnjmnt4n]

Related issue: #3419.

[tp-woven]

Thanks for implementing this @emesterhazy!

Just wanted to chime in on the @ vs @- discussion:

Whether or not to advance to @ or @- doesn't actually make much difference in the code. I can add another config option to control that behavior.

I am very much in favor of advancing to @. This is because my workflow usually looks like this:

1. jj co main -m"working on whatever" (or sometimes without the message, and I run jj describe later)
2. Change some code
3. jj git push --change=@
4. Iterate 2+3 until ready for review
5. Get PR comments
6. jj new -m"address pr comments"
7. jj branch set <branch name> <- ideally, this shouldn't be needed
8. Change some more code
9. jj git push <- pushes @
10. Iterate 8+9 until ready for merge

I guess most folks use the squash workflow and continually push their changes down, in which case @- makes sense, but personally I prefer this (mostly to make it easier to see diffs in IDEs, etc.)

EDIT: Added steps 4 and 10 for clarification

[BatmanAoD]

This feature certainly can't help with forgetting to create the branch, of course.

Why not just adopt a squash-based workflow, rather than advocating for this feature (which is already arguably supporting a niche use case, per those prior discussions) to work differently?

[BatmanAoD]

To put it differently, you are effectively using a squash-like workflow, in that you are repeatedly modifying an existing commit. From the perspective of the final commit history and the sequence of commits seen on the remote, there's no difference between what you're doing and repeatedly squash-ing. Why not just embrace that?

[tp-woven]

This feature certainly can't help with forgetting to create the branch, of course.

No, but the branch is already created by jj git push --change=@. This feature can help with forgetting to update it.

Why not just adopt a squash-based workflow

The main reasons I work on the commit itself rather than squashing are shell and IDE integrations - I prefer to see the status/diffs of the "whole" commit while I work, rather than "what I changed since the last squash".

[BatmanAoD]

Ah. This draft MR would resolve that, I think: #3405

[tp-woven]

Oh, nice! That could be really useful!

[amiryal]

Thanks @emesterhazy, it’s a very welcome ergonomic improvement!

• Should we do nothing if there are multiple branches eligible to advance?

Advance all of them.

• Should we only advance a branch if there isn't already a branch pointing to the target commit?

For jj commit: this sounds like a situation that can only happen in the edit workflow, for which the use case of advance-branches is not clear to me to begin with. If anything, then the branch pointing to the target (old @) should advance to the new @.

For jj new: forgive me, but I do not see the use case for advance-branches at all. Can we add a configuration to limit this feature to jj commit only?

• Should we add a special case for new commits with multiple parents (aka merges) at moves a branch to the merge commit?

Advance from the first parent only, maybe? Tough question; it could be another configuration toggle.

[BatmanAoD]

The open questions were actually the result of a discussion about how to handle merges. In the current implementation, there's no good way to merge two "advanceable" branches and only advance one of them.

The "only advance a branch if there isn't already a branch pointing to the target commit" idea was suggested so that users could explicitly select which branch to advance:

jj new <branch1> <branch2>
jj branch set <branch1> -r @
jj branch commit -m "merge <branch2> into <branch1>"

For jj new: forgive me, but I do not see the use case for advance-branches at all. Can we add a configuration to limit this feature to jj commit only?

jj commit is effectively just jj new, except that -m (if provided) targets the old commit rather than the new commit. I don't know if it's possible or desirable to further differentiate their behavior.

Advance from the first parent only, maybe?

If I understand correctly, there's not really a "first parent" in the JJ commit metadata, but yes, advancing the first listed parent listed in the new command itself is the proposal in question. (Originally suggested here.)

[allancalix]

This is a great quality of life feature thanks for adding this! One thing I want to note as I set this is I wanted to have some sensible global defaults (only advance on branch with some pr glob convention) and disable on main.

I then set jj config set --repo experimental-advance-branches.enabled-branches '["main"]' on a repo where I push directly on top of main but the enabled-branches in the local config and disabled-branches in the global config conflicted. It's not that obvious how these config options get merged together and will behave as a result.

I just ended up setting both options locally but a couple ideas come to mind:

• log a warning if matching branches are in both enabled and disabled sets
• enforce in the config option that both options be set at the same level

Either way I think the flexibility here is awesome, especially with the glob support.

Here's what I ended up with for reference:
Shell function to override per repo
Global options preventing this behavior on main