-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use git protocol v2 for shallow git_repository fetches #12199
Use git protocol v2 for shallow git_repository fetches #12199
Conversation
@HackAttack can you take a look? |
Hi @michaeleisel, thanks for this fix! I tested this manually on:
It works fine everywhere except on Ubuntu 18.04:
So, this would be a regression for users of Ubuntu 18.04, which is a supported OS for Bazel. 😞 Could you explain more about the git invocation where you got an error without this additional flag? What was the error message? |
@@ -173,7 +173,7 @@ def _git_maybe_shallow(ctx, git_repo, command, *args): | |||
start = ["git", command] | |||
args_list = list(args) | |||
if git_repo.shallow: | |||
st = _execute(ctx, git_repo, start + [git_repo.shallow] + args_list) | |||
st = _execute(ctx, git_repo, ["git", "-c", "protocol.version=2", command, git_repo.shallow] + args_list) |
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.
Any reason to not make this change to start
above, so it applies everywhere?
@philwo this is just a fast path for which we handle failure for anyways (see directly below the change), and my understanding is that protocol v2 is needed for fetching arbitrary shas. so, it's not a regression in terms of stability. the example where you succeeded without it is one where it's not fetching an arbitrary sha. for specific cases like that where the sha is being advertised, it is indeed a regression in performance, and perhaps we could run this command twice, once with protocol v2, and once without @HackAttack the reason to only use it here was to limit the impact of this flag, which seems to be good since there's this regression |
I don’t think this is true.
|
It's needed for GHE on macOS at least, up until a very recent version (also note that fetching an individual sha can succeed if it just happens to be tied to a branch or something) |
Sorry, I didn't have time to look into this again yet. I'm still confused why Git is not smart enough to negotiate the correct protocol automatically :(
Did we figure out whether that's a good idea? |
@michaeleisel What kind of failing did you see? Have you seen this: #10292 |
Wanted to check in on this, is Ubuntu 18.04 still considered a supported platform? |
Without this, the git fetch was failing for me. I believe git operations run with protocol v2 should work seamlessly with other git operations, but I'm not an expert. v2 is the default in git 2.26+, but e.g. MacOS isn't there yet. The global config analogy would be
git config --global protocol.version 2
.