Skip to content
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

Merged
merged 3 commits into from
Apr 20, 2024

Conversation

emesterhazy
Copy link
Collaborator

@emesterhazy emesterhazy commented Feb 24, 2024

This is an experimental feature that allows users to configure branches which will automatically advance when a new commit is created with jj commit or jj 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:

[experimental-advance-branches]
enabled-branches = ["glob:*"]

Specific branches can also be disabled:

[experimental-advance-branches]
enabled-branches = ["glob:*"]
disabled-branches = ["main", "glob:push-*"]

Branches that match a disabled pattern will not be advanced, even if they also
match an enabled pattern.

How it works

  • When jj commit and jj new are run, they target a specific revision, the "target revision". For jj commit this is always @, and for jj new the user specifies the target.
  • When jj commit or jj 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.
  • Eligible branches will be advanced and will point to @- after jj commit or jj 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:

This feature also includes a convenience for colocated git repos. When the working commit is changed in a colocated repo and there is an advance-branches eligible branch pointing to its first parent, the Git HEAD is set to the branch ref instead of being detached at the parent commit id. This applies any time the working commit is changed, not just when jj commit is run.

I decided to split that feature into a separate PR, which I will mail after this PR is submitted.

@emesterhazy
Copy link
Collaborator Author

@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 jj commit)is typically WIP analogous to an uncommitted working copy in Git.

@martinvonz
Copy link
Member

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 jj commit)is typically WIP analogous to an uncommitted working copy in Git.

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?).

@martinvonz @yuja Would you mind taking a look at this and sharing your thoughts when you get a chance?

I think I'm back to preferring the model where we consider branches pointing to @ as active, even if that means more work when exporting to Git.

@emesterhazy
Copy link
Collaborator Author

I think I'm back to preferring the model where we consider branches pointing to @ as active, even if that means more work when exporting to Git.

I think this means that when jj commit is run, only the branch pointing at @ is eligible to advance, and it will be moved to the new commit created by jj commit is that right? The current PR does this for @- instead of @.

Can you elaborate on why you think it's better to use @ instead of @-? I actually think that using @- as the target makes more sense because the new commit created by jj commit or jj new is usually either left empty (if you're done and pushing your work to Github or some other forge) or is an undescribed work-in-progress commit. I actually don't think I'd want the branch to move to the new @ commit until I'm ready done with it and ready to jj new on top of it.

@0xdeafbeef 0xdeafbeef linked an issue Feb 24, 2024 that may be closed by this pull request
@martinvonz
Copy link
Member

I think this means that when jj commit is run, only the branch pointing at @ is eligible to advance, and it will be moved to the new commit created by jj commit is that right? The current PR does this for @- instead of @.

Correct.

Can you elaborate on why you think it's better to use @ instead of @-? I actually think that using @- as the target makes more sense because the new commit created by jj commit or jj new is usually either left empty (if you're done and pushing your work to Github or some other forge) or is an undescribed work-in-progress commit. I actually don't think I'd want the branch to move to the new @ commit until I'm ready done with it and ready to jj new on top of it.

You might be right. I never use branches so I'm not sure what behavior people want. We generally don't treat @- differently. Maybe that's all that bothers me about this proposal. But again, I'm not the target audience for this.

@yuja
Copy link
Collaborator

yuja commented Feb 25, 2024

Just dropping random comments. I'm also not the target audience as I always start git with checkout --detach.

(In the commit message)

We need to support jj new, and we need to add a --branch argument to jj new

It would be weird if jj new --branch created a branch on the parent of the newly created commit. And it's not possible if new created a merge. I think this is the same kind of problem that jj git push -b would have to push the parent if the current branch were advanced to @.

FWIW, when learning Git, I find it's confusing that git checkout -b is most likely the right command to create new branch, and git branch is NOT the command to start working on the created branch.

#3129 (comment)

We generally don't treat @- differently. Maybe that's all that bothers me about this proposal.

For some local commands like jj log -r..<current-branch>, it would be more useful if the current branch is pointing to @. For git push/export (and native push?), it's not. I have no idea how to model that in jj.

@emesterhazy
Copy link
Collaborator Author

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 @ in jj but push @- to github, or have the branch set to @ in jj and set the git ref for the branch to @-. I think that type of inconsistency will be cumbersome and confuse users.

I think there are probably three user "stories" for this feature:

  1. I don't care about automatically advancing branches and will leave this feature disabled.
  2. I am using jj in a non-colocated repo and am pushing multi-commit branches to a Git forge. When I am working on a branch, I want it to point to my last finished commit. When I am done working, I will have @ set to an empty commit as the result of jj commit or jj new. I don't want to push this empty commit to my forge.
  3. I am using jj in a git colocated repo and am pushing multi-commit branches to a Git forge. When I am working on a branch, I want it to point to my last finished commit. If I use the git CLI, I want HEAD to be set to the branch instead of being detached so that git behaves normally. When I am done working, I will have @ set to an empty commit as the result of jj commit or jj new. I don't want to push this empty commit to my forge.

If you look at the original FR #2338 (comment) I think it's asking for an easy way to set the "working" branch to @- automatically. I think that user probably falls into category 2.

Do you think there's a user story that's better served by advancing the branch to @ instead of @-? What do you think it looks like?

@martinvonz
Copy link
Member

If there are unfinished commits on top of branches, I probably would want to see those changes when I run jj log main..my-branch, for example. That would require the branch to point to the latest commit, whether or not that commit is finished.

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 jj git commit command? User who use that workflow a lot can create an alias for that command. That seems like a simple solution that doesn't involve trying to figure out a jj-native workflow for advancing branches.

@BatmanAoD
Copy link
Collaborator

BatmanAoD commented Feb 26, 2024

So do you think we should simply put this in a jj git commit command? User who use that workflow a lot can create an alias for that command. That seems like a simple solution that doesn't involve trying to figure out a jj-native workflow for advancing branches.

I'm not sure how much sense that makes logically, but as one data point, I do pretty often mistype jj commit as jj git commit.

But isn't this really about how branches are used, rather than how git is used? How about jj branch commit?

@BatmanAoD
Copy link
Collaborator

I already said this in the issue thread, but I don't think this feature would make much sense if it used @ instead of @-. I'll quote what I wrote there:

  • Possibly the biggest benefit of jj is being able to have a record of every workflow state the tool has seen stored as a proper commit, like an auto-updating stash, without "committing" changes to a branch until you're actually ready to do so.
  • @ is usually empty once you're ready to push to a remote (hence new being preferred to edit, and commit being equivalent to describe+new rather than just describe). But it doesn't make sense to commit empty commits to a remote, and commits without descriptions can't be pushed.

@emesterhazy
Copy link
Collaborator Author

I think the question about whether the branch should move to @ or @- is fundamentally about what a branch represents and which changes should be included. Right now it's completely up to the user because they need to manually move the branch. This is where my opinions come in, but I think that one of the most common uses of branches is to point to a line of "completed" work, and the more I think about that use case the more I'm convinced that advancing the branch to @- is the only way to make this feature ergonomic.

If we move the branch to @:

  • In your jj log main..my_branch example you would see the empty or unfinished commit.
  • If you run jj new my_branch it will create a second empty commit on top of the one my_branch points to.
  • If you don't make any changes to the empty commit the branch points to and run jj new on a different revision, the empty commit will not be abandoned. Without a branch set to the empty commit, it would be abandoned.
  • If you want to build on top of the last "completed" commit in my_branch you'll need to use jj edit my_branch instead of jj new
  • If you are pushing a branch to a forge (git or otherwise), you'll need to manually move it behind the empty commit.

If we move the branch to @-:

  • You can run jj log (main..my_branch):: to see any unfinished commits on top of your branch.
  • You can run jj new my_branch to get a new empty commit on top of the branch. If you don't make any changes and run jj new on a different revision, the empty commit will be abandoned (this is the current default behavior).
  • If you want to build on top of the last "completed" commit in my_branch, you can just run jj new my_branch. You don't need to switch to the edit command to avoid creating an extra empty commit.
  • If you are pushing a branch to a forge, you don't need to move the branch before pushing to avoid including an empty commit.

jj doesn't distinguish between committed and uncommitted changes, but that doesn't mean that users don't treat the "working commit" at @ differently than @-. Even jj itself treats the empty @ commits created by new and commit differently since it will abandon them if you make no changes and run jj new on a different revision.

I think "branches point to finished work" for some definition of "finished work" that doesn't include empty commits created by jj new and commit is probably the most common use of branches. As long as we support a common and reasonable workflow I think we're in good shape. Users can always opt out of automatically advancing branches and have full control over the branch pointers.

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 jj new and jj commit.

@yuja
Copy link
Collaborator

yuja commented Feb 27, 2024

I think that one of the most common uses of branches is to point to a line of "completed" work, and the more I think about that use case the more I'm convinced that advancing the branch to @- is the only way to make this feature ergonomic.

Regarding "completed" work, I think @- is often unfinished (wip) commit under topic branch worflow, and for me, @- is not so different from @ in that they are all draft changes. I would expect jj log -r [email protected]_branch shows the stack of draft changes including @ if my_branch auto-advances.

If we move the branch to @:
[...]

  • If you don't make any changes to the empty commit the branch points to and run jj new on a different revision, the empty commit will not be abandoned. Without a branch set to the empty commit, it would be abandoned.

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 @, it might be better to revert the change (or at least make it configurable.)

  • If you are pushing a branch to a forge (git or otherwise), you'll need to manually move it behind the empty commit.

Yes, this is the problem. It's unclear whether the @ commit should be pushed if it's not empty.

@martinvonz
Copy link
Member

If we decide to set the active branch to @, it might be better to revert the change (or at least make it configurable.)

+1

  • If you want to build on top of the last "completed" commit in my_branch, you can just run jj new my_branch. You don't need to switch to the edit command to avoid creating an extra empty commit.

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 my_branch.

  • If you are pushing a branch to a forge (git or otherwise), you'll need to manually move it behind the empty commit.

Yes, this is the problem. It's unclear whether the @ commit should be pushed if it's not empty.

I think jj git push should not push it. That would better match Git's behavior.

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 @ commit too, but it's too early to say yet if users will like that behavior.

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.

@emesterhazy
Copy link
Collaborator Author

emesterhazy commented Feb 27, 2024

I'm starting to think there's no perfect solution here since there are many possible workflows. I still think that @- is the "lesser of two evils", but that's as much as it is since you can conceive of a useable workflow where branches are advanced to @ as well.

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 jj from this branch, here's what I'd suggest:

  1. I will finish the code for jj new and add more tests.
  2. Can we name the config option experimental-advance-branches to make it clear that this feature may change or be removed?
  3. 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.
  4. If we add the @ config option, I can look into disabling the behavior from #1854. I don't think we should add a separate off switch, so if we think it should be disabled if branches are advanced to @, then we should just do it.

What do you think?

@martinvonz
Copy link
Member

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.

@yuja
Copy link
Collaborator

yuja commented Feb 27, 2024

  1. I will finish the code for jj new and add more tests.
  2. Can we name the config option experimental-advance-branches to make it clear that this feature may change or be removed?
  3. 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.

If we choose the @ model, we won't need the advance-branches config. Having a branch on @ means the branch is current/active, and it will advance. In other words, HEAD = <branch> (not detached) in git is encoded as @ = <branch>, @- = <branch>@git in jj. Anyway, I agree that trying 3 isn't needed. IMHO, it would be quite different from the @- design.

I'm skeptical about jj new --branch because it wouldn't make sense if experimental-advance-branches is off.

@BatmanAoD
Copy link
Collaborator

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!

@BatmanAoD
Copy link
Collaborator

This is excellent. It's exactly what I want.

@emesterhazy
Copy link
Collaborator Author

Thanks for the feedback. I'll add support for jj new as well. Just to be clear, this means that if jj new is run on revision xyz and one of the parents of xyz has an advanceable branch pointing to it, then the branch will be advanced to xyz. It is analogous to how jj commit works, except that you can specify a revision.

I'm skeptical about jj new --branch because it wouldn't make sense if experimental-advance-branches is off.

I'm not planning to add any flags to jj new or other commands. I think in an earlier version I proposed adding a flag to disambiguate in scenarios where multiple branches might advance, but I think we should just make the user manually move the branch with jj branch set in those cases. So now my plan is to have this controlled exclusively by the config.

@emesterhazy emesterhazy force-pushed the advanceable-branches branch 3 times, most recently from bbff561 to 5779cb1 Compare March 14, 2024 21:15
@emesterhazy emesterhazy force-pushed the advanceable-branches branch 4 times, most recently from 8e19e70 to 57911bb Compare March 20, 2024 13:52
@emesterhazy emesterhazy marked this pull request as ready for review March 20, 2024 13:52
@emesterhazy
Copy link
Collaborator Author

@martinvonz

Hi everyone, sorry for the delay. This is ready for review now. A couple of notes:

  1. The config setting is called experimental-advance-branches. If we decide to stabilize this we can remove the experimental prefix.
  2. I added support for jj new. It will only advance branches if a single revision is targeted and neither --after or --before are used. I think this is a reasonable limitation to keep the behavior simple. In the common case where commits are usually appended, this limitation doesn't matter.
  3. This PR does not include the change to not detach the Git head for colocated repos. I will mail a separate PR after this one is submitted. That PR will also include updates to the documentation and changelog.
  4. As I mentioned in a comment earlier, I changed the semantics so that multiple branches can advance at the same time. This ends up being simpler for the user since they don't need to worry about resolving ambiguity.
  5. I updated the PR description based on the new updated semantics.

Copy link
Collaborator

@yuja yuja left a 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.

cli/tests/test_advance_branches.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/commands/commit.rs Outdated Show resolved Hide resolved
cli/tests/test_advance_branches.rs Show resolved Hide resolved
@emesterhazy
Copy link
Collaborator Author

Thanks Yuya for all of the helpful feedback. I'll go through your comments and address one by one.

@emesterhazy
Copy link
Collaborator Author

I've made changes and resolved most of the conversations based on your comments. Please take another look. Thanks!

cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/cli_util.rs Outdated Show resolved Hide resolved
cli/src/commands/commit.rs Outdated Show resolved Hide resolved
lib/src/settings.rs Outdated Show resolved Hide resolved
cli/src/commands/new.rs Outdated Show resolved Hide resolved
@emesterhazy emesterhazy force-pushed the advanceable-branches branch from 45a69b1 to e1498fc Compare March 30, 2024 16:25
Copy link
Collaborator

@yuja yuja left a 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!

cli/src/cli_util.rs Show resolved Hide resolved
cli/tests/test_advance_branches.rs Outdated Show resolved Hide resolved
cli/src/commands/new.rs Outdated Show resolved Hide resolved
## 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.
@emesterhazy emesterhazy force-pushed the advanceable-branches branch from bf681cf to 5e10eef Compare April 20, 2024 13:05
@emesterhazy emesterhazy force-pushed the advanceable-branches branch from 5e10eef to e67d6f7 Compare April 20, 2024 13:29
@emesterhazy emesterhazy merged commit 0fb582e into main Apr 20, 2024
16 checks passed
@emesterhazy emesterhazy deleted the advanceable-branches branch April 20, 2024 14:26
@yuja yuja mentioned this pull request May 31, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FR: --advance-branches flag for jj commit
4 participants