-
-
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
base: master
Are you sure you want to change the base?
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. |
asyncgit: config add push default strategy config
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.
asyncgit/src/sync/config.rs
Outdated
Ok(PushDefaultStrategyConfig::try_from( | ||
entry_str.as_str(), | ||
) | ||
.unwrap_or_default()) |
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 a note: The behavior of the git command line client is to reject a push if the value of push.default is defined but not valid.
% git -c push.default=snerflop push origin HEAD
error: malformed value for push.default: snerflop
error: must be one of nothing, matching, simple, upstream or current
fatal: unable to parse 'push.default' from command-line config
128 %
Doing this might come in handy in case git
adds new options in the future, which gitui does not support, so we don't accidentally implement incorrect push behavior.
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.
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! |
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)