-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
Determine remote endpoint for Git network operations #9139
Conversation
This may be premature, but I spent some time combing over the areas outlined above to search for potential inconsistencies, however nothing was found. Fetch, push, and pull, checkout, revert, clone, and the tutorial were tested, however LFS and submodules were not at this point. I will continue testing and report back if any out of the ordinary is noticed. |
This caused a "Pulling [Object object]" progress text
This comment has been minimized.
This comment has been minimized.
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.
two minor comments, looks good! 📳
This is another continuation of the proxy support started in #9126 and #9127.
Now that we're going to start looking up the proxy to use for Git network operations we're going to have to have an understanding of what the remote endpoint is gonna be for any given network operation.
Corporate proxies are often set up so that a script determines which proxy to use based on the url that the client wants to access (for https urls the script usually only gets the protocol and domain). See #9127 for details on that.
Unfortunately for us there's not a 1:1 correlation between git command and url. Take the simplest case of
git clone URL
for example. While the initial request will be toURL
it's possible that the repository could contain submodules pointing to other hosts. It's also possible that the repository is set up to use LFS in which case there may be subsequent requests to a dedicated LFS server.While there might be multiple endpoints accessed by a single Git call we only have the ability to provide Git one proxy url per protocol (http/https) so we're gonna have do do a best-effort guess at a reasonable host. We're also gonna assume that the vast majority of repositories these days do all of their communications over https.
It's also worth noting that the model we've used to provide Git with authentication details (username/password) has been based on the premise that the same credentials will work for any submodule and/or LFS instance accessed during the operation.
I've introduced a substitute method for
envForAuthentication
calledenvForRemoteOperation
. This will now be the centralized location for determining the proxy url. The actual process of setting the required environment variables will come in a follow up pr.Notes for the reviewer
This PR touches sensitive parts of the codebase such as fetch, push, and pull. Please keep a lookout for anything that looks off or alters the behavior of either of these operations in a way not described below.
Affected operations
deleteBranch
deleteBranch
is also responsible for deleting the branch on the remote which means that it's essentially apush
operation. Here I've had to introduce a new git operation to load the url of the branch remote. The risk of not being able to resolve the remote url is slim here but there's a fallback anywayExpected altered behavior: One extra Git exec to lookup the remote url of the remote tracking branch
checkout and revert
While not a network operation in Git's eyes it's possible that the repository is set up to use LFS which might have to communicate with its server on checkout or revert. While we can't tell for certain what the LFS server url is (without shelling out to git lfs) it's a pretty safe bet that the account endpoint is a decent substitute given that that's what Git LFS will use to authenticate.
Expected altered behavior: None
clone
We use whatever url is provided as the remote url
Expected altered behavior: None
fetch and fetchRefSpec
We already had access to
IRemote
objects a few levels up in the call chain to the changes in here are just forwarding that information down rather than only using thename
property of theIRemote
Expected altered behavior: None
push
This one got a bit complicated because I really didn't want to risk introducing any new behavior here. See the code for a long comment explaining the tradeoff.
Expected altered behavior: None
Tutorial repository creation
Simply a refactor to pass the known remote url to
envForRemoteOperation
Expected altered behavior: None