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

FR: Argument to jj git push that creates a branch and immediately pushes it #5472

Open
ilyagr opened this issue Jan 26, 2025 · 22 comments · May be fixed by #5698
Open

FR: Argument to jj git push that creates a branch and immediately pushes it #5472

ilyagr opened this issue Jan 26, 2025 · 22 comments · May be fixed by #5698

Comments

@ilyagr
Copy link
Contributor

ilyagr commented Jan 26, 2025

Update: The original title of this issue was "FR: When creating a branch, I'd like to be able to specify some remote(s) it can be pushed to". Scott's reply below redirected my efforts.


Is your feature request related to a problem? Please describe.

I like and use the behavior where new branches are not pushed to remotes by default. In particular, I have some branches I only want to push to origin, and some I only want to push to upstream (and sometimes perhaps some that get pushed to both)1.

However, it is annoying to me that I cannot designate a branch as "this branch should be pushed to the origin" at the moment of its creation (when I usually am thinking about that).

Describe the solution you'd like

Perhaps jj branch create --track origin could create a branch that's pre-cleared to be pushed to origin, even with jj git push --tracked.

If we use this syntax, we might also want jj branch track docs-single-page@origin to work after creating the branch. Currently, it results in Error: No such remote bookmark: docs-single-page@origin

Describe alternatives you've considered

I could, of course, disable this check entirely with #5094.

We could have jj branch create --allow-push-to origin, but then it'd be less clear whether jj git push --tracked should push it.

This might introduce too much complexity to be worth implementing.

Another, possibly easier, option would be to implement jj branch create br --push-to origin that would push the branch immediately after creating it. OTOH, this would be slightly less convenient if the push fails, and it might be a good idea to keep all push/fetch functionality confined to those two commands.

I'm not sure this is possible right now, but we could have a way to make a user command alias that creates a branch and immediately pushes it.

Footnotes

  1. More details about my use-case: I usually have a feature branch that is only pushed to origin (my personal Github fork), and then at some point I create an ig/feature branch (at the same location) that I want to push (only) to upstream. different set of branches that I want to be pushed to origin.

@ilyagr ilyagr changed the title FR: When creating a branch, I'd like to be able to specify which remote(s) it can be pushed to FR: When creating a branch, I'd like to be able to specify some remote(s) it can be pushed to Jan 26, 2025
@scott2000
Copy link
Contributor

Since in jj it's common to create bookmarks immediately before pushing, maybe this could be an option to jj git push so it could all be done as a single operation? Like jj git push -c <revset> --name <name> would push the change, but it would use the supplied name instead of the default push- prefix.

Then this issue would be solved by preventing a bookmark which has already been pushed to one remote from being pushed to a different remote.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 26, 2025

That's a good idea! It'd require me to change my workflow slightly, but not by much, and it sounds a lot easier.

@martinvonz
Copy link
Member

Isn't that #4388?

Note that jj git push -c accepts multiple revisions, so it's unclear how --name would be used. It would presumably fail if the revset had more than one revision. This is the reason for a separate command (e.g. jj git push-ref) in #4388.

@scott2000
Copy link
Contributor

I think that's a bit different because jj git push-ref would allow pushing artibtrary refs, so it presumably wouldn't create a local bookmark tracking the remote branch (since it might not even be a branch).

I think it's a lot more similar to the jj git push -c functionality, since it basically would be doing the same thing but with an explicit name. It would be strange having -c accept multiple revisions sometimes and not other times though, so I'm not sure what the best UI would be.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 26, 2025

It would be strange having -c accept multiple revisions sometimes and not other times though, so I'm not sure what the best UI would be.

I think it'd be OK to forbid --name unless -c has a single revision. We allow repeating -c; we could also (optionally) allow repeating --name so that you get a single --name per -c (and each -c would need to expand to a single revision).

We already treat a -c argument expanding to multiple revisions specially, requiring the all: modifier (IIRC).

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 29, 2025

There are some devils in the details. Currently, my plan is to have

jj git push -c abc --name fun-branch --remote qq

fail unless either fun-branch does not exist or there is a local fun-branch located precisely at abc. The latter case is so that if the push fails, you can repeat the jj git push -c abc --name fun-branch command.

As far as the remote is concerned, we'll use the usual rules (so, if the branch already existed, fast-forwards are allowed; anything else is considered a conflict and you are told to run jj git fetch).

If the remote branch exists and is untracked, we also fail (I think this is part of the "usual rules").

Let me know if you have thoughts. Is this worth having in this form?

I've been using my draft version (without any detection of same-named branches), and it's been nice, though obviously not essential.


Another alternative would be to make it more similar to

jj bookmark set -r abc fun-branch
jj git push --allow-new --remote qq -b fun-branch

One reason I didn't like that is that it leads to a question: do we also add a --allow-backwards flag to jj git push? I don't want to do that.

Update: Also, this would create a possibility that the branch was moved but the push failed. On second thought, this seems important to avoid.


I also considered recommending a shell script that does the above two commands, but that would mess up command-line completion for revisions. If aliases were powerful enough, we could perhaps go with that.

Update: Though, again, this would create a possibility that the branch was moved but the push failed.

@yuja
Copy link
Contributor

yuja commented Jan 30, 2025

jj git push -c abc --name fun-branch --remote qq

fail unless either fun-branch does not exist or there is a local fun-branch located precisely at abc. The latter case is so that if the push fails, you can repeat the jj git push -c abc --name fun-branch command.

If push failed within the same transaction, the created local bookmark would also disappear. So I think we can just recreate the same bookmark as -c (without --name) would do?

FWIW, I don't like the UI that we have to pair up -c and --name, but I don't have a better idea. --name could be a template, but it would be useful only when the user wants to give different prefix/suffix other than push-{id}.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 30, 2025

If push failed within the same transaction, the created local bookmark would also disappear.

Ah, that's nice, good point!

So, perhaps we can require the bookmark to not exist. I'm not sure whether or not there's harm in allowing the bookmark to exist at that commit, but not allowing it makes for easier code; we could start with that.

So I think we can just recreate the same bookmark as -c (without --name) would do?

What do you mean?

FWIW, I don't like the UI that we have to pair up -c and --name, but I don't have a better idea.

+1

One possibility would be to assume -c @ if --name is used, but I'm worried this would cause confusion about what --name does, and it would contradict the idea of requiring @ to be explicit for bookmark set.

--name could be a template, but it would be useful only when the user wants to give different prefix/suffix other than push-{id}.

If I understand the idea correctly, we could add this in the future (using some sort of template that uses a character uncommon in bookmark names), or this could be a separate --name-prefix or --name-template option.

@scott2000
Copy link
Contributor

Maybe it would be better if jj git push took a revset argument, and then --change and --create-bookmark were just flags to specify how to push the revset? Something like this:

# Push bookmarks in default revset
jj git push
# Push bookmarks in specified revset
jj git push REVSET
# Create bookmark based on change ID at revset
jj git push REVSET --change
# Create bookmark at revset with given name
jj git push REVSET --create-bookmark NAME

I'm not sure how well it would interact with --bookmark, --all, --tracked, and --deleted though. Probably some options would be mutually exclusive with REVSET, and --create-bookmark would require the revset to evaluate to only a single revision. It would be backwards compatible I think, because jj git push -c REVSET would still be valid (but the parsing would change), and we could add a dummy -r flag to keep jj git push -r REVSET valid too.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 30, 2025

There is also the distinction between "pushing every commit in a revset" and "pushing every bookmark in a revset". Currently, -c does the former and -r does the latter. We probably want to maintain this somehow, though perhaps there could be some short way to intersect a revset with bookmarks() (IIRC the name correctly).

--tracked and --all have more to do with -b, which is a third category, since the same revision can have multiple bookmarks.

@yuja
Copy link
Contributor

yuja commented Jan 30, 2025

So I think we can just recreate the same bookmark as -c (without --name) would do?

What do you mean?

I just meant the same rule as -c could apply to -c + --name. (Oh, but -c allows to move existing bookmarks, which seemed a bit weird.)

FWIW, I don't like the UI that we have to pair up -c and --name, but I don't have a better idea.

+1

One possibility would be to assume -c @ if --name is used, but I'm worried this would cause confusion about what --name does, and it would contradict the idea of requiring @ to be explicit for bookmark set.

I agree it would be more confusing. My point was that -c can be resolved to multiple revisions, so the same number of --name arguments have to be specified, and the order matters. That's scary.

--name could be a template, but it would be useful only when the user wants to give different prefix/suffix other than push-{id}.

If I understand the idea correctly, we could add this in the future (using some sort of template that uses a character uncommon in bookmark names), or this could be a separate --name-prefix or --name-template option.

It's just to avoid "N change revisions" + "N name arguments" problem. The template argument would override the default push-{id} template. It probably works, but the use case would be different.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 30, 2025

Oh, but -c allows to move existing bookmarks, which seemed a bit weird.

Yeah, I didn't fully think it through and follow the logic, but I think -c currently makes different choices because the bookmark name it chooses is likely to be random and unique.

My point was that -c can be resolved to multiple revisions

For now, my plan is to require it to expand to a single revision when --name is used. See also #5472 (comment) for a further (slightly weird) possibility.

@martinvonz
Copy link
Member

For now, my plan is to require it to expand to a single revision when --name is used.

It could also be separate arguments like jj git push --create-bookmark-target=@ --create-bookmark-name=my-feature (but with better names).

I think it's best to make it work only with -r and not with -c.

See also #5472 (comment) for a further (slightly weird) possibility.

We don't do anything like that anywhere else so I think it would be quite surprising.

@yuja
Copy link
Contributor

yuja commented Jan 30, 2025

It could also be separate arguments like jj git push --create-bookmark-target=@ --create-bookmark-name=my-feature (but with better names).

or single argument taking both name and revision --create-bookmark=NAME=REVSET?

@martinvonz
Copy link
Member

Yes, that seems much better, especially since it can be repeated and be combined with the existing -c/-r/-b!

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 31, 2025

I don't know how strongly I feel about it, but I'm not sure why you dislike using -c for this. I guess you feel that the difference of behavior between -c --name and -c would only grow, while I hoped it would lessen over time, especially in common usages.

I thought it was nice for jj git push -c @ to say "I want to push @ and I don't care what the resulting branch is named" while jj git push -c @ --name something would mean "I want to push @ and I do care what the resulting branch is named". I was also hoping that this would make people who only ever use -c and people who only ever use -c --name feel less alienated from each other. I was thinking that it's annoying that there are some differences in behavior between -c and -c --name, but they only seem to exist in edge cases. So, in my mind, we'd start with the simplest and most useful verison of this (single revision), and then perhaps extend it in the future, and figure out whether the differences in behavior between -c and -c --name could be reduced.

It could also be separate arguments like jj git push --create-bookmark-target=@ --create-bookmark-name=my-feature (but with better names).

or single argument taking both name and revision --create-bookmark=NAME=REVSET?

This could work, if we decide that this is worth introducing yet another flag that takes a revset to jj git push. The other advantage I saw in using -c is that it did not require doing so. I think having to many such flags will get confusing and people won't know which one to reach for. The two we have now are already subtly different (see also below).

A few related more minor unordered thoughts on this topic:

We seem to be reinventing refspecs.

I'm worried this would be somewhat awkward to type, and to define shell completions for.

I think clap allows a flag to take exactly two arguments, --create-bookmark NAME REVSET. I'm not sure this is preferable to --name NAME -c REVSET. If clap's errors in this situation are clear enough, I might slightly prefer this to --create-bookmark=NAME=REVSET.

I think it's best to make it work only with -r and not with -c.

I don't think this works, since -r only pushes already existing bookmarks that point into a revset (and not every commit in a revset), just as my reply to Scott in #5472 (comment) . Or perhaps I misunderstood what you meant.


All that said, we could try going with something like --create-bookmark=NAME=REVSET or --create-bookmark NAME REVSET. I'd call it "experimental", since at the moment this syntax seems awkward to me, but it's quite possible it'll seem natural after I use it for a bit.

It's also possible there's a better way to reorganize -c/-r/this new option/tracking maybe as opposed to merely adding on top of what we have. Scott's #5472 (comment) above and some threads on Discord indicate some amount of desire to think about a larger change, and the distinction between -c and -r, while quite useful, is also confusing.

@yuja
Copy link
Contributor

yuja commented Jan 31, 2025

What I don't like about -c REV --name NAME is that it doesn't work if the user wants to push multiple revision/bookmark pairs, whereas -c REV1 -c REV2 just works. That's inconsistent. And -c REV1 -c REV2 --name NAME1 --name NAME2 is the pattern we don't have right now, so we would want to pretty much avoid that.

-c can be extend to -c [NAME=]REV, but I'm not sure if that's a good idea. There are subtle behavior differences (such as REV cannot be expanded to multiple revisions), so it might be better to represent them as different command flags.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 31, 2025

I think that the new option should be shorter, e.g. --new as an alias for --new-bookmark or --create as an alias for --create-bookmark.

I'd like the latter name better, all else being equal, but then people will think that -c is short for --create. So I'm thinking of going with the former (unless new ideas appear).

Either way, my gut feeling remains that it might be confusing to people to have an option like --new-bookmark, and then to have a differently-behaved -c option that also (in the common case) creates a new bookmark. But I'll trust your feeling that that's better than the downsides of using -c for both. Perhaps we can make it clear with good help text or messages. If necessary, this is also something we can change later, if we get either new ideas or more feedback.

@ilyagr
Copy link
Contributor Author

ilyagr commented Jan 31, 2025

Ah, maybe --named aka --named-bookmark is a better name to distinguish it from -c .

@yuja
Copy link
Contributor

yuja commented Jan 31, 2025

How about --create-bookmark|-B? We can later merge --create-bookmark and --change if that makes sense. I'm not sure which would be better overall.

@joyously
Copy link

It seems that the original request is asking for specifying remotes, but the discussion is focusing on naming bookmarks.
Also, doesn't the -c have a configuration option for how to name its prefix? Maybe that could be dynamic?

@ilyagr
Copy link
Contributor Author

ilyagr commented Feb 3, 2025

Re my gut feeling, one thing that makes me feel better is that, with the current design, the user would get an immediate error if they confused -c and --new-bookmark (the command wouldn't do something unexpected). This is good.

@yuja

How about --create-bookmark|-B?

Sorry, I am not a huge fan. -B already means --allow-backwards in other related commands. Less importantly, we've been generally using capital letters for --allow-something. Also, --create-bookmark starts with the letter c, which makes it easy to confuse.

Again, the fact that such confusion results in an error and not misbehavior makes this better. So, I don't think --create-bookmark is entirely out of the question.

Still, I think I'd prefer something like --new-bookmark or --named-bookmark (which makes some sense since you are immediately name the bookmark) or maybe --name-revision (since you name a revision by providing this option, and also this option expects a name and a revision).

We can, of course, keep coming up with ideas; the name is easy to change.

We can later merge --create-bookmark and --change if that makes sense.

I'm glad you think this might be more feasible in the future.

@joyously

It seems that the original request is asking for specifying remotes, but the discussion is focusing on naming bookmarks.

Scott hijacked the discussion with his idea in #5472 (comment) ; his proposal seemed to achieve vaguely the same effect (enough to satisfy my use-case) and be easier than what I was thinking. Thanks Scott!

Also, doesn't the -c have a configuration option for how to name its prefix? Maybe that could be dynamic?

Yuya suggested something similar in #5472 (comment) . I think, if that's all we did, it wouldn't quite satisfy the usecase in my mind, since I want exact control over the name. Still, it's an idea worth discussing, for example as part of a more overarching solution and/or the pursuit of merging --create-bookmark and --change (if it makes sense).

I think I'd like, if possible, to implement some version of this idea relatively soon to see whether it helps with issues like #5512, so I don't mind postponing that overarching solution.

@ilyagr ilyagr changed the title FR: When creating a branch, I'd like to be able to specify some remote(s) it can be pushed to FR: Feb 14, 2025
@ilyagr ilyagr changed the title FR: FR: Argument to jj git push that creates a branch and immediately pushes it Feb 14, 2025
ilyagr added a commit that referenced this issue Feb 14, 2025
ilyagr added a commit that referenced this issue Feb 14, 2025
ilyagr added a commit that referenced this issue Feb 14, 2025
ilyagr added a commit that referenced this issue Feb 18, 2025
ilyagr added a commit that referenced this issue Feb 18, 2025
ilyagr added a commit that referenced this issue Feb 18, 2025
ilyagr added a commit that referenced this issue Feb 18, 2025
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 a pull request may close this issue.

5 participants