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

Option to jj git push --allow-new by default #5094

Closed
yuja opened this issue Dec 13, 2024 · 20 comments
Closed

Option to jj git push --allow-new by default #5094

yuja opened this issue Dec 13, 2024 · 20 comments
Labels
enhancement New feature or request

Comments

@yuja
Copy link
Contributor

yuja commented Dec 13, 2024

Update: Current status

This is now implemented and released in jj 0.26; you can have jj push new branches by default by setting git.push-new-bookmark=true. Note that this is not recommended unless --allow-new really inconveniences you. (Ultimately, we'd prefer a UI where such an option would not be needed)

See also the documentation for --allow-new in jj help git push.


(Extracted from #4871 so the issue won't be buried)

29a2247 changed jj git push to not create and track new remote bookmarks by default, for the reason described in the commit message. Apparently, the new behavior is too strict for certain use cases.

Should we

Previous discussions:

Related:

@yuja yuja added the enhancement New feature or request label Dec 13, 2024
@BatmanAoD
Copy link
Contributor

Any of the first three bullets seems reasonable to me, especially the one to make the behavior depend on the number of remotes, since I haven't seen a rationale for imposing this restriction when there's only one remote, which I suspect is the common case.

I don't understand the --allow-track-new suggestion; but upthread, it looks like the original suggestion was to permit using an explicit --remote, which does makes sense.

jj bookmark track doesn't seem like a solution to the --allow-new inconvenience, but I'll comment anyway. It seems a bit odd because that's the command used to track an existing branch. Maybe it's fine for it to effectively mean both "track an existing remote branch" and "create a remote from an existing local branch", i.e., "find where this branch exists and then ensure it exists in the other place as well." I think the proposed behavior is analogous to git branch --set-upstream-to (and --track and --set-upstream), but I don't know if similarity to git is desirable in this case.

@steveklabnik
Copy link
Contributor

I thought the original behavior made the most sense, and will end up typing --allow-new in basically all cases. So allowing it to at least be configurable would be nice.

@yuja
Copy link
Contributor Author

yuja commented Dec 14, 2024

the --allow-track-new suggestion

Any new local bookmarks can be pushed to new remote by default, but if the bookmark is already associated with other remotes, --allow-track-new is required.

% jj bookmark create foo
% jj git push  # will push "foo" to the default remote (e.g. "origin")
% jj git push --remote other  # won't push "foo" because it's associated with the "origin"
% jj git push --remote other --allow-track-new  # will push "foo" to "other"

I think it works better than changing behavior based on the number of the configured remotes. The downside is that it's harder to understand/explain what --allow-track-new would do than --allow-new. (FWIW, --allow-new would have to be renamed if the default depends on the number of the configured remotes.)

@martinvonz
Copy link
Member

I thought the original behavior made the most sense

@steveklabnik, do you mean the explicit --remote argument? Or the behavior before (in 0.23.0)?

@steveklabnik
Copy link
Contributor

@martinvonz

My usage of bookmarks is basically only for interop with git remotes, so creating and then pushing them is the sort of default action. That being said, I also probably did not run afoul of the issue that motivated the change because I rarely work on projects with multiple remotes at the moment, and the ones I do, I do want them pushed everywhere.

This change, for example, breaks my tutorial; we create a repository on github that's empty, and then push our local project up to it; this now requires --allow-new.

That being said, I like @yuja 's suggestion above a lot. I think it retains the simplicity of the simple cases while still catching the original problem.

@chriskrycho
Copy link
Contributor

Concur strongly with @steveklabnik here. While I think the multiple-remotes approach I suggested was a reasonable compromise, I like @yuja’s suggestion here better than what I proposed—it makes the easy case easy, and should prevent most of the weird/possibly-dangerous scenarios that the original was trying to prevent.

I do also think some degree of configurability/tunability could be nice? But that seems separate.

@martinvonz
Copy link
Member

Sounds good to me too.

@chriskrycho
Copy link
Contributor

@yuja are you planning to implement this? If not, I may have time to take a swing at it over my Christmas break.

@yuja
Copy link
Contributor Author

yuja commented Dec 19, 2024

@yuja are you planning to implement this?

Yes, I'll basically need to restore the original patch from #4730. If there aren't any other comments, maybe I'll do next week.

yuja added a commit to yuja/jj that referenced this issue Dec 22, 2024
This partially reverts 296961c "cli: git push: do not push new bookmarks by
default." Apparently, the restriction introduced by that patch was too strict
for some use cases. The new rule is that any non-tracking (or local-only)
bookmarks will be pushed to new remote by default, but --allow-new-tracking is
required if a bookmark is already tracking other (real) remotes.

Closes jj-vcs#5094
@istudyatuni
Copy link

My usage of bookmarks is basically only for interop with git remotes

I do use bookmarks not only for pushing, and --allow-new solved the problem (to not use -b every time). Though with new --allow-track-new it will be possible to do something similar (but with a hacky way - use local remote)

Also, bookmarks is not the git-only concept in jj

So I think making it configurable from the start would be nice

@Gingeh
Copy link

Gingeh commented Dec 28, 2024

I would also like this default to at least be configurable.

Alongside interacting with git, I also use bookmarks as named changes so that I don't need to remember change ids, having these be automatically pushed when they're not already tied to a remote feels very surprising to me.

This would also mean other people's PRs that I've checked out with gh pr checkout will be automatically pushed to my own fork, which I do not want.
(in fact, I only learned about this upcoming change after searching for "local only bookmark" in the discord server because I wanted to further separate my local and tracking bookmarks)

@BatmanAoD
Copy link
Contributor

@Gingeh

Alongside interacting with git, I also use bookmarks as named changes so that I don't need to remember change ids, having these be automatically pushed when they're not already tied to a remote feels very surprising to me.

I'm not sure I understand this statement; are you saying that you've had cases where the default push revset contained bookmarks you didn't want to push, along with bookmarks you do want to push, and that jj git push now only pushes the ones you want to push?

This would also mean other people's PRs that I've checked out with gh pr checkout will be automatically pushed to my own fork, which I do not want.

My understanding is that the proposal to make the behavior depend on the number of remotes would resolve this issue.

@Gingeh
Copy link

Gingeh commented Jan 8, 2025

@BatmanAoD

are you saying that you've had cases where the default push revset contained bookmarks you didn't want to push, along with bookmarks you do want to push, and that jj git push now only pushes the ones you want to push?

I'm saying I've had bookmarks which I don't want to push and which would be pushed automatically if --allow-new was set by default.

My understanding is that the proposal to make the behavior depend on the number of remotes would resolve this issue.

I have two remotes but only one is configured for pushing, so that could solve my issue if it only counts git.push remotes.

@yuja
Copy link
Contributor Author

yuja commented Jan 8, 2025

I have two remotes but only one is configured for pushing, so that could solve my issue if it only counts git.push remotes.

Perhaps, #5173 would work in that situation assuming that other people's bookmarks are tracking their remote bookmarks. That said, it would be a bit scary to do jj git push if you have bookmarks for local use.

@BatmanAoD
Copy link
Contributor

@Gingeh

I'm saying I've had bookmarks which I don't want to push and which would be pushed automatically if --allow-new was set by default.

Prior to the most recent jj release, the behavior was always as if --allow-new were applied. Did you use jj prior to the current release, and if so, did you actually run into this issue?

...that could solve my issue if it only counts git.push remotes.

This sounds backwards to me; the idea is that if you have multiple remotes, the default would not be --allow-new, whereas with only one remote, the behavior would match the old behavior of --allow-new being the default.

@yuja
Copy link
Contributor Author

yuja commented Jan 8, 2025

I'm saying I've had bookmarks which I don't want to push and which would be pushed automatically if --allow-new was set by default.

Prior to the most recent jj release, the behavior was always as if --allow-new were applied. Did you use jj prior to the current release, and if so, did you actually run into this issue?

Just for reference, #5053 suggests that the user wants to exclude some bookmarks from the default push revset.

I'm leaning towards adding config knob because --allow-new-tracking appears not the single default which most people are happy with.

@chriskrycho
Copy link
Contributor

A config knob seems needful, yeah: it’s fairly apparent from reading this issue and other discussion that there are several very different workflows folks are using with bookmarks, and in some of them the user basically always wants to push a new bookmark when they jj git push -b some-bookmark (i.e. the pre-0.24 behavior) and in others the user basically never wants to. There’s no obvious way to reconcile those, and any heuristic we choose (including those I have proposed) will violate someone’s needs—though I think those heuristics are likely useful for thinking about what the inputs to the config knob should be?

@Gingeh
Copy link

Gingeh commented Jan 8, 2025

@BatmanAoD

Did you use jj prior to the current release, and if so, did you actually run into this issue?

Yes I did, often, and I was glad to see the change in 0.24.

This sounds backwards to me; the idea is that if you have multiple remotes, the default would not be --allow-new, whereas with only one remote, the behavior would match the old behavior of --allow-new being the default.

Yes sorry, I misunderstood the proposal. No it would not solve my issue because the checked out PRs are not tied to any remote.

yuja added a commit to yuja/jj that referenced this issue Jan 14, 2025
…y default

This goes against our rule that we shouldn't add config knob that changes the
command behavior, but I don't have any other idea to work around the problem.
Apparently, there are two parties, one who always wants to push new bookmarks,
and the other who mildly prefers to push&track new bookmarks explicitly.
Perhaps, for the former, creation of bookmarks means that the target branches
are marked to be pushed.

The added flag is a simple boolean. "non-tracking-only" behavior jj-vcs#5173 could be
implemented, but I don't want to complicate things. It's a failed attempt to
address the issue without introducing config knob.

Closes jj-vcs#5094
Closes jj-vcs#5173
github-merge-queue bot pushed a commit that referenced this issue Jan 15, 2025
…y default

This goes against our rule that we shouldn't add config knob that changes the
command behavior, but I don't have any other idea to work around the problem.
Apparently, there are two parties, one who always wants to push new bookmarks,
and the other who mildly prefers to push&track new bookmarks explicitly.
Perhaps, for the former, creation of bookmarks means that the target branches
are marked to be pushed.

The added flag is a simple boolean. "non-tracking-only" behavior #5173 could be
implemented, but I don't want to complicate things. It's a failed attempt to
address the issue without introducing config knob.

Closes #5094
Closes #5173
@yuja
Copy link
Contributor Author

yuja commented Jan 15, 2025

Fixed by f9906dc

@yuja yuja closed this as completed Jan 15, 2025
@ilyagr ilyagr pinned this issue Jan 29, 2025
@ilyagr ilyagr changed the title jj git push --allow-new by default or not Option to jj git push --allow-new by default Jan 29, 2025
@ilyagr
Copy link
Contributor

ilyagr commented Feb 6, 2025

TODO: Add this to docs/config.md, maybe. The fact that we can link to the CLI reference ameliorates the absence of the setting in config.md a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants