cli: git push: exclude new bookmarks tracking other remotes if no --remote set #4730
Conversation
|
Am I understanding this feature correctly? $ jj git remote add origin ...
$ jj git remote add upstream ...
$ jj bookmark create foo
# pushes foo to upstream and tracks it
$ jj git push -r @ --remote upstream
# this is the new behavior:
# this does nothing since foo@upstream is tracking
$ jj git push -r @
# pushes foo to origin if the remote is mentioned explicitly
$ jj git push -r @ --remote originIf I'm understanding this right, the goal seems reasonable but using |
Yes. That's why I added a hint saying "... Use --remote=origin to push to new remote."
That seemed scary. User might expect the bookmark would be pushed to the associated remote as |
This forces us to think about which repo should be used. It doesn't matter in
find_bookmarks_to_push() because moved "push-{change_id}" bookmarks are
prioritized. It doesn't matter in print_commits_ready_to_push() either, which
uses the repo only for ancestry lookup, but I think tx.repo() is better here.
…emote set If you have multiple remotes to push to, you might want to keep some changes in your private fork. Git CLI has one upstream remote per branch, but jj supports multiple tracking remotes, and therefore "jj git push" can start tracking new remotes automatically. This patch makes new bookmarks not eligible for push if tracked bookmarks already exists on other remotes. I considered adding a warning, but it's not always possible to interrupt the push shortly after a warning is emitted. This check can be turned off by specifying --remote=NAME explicitly. Another stricter (and simpler in terms of implementation) idea is to force user to pass --new-bookmark or something to push any local bookmark to new remote. martinvonz#1278
yuja
force-pushed
the
push-vvvzpmtstnpw
branch
from
5ef14aa
to
fd934f9
Compare
As someone who just recently got around starting to use |
I suppose we could have a separate flag instead, if people like that better. |
|
(No action needed) The difference between I don't think it has to be fixed in this PR. Fixing this problem might be worth the added confusion until we figure out a better approach (and I don't have one clearly in mind).
I like this line of thinking, but I'm not sure what the flag should be exactly or how to name it. Should it allow new tracking for branches that are not already tracking something? Should it exist in addition to |
|
We can of course add My feeling is that |
I would expect that |
|
Are we okay to change
This behavior is easier to explain and implement than |
|
I like this idea in general, though I'd implement it after the release to give us a month to test it out and to see whether it confuses people. Either in this way or some other, I'd like more people to spend some time trying it out and/or thinking about how well it works for them. One possibility is to have a Update: It's less clear how branch deletion fits into my concept, and it breaks the analogy a bit on second thought. It could make sense to consider deletion a kind of moving (so If we don't go with |
It might then make sense to rename This PR already has this problem: I'm not sure how bad this problem is (this PR's Update: One possible solution would be to have |
Agreed.
Yep. I wouldn't want to think about the movement on
I assume the user would notice warnings about skipped bookmarks. If they don't, they wouldn't notice push error either. I have no idea whether |
My reasoning is that people pay attention when nothing is pushed and all they see is an error. If people see the familiar progress bar of branches being pushed, they might not notice the warning or that the branch they cared about was not pushed. So, I like Aside: I've considered from time to time also having a Aside: I'm not sure |
Maybe deprecate |
|
That's an interesting thought. I don't understand your last point though: what would |
|
Checklist
If applicable:
CHANGELOG.md