- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 602
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
Push with refspec #2542
Push with refspec #2542
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for completeness, I'll add a reference to our discussion @vlad-anger. Thank you for the productive discussion!
In #2537 we discussed that the new behavior should be conditional on the value of push.default
: Only if push.default == upstream
, merge.*.branch
is used as the remote branch.
Just to make sure I get the context right: does any of this require changes to this part of the codebase? gitui/asyncgit/src/sync/remotes/mod.rs Lines 133 to 233 in 35b2529
Or is that not the case as this PR deals with branches while the code I linked to deals with remotes? |
Yeah those helpers out of scope, they tell which remote can be used, Ex. locally i have branch t1, which should be pushed to remote t2. |
1cc6809
to
9abc6bb
Compare
Tweaked logic, updated PR desc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for addressing my concerns. 👍
Here are some thoughts on the current version.
Apart from the review item regarding pushing tags, I believe the push logic in this version is correctly aligned with git for
|
@naseschwarz I liked first review round, let's go for another one! |
In case someone else wants to try this, too: You'll need an exact version of libgit2 after @vlad-anger's merge, as there's a breaking change afterwards: naseschwarz@13833c9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested this against the git2@d1ae3b6c2d1200e7d82468af447fa66259225ecf and it works as I would have expected.
There's only one small formatting remark.
There has been a git2 release, upgrade and patch in #2567! :) |
c5e00bc
to
859cdde
Compare
tests: cover get_branch_upstream_merge
859cdde
to
9d6336a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @vlad-anger. Please excuse me missing this in the previous two rounds, but I have one more request: Would you please add an entry to CHANGELOG.md?
9d6336a
to
259c734
Compare
Yeap added changelog. Rebased on top of master |
@naseschwarz Thanks for reviewing! :) |
@vlad-anger really great contribution! Thank you so much! @naseschwarz great work reviewing this! |
Fixes #2537
git2 merged necessary PR
This PR is_blocked until new version git2 lib released
For now use git2 from master to be able to test PR.
Assume git config:
Prev. behavior:
push -> push oblava -> remote oblava <- new remote created
New behavior:
push -> push oblava -> remote master
I followed the checklist:
make check
without errors (see commit with tests fixes)