-
Notifications
You must be signed in to change notification settings - Fork 353
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
jj git push --allow-new
by default or not
#5094
Comments
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
|
I thought the original behavior made the most sense, and will end up typing |
Any new local bookmarks can be pushed to new remote by default, but if the bookmark is already associated with other remotes,
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 |
@steveklabnik, do you mean the explicit |
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 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. |
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. |
Sounds good to me too. |
@yuja are you planning to implement this? If not, I may have time to take a swing at it over my Christmas break. |
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
I do use bookmarks not only for pushing, and Also, bookmarks is not the git-only concept in jj So I think making it configurable from the start would be nice |
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 |
(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
git.auto-local-bookmark
for the remote)--allow-new
based on the number of the configured remotes?cli: git push: do not push new bookmarks by default #4871 (comment)
--allow-track-new
instead? (non-tracking local bookmark can be pushed to new remote, but tracking one won't by default)cli: git push: exclude new bookmarks tracking other remotes if no --remote set #4730 (comment)
jj bookmark track <name>@<new-remote>
to flag new bookmarks to be pushed?cli: git push: do not push new bookmarks by default #4871 (comment)
Previous discussions:
Related:
The text was updated successfully, but these errors were encountered: