-
Notifications
You must be signed in to change notification settings - Fork 343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement FR #2338 for current branches / automatically advancing branches #3129
Conversation
@martinvonz @yuja Would you mind taking a look at this and sharing your thoughts when you get a chance? We've been discussing variations of what I think is the same idea in #2338. I think this implementation will work well for Git users, and still makes sense for jj users where the working commit (i.e. what's created by |
Thanks for mentioning that. I think it's important that we design something that works well in repos that are not colocated with Git. Once we have come up with something we like, we should try to make our code for colocated repos adapt to that model. So I think we should try to ignore how Git behaves when we design this feature, unless we say that this is a feature designed only for Git interop (so it would apply only when using the Git backend?).
I think I'm back to preferring the model where we consider branches pointing to |
I think this means that when Can you elaborate on why you think it's better to use |
Correct.
You might be right. I never use branches so I'm not sure what behavior people want. We generally don't treat |
Just dropping random comments. I'm also not the target audience as I always start git with (In the commit message)
It would be weird if FWIW, when learning Git, I find it's confusing that
For some local commands like |
I think whatever we do, if anything, the model needs to be simple and intuitive. I don't think that we should, for example, have the branch set to I think there are probably three user "stories" for this feature:
If you look at the original FR #2338 (comment) I think it's asking for an easy way to set the "working" branch to Do you think there's a user story that's better served by advancing the branch to |
If there are unfinished commits on top of branches, I probably would want to see those changes when I run It sounds like this feature is only for Git interop. Your two no-op user stories were both about Git interop. So do you think we should simply put this in a |
I'm not sure how much sense that makes logically, but as one data point, I do pretty often mistype But isn't this really about how branches are used, rather than how git is used? How about |
I already said this in the issue thread, but I don't think this feature would make much sense if it used
|
I think the question about whether the branch should move to If we move the branch to
If we move the branch to
jj doesn't distinguish between committed and uncommitted changes, but that doesn't mean that users don't treat the "working commit" at I think "branches point to finished work" for some definition of "finished work" that doesn't include empty commits created by Let's try to sort out the behavior and supported use cases first and then decide if this should be controlled by a config setting or flag on |
Regarding "completed" work, I think
I made this change to support a workflow where named branch is set at the root of the stack #1854, not at the head (which would require auto-advancing.) If we decide to set the active branch to
Yes, this is the problem. It's unclear whether the |
+1
OTOH, you will then start a new commit when you might have wanted to continue working on the unfinished commit you already had on top of
I think I don't know what I would expect non-Git forges to do. FWIW, when exporting to our internal forge at Google, we do export the I'm still not sure which is the right choice. I don't want to block progress on this. As you long as you don't change the default behavior too much, we can just get something in so people can experiment with it. |
I'm starting to think there's no perfect solution here since there are many possible workflows. I still think that So, I would just like to acknowledge that there are lots of different opinions here, and we may not be able to come up with a solution that's simple and useful for everyone. @martinvonz @yuja if you're interested in submitting something experimental so that users can try it without needing to build
What do you think? |
SGTM. I think we can ignore items 3 and 4 until people have experimented with the feature for a while. I don't want you to waste time implementing item 3 if it seems like everyone is happy with what you already have in this PR. Thanks for all the time you've spent thinking and coding on this. |
If we choose the I'm skeptical about |
Tried this out very briefly just now, and it looks like it does exactly what I was expecting. I'll be using it this week and see if I have further feedback. Thanks very much for implementing! |
This is excellent. It's exactly what I want. |
Thanks for the feedback. I'll add support for
I'm not planning to add any flags to |
bbff561
to
5779cb1
Compare
8e19e70
to
57911bb
Compare
Hi everyone, sorry for the delay. This is ready for review now. A couple of notes:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me, thanks.
Thanks Yuya for all of the helpful feedback. I'll go through your comments and address one by one. |
d843258
to
45a69b1
Compare
I've made changes and resolved most of the conversations based on your comments. Please take another look. Thanks! |
45a69b1
to
e1498fc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you for working this feature!
8a218d2
to
f5c4305
Compare
f5c4305
to
bf681cf
Compare
## Feature Description If enabled in the user or repository settings, the local branches pointing to the parents of the revision targeted by `jj commit` will be advanced to the newly created commit. Support for `jj new` will be added in a future change. This behavior can be enabled by default for all branches by setting the following in the config.toml: ``` [experimental-advance-branches] enabled-branches = ["glob:*"] ``` Specific branches can also be disabled: ``` [experimental-advance-branches] enabled-branches = ["glob:*"] disabled-branches = ["main"] ``` Branches that match a disabled pattern will not be advanced, even if they also match an enabled pattern. This implements feature request #2338.
In a future commit these tests will run with both `jj commit` and `jj new` since both will have the same semantics for advancing the branch pointer. Parameterizing the tests allows us to run both variants without duplicating the test bodies. Since the commit IDs are different when `jj describe` + `jj new` is used instead of `jj commit`, this commit also drops the commit IDs from the test snapshots. This is fine because the commit IDs are not important for these tests.
bf681cf
to
5e10eef
Compare
5e10eef
to
e67d6f7
Compare
This is an experimental feature that allows users to configure branches which will automatically advance when a new commit is created with
jj commit
orjj new
. This feature may change or be removed in the future.Setup
This behavior can be enabled by default for all branches by setting
the following in the
config.toml
:Specific branches can also be disabled:
Branches that match a disabled pattern will not be advanced, even if they also
match an enabled pattern.
How it works
jj commit
andjj new
are run, they target a specific revision, the "target revision". Forjj commit
this is always@
, and forjj new
the user specifies the target.jj commit
orjj new
is run, jj will look at the parents of the target revision and advance any eligible branches pointing to them to the target revision (not the new commit). Branches are eligible to advance if the feature is enabled globally and the branch is not in the overrides list, or vice versa.@-
afterjj commit
orjj new
completes.jj new
only advances branches if the target is a single revision and the--after
and--before
are not used.The result is that the branches move forward automatically as new commits are added. This is useful if you are committing linearly and don't want to move the branch pointer manually. Users familiar with Git may find that this feature mimics Git's behavior where branches move automatically as commits are added.
Misc
An earlier version of this PR also included a feature for colocated Git repos:
I decided to split that feature into a separate PR, which I will mail after this PR is submitted.