search

martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
8.39k stars 289 forks source link

FR: `--advance-branches` flag for `jj commit` #2338

Open emilykfox opened 1 year ago

emilykfox commented 1 year ago

Is your feature request related to a problem? Please describe. I often work directly on main for personal projects, but to successfully push my changes to GitHub, I have to run something like jj commit -m [message] && jj branch set main -r @- or jj branch set main && jj commit -m [message]. The former is error prone as it's easy to forget the -r flag. The latter feels slightly unnatural as there's a brief moment where the main branch is pointing to something without a description. They're both pretty verbose.

Describe the solution you'd like In Discord, @martinvonz suggested adding a --advance-branches flag to jj commit which could move any branches pointing to the old @- to point to the new @-.

Describe alternatives you've considered None

Additional context See the brief conversation starting at https://discord.com/channels/968932220549103686/969829516539228222/1159510245849186335.

Probably more important is that this feature would also improve workflows that involve adding several commits in sequence to a single feature branch.

ilyagr commented 1 year ago

I'd like this too.

Some miscellaneous thoughts:

  1. I think jj new could also have such a flag, for moving branches from whichever commit jj new makes the parent of the new commit (usually @).
  2. This is minor, but I think jj commit --advance-branches should also move branches from old @ to new @ (if any).
  3. (Not for the first version) I wish there was some way to specify the branches that define immutable commits (e.g. main), so that jj commit --advance-branches would know not to move these branches. This would make defining the immutable commits revset harder, though, since it's actually defined by main@origin and main@upstream.
necauqua commented 1 year ago

I think I've advocated in the past that jj commit should just do that - makes sense as commit is "an alias to be familiar for git users, as well as just a shortcut for new+describe"

Maybe there should be an option for jj new, and then we describe commit as an alias for new with such flag+describe - again, makes total sense as it is just for more git-like workflow

martinvonz commented 1 year ago

EDIT: The below was in response to @necauqua's comment above.

I'm concerned that not all users who do jj new main; <edit files>; jj commit want to move main forward. Some prefer to work with anonymous branches (aka detached heads) but still like to use jj commit as a describe+new shortcut.

necauqua commented 1 year ago

Well, I now realised that if we just have the flag (probably on both commands), then I can alias it anyway

Especially since in personal repos I do anonymous heads and at work I have to maintain feature branches (and I do use the commit shortcut as much as it "un-coreness" bothers me, it's still a great shortcut) :)

Such an alias (don't remember off the top of my head if you can alias used words like "ci/commit") could then be somewhere at the "useful aliases" page

ilyagr commented 1 year ago

3. (Not for the first version) I wish there was some way to specify the branches that define immutable commits (e.g. main), so that jj commit --advance-branches would know not to move these branches. This would make defining the immutable commits revset harder, though, since it's actually defined by main@origin and main@upstream.

A simpler version of this idea: we could have a config that lists some names of branches that are not advanced by --advance-branches, defaulting to ["main", "master"] (and perhaps some more, I'm not sure).

We could then think about whether we can define immutable-heads using such a list of branches but, again, this could be a bit tricky or controversial.

BatmanAoD commented 11 months ago

I'm a bit confused by the above conversation about main and master; does it predate the existence of the trunk() alias? Doesn't the trunk() alias pretty much resolve the question of how to exclude main and master from automatic advancement?

BatmanAoD commented 11 months ago

Separately: I think in general, if I have to manually --advance-branches, I'd want the ability to go back more than one commit. I.e. if I use jj new or jj commit several times and then realize I left my tracking branch behind, I want an easy way to do the equivalent of jj branch set <name> -r @- (or -r @, depending on whether the working copy is empty).

...speaking of which, the fact that the "obvious" commit to choose is either -r @ or -r @- conditionally on the state of the working copy is, I think, a really good reason to make this functionality more automatic.

ilyagr commented 11 months ago

Doesn't the trunk() alias pretty much resolve the question of how to exclude main and master from automatic advancement?

trunk() would indeed often work, but it is currently supposed to refer to exactly one branch. I'm worried about the situation where you'd want to preserve multiple branches. For example, this would occur if immutable_heads() contained more than one branch (e.g. main and develop). You wouldn't want jj new --advance-branches to change which revisions are considered immutable.

ilyagr commented 8 months ago

Probably in addition to --advance-branches, here's an idea that could maybe help with the fancier use-cases. It feels a bit incomplete/underbaked at the moment, but might be worth considering and evolving.

We could support a syntax along the lines of jj branch s topo:ancestor, which would find the closest ancestor commit with a branch and move that branch to the current commit.

If more than one branch fits the description, it's unclear to me what's best (exit with an error or move them all).

ancestor could also be a function, so jj branch s topo:ancestor -r qq is an alias for jj branch s 'topo:ancestor(::qq)' -r qq.

yuja commented 8 months ago

What if we could support a syntax along the lines of jj branch s topo:ancestor, which would find the closest ancestor commit with a branch and move that branch to the current commit.

It could be modeled as a "branch set" expression, but I would add a separate command jj branch advance [--from REV] --to REV (or jj branch move --advance --to REV) for that purpose.

BatmanAoD commented 8 months ago

I have been thinking about this, and if we can agree on the design, would like to help implement it.

I have a few observations and questions, plus a possible design.

I'll start by explicitly stating the intent of this issue as I understand it:

Introduce a more convenient way to advance to advance branches to newer commits than branch set

Here are some characteristics I'd like the solution to have:

Questions:

Anyway, given all of that, my proposal is that this new behavior should be added as a flag to describe rather than new, because the purpose is to attach semantic information to a particular existing workspace-snapshot. This lines up well with the fact that jj git push requires both a description and a branch for each new commit being shared with the remote; so it makes sense that jj describe --advance-branches would enable providing both of these in a single command.

Proposal in more detail:

BatmanAoD commented 8 months ago

@ilyagar I'm not sure what you mean by this:

We could support a syntax along the lines of jj branch s topo:ancestor, which would find the closest ancestor commit with a branch and move that branch to the current commit.

I thought this was basically what --advance-branches was asking for? I'm also not sure what topo means in this example.

joyously commented 8 months ago

Does this issue arise from the difference in how jj and Git define branches? Is it only needed when pushing to Git or is it ingrained in the mental model? Breezy commands are centered around branches, and the default is for a branch to be a separate folder, but it can also be configured to colocate branches like Git does. And it interoperates with Git either way, I suppose because they both use the same concept of the branch pointer being the tip.

The model in jj seems like a tag instead of a branch. That would work also, if the interface to the different system takes that into account. I don't think the --advance-branch flag is the right way. I think it should be a revset for push.

BatmanAoD commented 8 months ago

@joyously In git and jj, branches are indeed essentially tags. But they can still be useful locally without pushing them, so this should not be part of push.

joyously commented 8 months ago

@joyously In git and jj, branches are indeed essentially tags. But they can still be useful locally without pushing them, so this should not be part of push.

Perhaps it shouldn't be called a branch if it isn't one? Or define what a branch is in jj as the existing implementation, and use a different concept to equate to a branch in Git.

BatmanAoD commented 8 months ago

@joyously This is a bit of a tangent, I think, since the branch model is fairly well established and unlikely to change as part of this particular feature request. I'm not sure why or how "jj branches" and "git branches" could be distinguished, because they're the same concept; and when using git as the backing store for jj (which is currently recommended), I believe jj branches are stored as git branches.

This is a good, brief explanation of how to think about branches in git, which applies to jj as well: https://wizardzines.com/comics/meet-the-branch/

joyously commented 8 months ago

I'm not sure why or how "jj branches" and "git branches" could be distinguished, because they're the same concept

Well, obviously they don't quite match or this new flag would not be needed. What I've heard is that jj is its own thing and just happens to use Git as a backend right now, but could also use other VCS libraries instead. As Darcs and Breezy use a slightly different concept for branches, I don't think it's right to clutter the CLI with flags for making it work like Git.

If the jj "branch" is really a "tag", maybe call it a tag and make a "real" branch that moves with latest commit.

BatmanAoD commented 8 months ago

@joyously I'm sorry, but you're mistaken: this request is not due jj and git having different branch models, but because jj always operates in what git calls the "detached HEAD" state. It's pretty rare for git users to operate in this mode, but some people use it a lot. This post has some details about using Git in headless mode.

tags are also an existing concept, and while they are very similar to branches, they're not exactly identical, so branches cannot simply be renamed "tags".

PhilipMetzger commented 8 months ago

Sorry to barge into your conversation, we recently talked about renaming branches to bookmarks like Mercurial, which definitely fits better to the current jj branch implementation.

We then should implement also something like topics, which are the canonical set of "named commits" which everybody knows. (See https://github.com/martinvonz/jj/discussions/2425#discussioncomment-7376935)

BatmanAoD commented 8 months ago

That sounds good. But would that make --advance-branches unnecessary?

For that, I think we'd need two conditions:

PhilipMetzger commented 8 months ago

And since I'm here anyway.

Here are some characteristics I'd like the solution to have:

  • Rather than a new command, this should be a flag on new, commit, and/or describe (for new and commit, the operation would modify the "old" revision, i.e. the one to which the commit message is attached)
  • the user shouldn't need to manually specify branch names unless there's unavoidable ambiguity

This is pretty accurately describing the feature request.

  • Possibly this should be the default behavior for commit (since it's a combination of describe and new that is intended to imitate Git behavior)

Not really, only because it is a convience built-in, does not mean it is a git commit equivalent. It could be gone in a few years. A configurable option, maybe?

  • can commit & sync with the remote in one command (hopefully this could be a user-defined alias; see "questions")

This functionality does not belong here, see #1039. And even then whats the reason for it?

  • the user should be prompted to provide descriptions for any commits without commit-messages

This is also does not belong here, see #2977.

The operation should not implicitly/automatically advance certain special branches:

  • any protected branch (in the GitHub sense; see "questions")

  • the default (i.e. remote HEAD) branch (see "questions")

  • trunk()

  • Does jj have, or could it have, any native concept of "protected" branches? ("protected" in the GitHub/GitLab sense, not immutable_heads)

Answering both, no there are no special branches for jj as it isn't required and won't be required. This is purely related to Git and its implementation of branches.

  • Similarly, does it, or could it, be aware of "default" branches? (This at least corresponds to a native git concept, the HEAD of each remote)

We fetch the default Git branch since, 0.14. But this is once again a pure Git concept which doesn't make sense in a Jujutsu world.

  • Can a user-defined alias define a combination of commands, the way commit combines describe and new?

    • Can the arguments to the original commands still be used?

No, jj commit is as before a special built-in.

Anyway, given all of that, my proposal is that this new behavior should be added as a flag to describe rather than new, because the purpose is to attach semantic information to a particular existing workspace-snapshot. This lines up well with the fact that jj git push requires both a description and a branch for each new commit being shared with the remote; so it makes sense that jj describe --advance-branches would enable providing both of these in a single command.

Since this is mostly a crutch for Git users, implementing it on describe and commit makes sense.

  • New flag on describe: --advance-branches
  • "forward" this flag to commit, or, better yet, make it the default behavior for commit

👍

Default behavior of branch selection for advancing:

  • advance the "nearest" branch head, searching breadth-first through ancestor commits

This is something @ilyagr implied with topo:<branches>, where topo is a revset for the topological nearest branch/commit.

Exclusions:

  • when a remote is configured, protected & default branches are excluded
  • trunk() is always excluded
  • all ancestors of excluded branches (i.e. when encountering a commit that's the head of an excluded branch, prune the rest of that ancestry path)

The trunk() part may only make sense for your projects, but not for everybody else. So make it configurable? I get your idea but we're not Git, so I'd keep exclusions and other ideas far away from this.

  • If a single candidate commit is the head of multiple branches, the command fails

This does not make sense with jj's current branches, I'd just drop it or make it a warning.

  • If multiple candidate branch-head commits are at the same depth, they are all advanced

👍

Configuration options:

  • additional excluded branches may be configured
  • users may opt-out of any of these exclusions (e.g. opt-in to advancing main)

These kinda make sense, but should not be in the first cut.

users may choose from these non-default behaviors for multiple branches on a single commit:

  • interactively select which branch(es) to advance
  • always advance all branches

I'd provide the interactive option only for ambigous branches, the other option makes sense.

If it's not possible for a jj alias to combine multiple commands, then I would like for there to be a built-in "combination alias" (like commit) that would enable synchronizing work in one step (a la svn commit):

  • advance branches
  • add commit messages
  • push

This is a straight "No" from me. Maybe it has a place in your downstream if you find the time.

PhilipMetzger commented 8 months ago

Sorry, the above took a bit longer than expected.

that sounds good. But would that make --advance-branches unnecessary?

Yes, it would make --advance-branches unnecessary, but until something like it exists, branch-oriented workflows need crutches, which we'll need to provide.

For that, I think we'd need two conditions:

  • either new or describe (or both) automatically applies the current "topic" to @ (the "old" commit in the case of new)
  • topics can be pushed as branches to git remotes

Both provided options make more sense than providing a janky Git-like "branch". I'd assume jj new -t <topic> would advance the branch automatically matching your expectation, then it would be entirely unecessary on describe.

BatmanAoD commented 8 months ago

@PhilipMetzger I think we have pretty different workflows and/or philosophies, but I do want to understand each of your points and where you're coming from.

Not really, only because it is a convience built-in, does not mean it is a git commit equivalent. It could be gone in a few years

Sorry, what is "it" here? Are you saying that --advance-branches "could be gone in a few years" or that jj commit itself could be gone?

So make it configurable? I get your idea but we're not Git, so I'd keep exclusions and other ideas far away from this.

These kinda make sense, but should not be in the first cut.

These seem at odds. I was suggestion a default-but-configurable behavior, but the "not in the first cut" comment is in response to a description of how the behavior would be configurable. The need for exclusions already came up in the original few comments in the thread, since main and master, at least, should not be automatically advanced.

This is purely related to Git and its implementation of branches.

I'm assuming that by "this" here you mean "protected branches"? I'm not familiar with many version control systems, but I would be surprised if this is truly unique to Git forges. By "protected branch" I just mean "somewhere that you won't be allowed to push commits to." This is a feature of the remote (i.e. the forge, github/gitlab/etc), not of the version control system. But I acknowledge that, in general, this isn't something that's knowable from the local repository state, so it isn't something the tool should attempt to infer automatically.

This does not make sense with jj's current branches, I'd just drop it or make it a warning.

I'm not sure what part doesn't make sense with jj's model. jj already gives a warning when branch set-ing multiple branches at once explicitly, so doing so automatically feels like it probably shouldn't happen. But I agree that a warning would be sufficient.

But [the default branch] is once again a pure Git concept which doesn't make sense in a Jujutsu world.

"Default branch" in the sense I'm using it here is another property of the forge rather than of the version control system. It simply means "the basis against which pull requests are compared, by default." This seems very non-git-specific to me. Any forge that supports any kind of pull-request-like feature needs some kind of basis for comparison. Even in a classic patch-based mailing-list system (i.e. without a forge), submitting patches still requires some way of indicating or assuming what version of the code the patch was generated against.

...where topo is a revset for the topological nearest branch/commit.

Thanks for clarifying. So the idea is that topo would be a new "keyword" in the revset syntax? (I'm not sure I understand why this would require a new syntax; surely topo(ancestor()) would be okay? I also don't love the name topo but won't start bikeshedding that here.)

Since this is mostly a crutch for Git users, implementing it on describe and commit makes sense.

I'm glad you agree that describe and commit make sense as the base commands for the new flag. I'm not sure why you're describing --advance-branches as a "crutch for Git users," though. Currently, as far as I can tell from the documentation, jj git push is the only way to publish local changes, and that requires either pushing all anonymous branches (which is not generally recommended) or assigning a named branch to the commits. So until the native backend or some other backend supports pushing changes, every user who wants to make multiple separate commits in a sequence (rather than simply squashing every change preemptively) must advance branches before pushing, either explicitly with branch set or implicitly with --advance-branches. Is that not the case? Is there another way to push new commits that I'm missing?

This is a straight "No" from me. Maybe it has a place in your downstream if you find the time.

I sort of assumed this would be the least-well-received part of my proposal, but I'd actually understand what's objectionable about it. (I'd also like to note that I think having a way to alias a sequence of commands would be a very useful feature that would make it completely unnecessary to have commands like commit built into jj itself; but I realize this would be a very complex feature to add and that it wouldn't necessarily be preferable to just writing a short shell script.)

In svn and older version control systems, commit was sufficient to share your local work with others. In git, this became a longer sequence, something like git add ... ; git commit ; git push. With jj, we no longer need add, but we have a different extra step, which is the branch-updating that's required in order to push with git. So we have something like jj [describe|commit]; jj branch set ... ; jj [backend] push.

I understand that there's value in separating out each of these operations both in one's mental model and in the design of the tool. But the most common thing I do in version control is still a combination of all of these in quick succession: jj commit -m "what these changes are for"; jj branch set -r @- <name>; jj git push. What would be wrong with just...doing all of these together, without so much typing? As soon as I've assigned a named branch to my work, I'm pretty sure I want to send that to the remote. (Again, not as the default or only behavior as in svn, but as an option supported by the tool's CLI.)

yuja commented 8 months ago

...where topo is a revset for the topological nearest branch/commit.

So the idea is that topo would be a new "keyword" in the revset syntax?

I think it's a kind of a "branch set expression" to select a branch name (just like a revset expression to select revisions.) For example, topo:ancestor in jj branch set topo:ancestor -r q will be resolved to the branch name of the closest ancestor revisions.

  • Rather than a new command, this should be a flag on new, commit, and/or describe (for new and commit, the operation would modify the "old" revision, i.e. the one to which the commit message is attached)

I think it would be a bit odd if describe had --advance-branch. commit --advance-branch makes sense because it creates new commit, and in Git-like workflow, the current branch will have to be moved to the new commit. OTOH, describe is the command to mutate commit metadata. It could be considered a command to finish up the commit, but the command name describe doesn't suggest that.

btw, I usually go without named branches, and occasionally need to pull up the main branch locally. In that situation, a separate branch advance/move command will be useful.

  • The operation should not implicitly/automatically advance certain special branches:

    • any protected branch (in the GitHub sense; see "questions")
    • the default (i.e. remote HEAD) branch (see "questions")
    • trunk()

Perhaps, this configuration will become a revset? I wouldn't want to move someone else's branch while reviewing code, so I might want to specify ... & mine().

PhilipMetzger commented 8 months ago

I think we have pretty different workflows and/or philosophies, but I do want to understand each of your points and where you're coming from.

I generally don't think we differ in workflows but mostly in philosophies, I just mentally moved on from Git and its branches quite a while ago.

Sorry, what is "it" here? Are you saying that --advance-branches "could be gone in a few years" or that jj commit itself could be gone?

"It" means jj commit here, as I'm vastly in favor of teaching people the jj new + jj describe workflow and I think that jj commit should be deprecated like checkout in a decade or two.

These seem at odds. I was suggestion a default-but-configurable behavior, but the "not in the first cut" comment is in response to a description of how the behavior would be configurable. The need for exclusions already came up in the original few comments in the thread, since main and master, at least, should not be automatically advanced.

What I meant here, is to say that --advance-branches should probably pick the nearest branch in the initial implementation and a follow-up then implements exclusions, when we already have a idea on how jj commit --advance-branches is used. Ideally we just take the immutable_heads() revset initially to mark some branches as un-moveable.

I'm assuming that by "this" here you mean "protected branches"?

Yes.

I'm not familiar with many version control systems, but I would be surprised if this is truly unique to Git forges. By "protected branch" I just mean "somewhere that you won't be allowed to push commits to." This is a feature of the remote (i.e. the forge, github/gitlab/etc), not of the version control system.

Yes, something similar exists in different version control systems but my issue with it is, that you only consider the Git/GitHub/Gitlab workflow in your solution. If you had to implement it for something Perforce-like or Mercurial-like, how would you proceed? The goal is to have a somewhat portable abstraction, not something built purely for Git.

But I acknowledge that, in general, this isn't something that's knowable from the local repository state, so it isn't something the tool should attempt to infer automatically.

It atleast could consider trunk().

So the idea is that topo would be a new "keyword" in the revset syntax? (I'm not sure I understand why this would require a new syntax; surely topo(ancestor()) would be okay? I also don't love the name topo but won't start bikeshedding that here.)

Ilya just made a suggestion which wasn't final at all. But agreed let's not open a bikeshed here.

Since this is mostly a crutch for Git users, implementing it on describe and commit makes sense.

I'm glad you agree that describe and commit make sense as the base commands for the new flag. I'm not sure why you're describing --advance-branches as a "crutch for Git users," though.

I'm describing it as a "crutch for Git users", as other workflows manage fine with anonymous heads. See https://social.jvns.ca/@b0rk/111709458396281239 and this (requires a Discord account).

Currently, as far as I can tell from the documentation, jj git push is the only way to publish local changes, and that requires either pushing all anonymous branches (which is not generally recommended) or assigning a named branch to the commits.

That totally is correct. I do think that you should try a workflow, where you don't immediately set a branch after a new commit as it vastly simplifies the usage of jj.

This is my answer for today, it took a good chunk of my evening, I'll try to explain my thoughts on the last part tomorrow or during the weekend. @BatmanAoD

BatmanAoD commented 8 months ago

@PhilipMetzger Thanks for the detailed reply; but I'm sorry, I'm still extremely confused about whether branches are necessary or not.

Currently, as far as I can tell from the documentation, jj git push is the only way to publish local changes, and that requires either pushing all anonymous branches...or assigning a named branch to the commits.

That totally is correct. I do think that you should try a workflow, where you don't immediately set a branch after a new commit as it vastly simplifies the usage of jj.

If I don't set a branch, then I need to use jj git push -a, right? So are you saying that I should try using jj git push -a for all of my pushing?

martinvonz commented 8 months ago

Sorry it has taken me so long to get to this thread, and I still haven't read all of it.

  • Can a user-defined alias define a combination of commands, the way commit combines describe and new?

No, that's not supported yet. I think we will want support for that eventually.

  • If a single candidate commit is the head of multiple branches, the command fails
  • If multiple candidate branch-head commits are at the same depth, they are all advanced

These seem inconsistent. I'd rather make both of the advance all branches. A configurable revset sounds as @yuja said might be good. Or maybe we even use immutable_heads(). I think that's what @ilyagr also said. So advance all branches pointing to commits in heads(immutable_heads()..@ & branches())? And advance the branches to 'heads(immutable_heads()..@ ~ empty())', perhaps? You can even try that like this:

jj log --no-graph -r 'heads(immutable_heads()..@ & branches())' -T 'local_branches ++ " "' | \
  xargs jj branch set -r 'heads(immutable_heads()..@ ~ empty())'

Consider putting that in a jj-advance-branches.sh or something and see how you like. Sure, you'll have to run it manually, but it's an easy way to see if the behavior seems to be what you want.

If I don't set a branch, then I need to use jj git push -a, right? So are you saying that I should try using jj git push -a for all of my pushing?

I think @PhilipMetzger suggested that you create commits without moving branches and then move the branches just when you're about to push. (I very rarely move branches myself because GitHub takes care of moving the main branch forward in the martinvonz/jj repo, and GitHub also takes care of deleting branches for merged PRs.)

PhilipMetzger commented 8 months ago

Here's the last part:

This is a straight "No" from me. Maybe it has a place in your downstream if you find the time.

I sort of assumed this would be the least-well-received part of my proposal, but I'd actually understand what's objectionable about it. [...]

In svn and older version control systems, commit was sufficient to share your local work with others. In git, this became a longer sequence, something like git add ... ; git commit ; git push. With jj, we no longer need add, but we have a different extra step, which is the branch-updating that's required in order to push with git. So we have something like jj [describe|commit]; jj branch set ... ; jj [backend] push.

I understand that there's value in separating out each of these operations both in one's mental model and in the design of the tool.

Basically my philosophy for new CLIs is that they should be composable and shouldn't special-case behaviors, as it is very confusing for new users which we're all at some point. Thus having a single command which does everything, as in sync, commit and push is in heavy conflict with this philosophy.

Another point to consider with jj at least is; there are different backends and not every backend would opt-in or benefit from such a command.

What would be wrong with just...doing all of these together, without so much typing?

There is absolutely nothing wrong with that, saying that as a big fan of automating everything and everywhere, it just doesn't fit in here. 🙁

As soon as I've assigned a named branch to my work, I'm pretty sure I want to send that to the remote. (Again, not as the default or only behavior as in svn, but as an option supported by the tool's CLI.)

This is in my opinion once again a workflow issue, which is only needed because Git is the dominant system. If we had a native repo + server where it didn't matter, it wouldn't cross your mind to use branches here.

Currently, as far as I can tell from the documentation, jj git push is the only way to publish local changes, and that requires either pushing all anonymous branches...or assigning a named branch to the commits.

That totally is correct. I do think that you should try a workflow, where you don't immediately set a branch after a new commit as it vastly simplifies the usage of jj.

If I don't set a branch, then I need to use jj git push -a, right? So are you saying that I should try using jj git push -a for all of my pushing?

No, what I meant is what Martin and Yuya in the previous comments said.

Short description of the workflow below:

You first build a stack commits of <main branch>, which are your actual "feature" and then at a later point only create the branch to push it to Github/Gitlab. jj git push will push all the commits in <main branch>@origin..<cool-feature>, which can be your complete feature.

BatmanAoD commented 8 months ago

You first build a stack commits of <main branch>, which are your actual "feature" and then at a later point only create the branch to push it to Github/Gitlab.

I'm sorry, I'm still not understanding. I create branches in jj because I need to be able to push commits to GitLab/GitHub. There is no "later point." I generally want a "draft" merge request open as soon as possible; in my particular setup, having a merge request is lso what triggers CI checks. So there's no point at which I am making branches just for the sake of making branches, nor is there a point during which I'm making a sequence of commits that I don't intend to push immediately.

martinvonz commented 8 months ago

I think we agree that the workflow for people who want/need to advance branches between every push is not very good.

@BatmanAoD, I'm curious to hear how it goes if you use the one-line script I suggested in https://github.com/martinvonz/jj/issues/2338#issuecomment-1945463209. If that works reasonably well, then we can consider adding a flag to either jj commit or jj git push to do basically that. Does that sound reasonable? I haven't read everything in this long thread, so I may very well have missed something.

BatmanAoD commented 8 months ago

@martinvonz Yep, definitely planning to try that for a few days! Thanks for writing out those revset expressions. I do like the idea of making push be where the flag lives.

martinvonz commented 8 months ago

A possible alternative I think we should at least consider is to model a "current branch" as any branch pointing to @. I think we've talked about this somewhere but maybe it was on Discord. Some things to consider if we model it this way:

EDIT: Maybe --advance-branches is a better name. We would of course have a short form either way.

emesterhazy commented 7 months ago

Based on some offline discussion, I'd like to propose a slight modification to https://github.com/martinvonz/jj/issues/2338#issuecomment-1948989063.

With that proposal, jj new and jj commit will behave differently depending on whether @ has a branch pointing to it. I think that could lead to a confusing UX since these otherwise equivalent workflows will behave differently:

  1. jj edit -r somerev_with_branch jj commit
  2. jj edit -r somerev_with_branch jj new

With the original proposal, jj commit would advance the branch pointer while jj new would not unless --activate-branch is used.

I think people will mostly fall into one of two camps, those who always want the branch to be advanced, and those who don't.

I'd propose we add two settings to control the behavior explicitly.

  1. A global setting that either advances or doesn't advance the branch pointers anytime commit or new is used with a revision that has a branch pointer. The default value will be false to maintain the current behavior.
  2. A per-branch override so that users can enable this behavior for specific branches, such as their long lived feature branches.

With these config settings jj new and jj commit will behave consistently, and changing their behavior requires an explicit action from the user.

joyously commented 7 months ago

I'd propose we add two settings to control the behavior explicitly.

What if this is baked into init, in a way? If you are using a colocated git repo, the branches behave like Git? Otherwise, they are however jj defines branches (which I guess I've never read about). Or maybe the jj branch command is really jj git branch? Or both exist, so you can have both kinds of branches?

martinvonz commented 7 months ago

What if this is baked into init, in a way? If you are using a colocated git repo, the branches behave like Git? Otherwise, they are however jj defines branches (which I guess I've never read about).

This is about the user's preferred workflow and different people/projects have different preferences. It's pretty much unrelated to whether you use a colocated repo (I'm sure there's some correlation because people who are ready to give up on using the git binary are probably more likely to give up on using branches too).

jyn514 commented 7 months ago

I'd propose we add two settings to control the behavior explicitly.

1. A global setting that either advances or doesn't advance the branch pointers anytime `commit` or `new` is used with a revision that has a branch pointer. The default value will be false to maintain the current behavior.

2. A per-branch override so that users can enable this behavior for specific branches, such as their long lived feature branches.

With these config settings jj new and jj commit will behave consistently, and changing their behavior requires an explicit action from the user.

i like this plan. i would kinda prefer that the default for the option is true, because it's the behavior people expect coming from git, but i don't feel strongly.

i have a question: you say "branch pointer" a couple times. do you mean the existing jj branch concept? or do you mean introducing something to parallel git's idea of the "current" branch (where everything jj currently does is a "detached head" state)? i would prefer not to introduce a current branch, i think all this stuff is doable just by knowing which branches point to @-. if multiple branches point to @-, we can require people to explicitly say jj commit --advance-branch foo to specify which one they mean.

emesterhazy commented 7 months ago

I mean the jj's current concept of branches. When I say "branch pointer", I guess I actually mean RefTarget. Is there a better way to refer to it colloquially? Maybe I should just say RefTarget to be unambiguous.

I trying to write a POC for this feature based on the description I gave. I am planning to update the RefTarget for any branch which has a RefTarget containing the ref targeted by jj new or jj commit. If the RefTarget has multiple adds, then the one for the ref targeted by the command will be replaced with a ref to the new commit. The branch will still remain conflicted in that case, but it will allow the user to continue working and moving the branch along until they decide to fix the conflict.

emesterhazy commented 7 months ago

Ok, I have a prototype of the idea in https://github.com/martinvonz/jj/issues/2338#issuecomment-1949090444 working for jj commit. jj new is a little more complicated because it can take multiple revisions and also has the --insert-after and --insert-before options.

It seems reasonable to not move the branch RefTarget when --insert-before is used because that would move it backwards. I don't think we'd have to do anything special for the --insert-after case, and multiple revisions can probably be handled as if jj new was called on each revision separately.

But in any case the implementation is more complex and still needs to be written. I'll upload a commit when I have something a bit more polished.

yuja commented 7 months ago

https://github.com/martinvonz/jj/issues/2338#issuecomment-1948989063

A possible alternative I think we should at least consider is to model a "current branch" as any branch pointing to @.

One of the potential problems of this model is that, in colocated repo, git will see the current branch pointing to the editing commit. It might worsen Git interop. We can of course adjust git::export_refs() to export the current branch to its parent commit (as push would do), but I don't know if that works properly.

emesterhazy commented 7 months ago

One of the potential problems of this model is that, in colocated repo, git will see the current branch pointing to the editing commit.

Can you elaborate on what the issue is? I just tested against the WIP commit that I pushed and git is still seeing a detached head after the branch pointer is updated. And, even without this feature, the user can set the branch to the current editing commit, so that must be ok?

➜  ~/o/g/e/test2 ((3ec6c558)) jjx
@  spwmxroq emesterhazy@ 2024-02-17 00:25:35.000 -05:00 foobar c7c76efb
│  (no description set)
◉  ovowmqmn emesterhazy@ 2024-02-17 00:25:05.000 -05:00 HEAD@git 3ec6c558
│  file1
◉  zzzzzzzz root() 00000000
➜  ~/o/g/e/test2 ((3ec6c558)) jjx config set --repo branches.advance-branches true
➜  ~/o/g/e/test2 ((3ec6c558)) jjx commit -m "file2"
Working copy now at: nytolypo 9fed31fe foobar | (empty) (no description set)
Parent commit      : spwmxroq bb262bfb file2
➜  ~/o/g/e/test2 ((bb262bfb)) jjx
@  nytolypo emesterhazy@ 2024-02-17 00:25:57.000 -05:00 foobar 9fed31fe
│  (empty) (no description set)
◉  spwmxroq emesterhazy@ 2024-02-17 00:25:57.000 -05:00 HEAD@git bb262bfb
│  file2
◉  ovowmqmn emesterhazy@ 2024-02-17 00:25:05.000 -05:00 3ec6c558
│  file1
◉  zzzzzzzz root() 00000000
➜  ~/o/g/e/test2 ((bb262bfb)) git log --oneline
bb262bf (HEAD) file2
3ec6c55 file1
➜  ~/o/g/e/test2 ((bb262bfb)) git log foobar --oneline
9fed31f (foobar)
bb262bf (HEAD) file2
3ec6c55 file1

You can get into exactly the same state by calling jj branch set -r @ on the editing commit, right?

yuja commented 7 months ago

Can you elaborate on what the issue is? I just tested against the WIP commit that I pushed and git is still seeing a detached head after the branch pointer is updated. And, even without this feature, the user can set the branch to the current editing commit, so that must be ok?

I don't have real examples, but it's unusual in Git land that the current branch is ahead of the HEAD, so some frontend tools (like IDE integration) might behave badly. For example, the user could checkout the branch by using IDE Git integration without noticing that he should do jj edit instead.

Suppose this is the feature primarily designed for Git users, I think Git interop matters the most.

emesterhazy commented 7 months ago

It's also unusual in Git land to constantly have a detached head. I think that's probably the real peculiarity, and my IDE is already complaining about it:

image

The documentation mentions this as a potential issue for colocated repos:

https://github.com/martinvonz/jj/blob/aaa5d6bc4fc547b6e3aaa1f21f99972e9c7ea1f0/docs/git-compatibility.md?plain=1#L107-L113

So maybe what we really need to do is fix the detached head state if there's a single local branch pointing to the editing commit and advance branches is enabled. If anything, that should improve the UX for colocated repos. The commit I'm working on for this feature refuses to automatically advance the branch if more than two branches point to the same commit, so we can ask the user to fix any ambiguity manually. What do you think?

yuja commented 7 months ago

So maybe what we really need to do is fix the detached head state if there's a single local branch pointing to the editing commit and advance branches is enabled.

I honestly don't know. I'm not sure how much complexity would be added to translate the "current branch pointing to @" model to Git.

emesterhazy commented 7 months ago

From your second bullet point it sounds like you're describing a model where the branch sits at @- instead of @. Is that correct? I'm not sure that's what Martin was describing:

We would presumably have something like jj new --activate-branch foo, which would create a new commit on top of commit foo and move any branches pointing to foo to the new commit.

But I think you're right that keeping the branch at the @- instead of @ could yield better git compatibility. I guess I don't have a full view of the complexity involved either.

martinvonz commented 7 months ago

From your second bullet point it sounds like you're describing a model where the branch sits at @- instead of @. Is that correct? I'm not sure that's what Martin was describing:

I think we're talking about different things. My proposal was to have the branch point to @ in jj. Yuya pointed out that we would have to point it to @- when we export to Git in colocated repos. Note that jj has its own record of where branches point, so it is possible to have them point to different commits.

But I think you're right that keeping the branch at the @- instead of @ could yield better git compatibility. I guess I don't have a full view of the complexity involved either.

Yes, it does seem complicated. Thanks for pointing that out, Yuya.

BatmanAoD commented 7 months ago

I absolutely agree that having a branch automatically pointing at @- rather than @ makes more sense. There are two reasons:

emesterhazy commented 7 months ago

I updated my prototype to move the branch to @-. @yuja would you mind linking me to where in the code we update the git HEAD? I'd like to make it check out the branch that's being advanced instead of the commit hash for @-. This should fix the "detached head" state when advanceable branches is being used.

yuja commented 7 months ago

where in the code we update the git HEAD? I'd like to make it check out the branch that's being advanced instead of the commit hash for @-.

It's handled by git::reset_head(). Since this function doesn't know if the @- branch should be set to the "current branch" or not, we'll need some config knob. People like me prefer detached HEAD state.

Some related previous attempt: https://github.com/martinvonz/jj/issues/2210

emesterhazy commented 7 months ago

Thanks, I'll take a look at that PR. From https://github.com/martinvonz/jj/issues/2338#issuecomment-1949090444 the plan is to make this behavior strictly opt-in. You will be able to enable it globally, or only for certain branches. I'm optimistic that we can do this in a way that's useful for the people that want this feature, and a no-op for everyone else.

emesterhazy commented 7 months ago

I have a draft pull request up with a basic implementation. This is still WIP, but it's useable and supports automatically advancing branches with jj commit and setting the Git HEAD to the branch instead of detaching it.

I'd appreciate if anyone interested in this feature takes a look at the explanation in the PR and tries it in a test repo to provide feedback.