Honor lfs.url when deciding on transfer adapters#3905
Merged
bk2204 merged 1 commit intogit-lfs:masterfrom Nov 13, 2019
Merged
Conversation
Currently, if you have a remote with a file URL but have lfs.url set to point to a non-file URL, your transfers will fail. This occurs because we look up whether to use a transfer adapter based on the remote URL, but we also look up whether to default to the built-in transfer adapter based on the remote URL, not the endpoint URL. We then attempt to pass the HTTP URL to the standalone transfer adapter, which doesn't work. Look up the endpoint URL as well and pass that to the code which determines whether we should use the builtin transfer adapter. If the actual endpoint is not a file URL, then we won't suggest the builtin transfer adapter.
chrisd8088
approved these changes
Nov 8, 2019
Member
chrisd8088
left a comment
There was a problem hiding this comment.
This seems all good, as far as I can tell -- thank you for the substantive and contrasting tests; they were very helpful in understanding the intent of the patch!
This was referenced Dec 11, 2019
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 17, 2023
The findStandaloneTransfer() function of the "tq" package was updated in commit 28f9b73 of PR git-lfs#3905 to determine whether to use the built-in file standalone transfer adapter based on the actual endpoint URL, taking into account settings like "lfs.url" which override the endpoint URL we would otherwise construct from the current remote's URL. However, the code still checked the "lfs.<url>.standaloneTransferAgent" setting using the URL constructed from the remote's URL, and so called both the Endpoint() and RemoteEndpoint() methods of the EndpointFinder interface. Then in commit 594f8e3 of PR git-lfs#4446, when the pure SSH transfer protocol was introduced, the call to the RemoteEndpoint() method was replaced with Endpoint(), which was done with the intent that it would honour "lfs.url" settings as well. However, this resulted in duplicate calls to Endpoint(), so we can remove one of them now. Note that the change in commit 594f8e3 of PR git-lfs#4446 implies that the URL used when looking for "lfs.<url>.standaloneTransferAgent" settings is the URL as fully determined by settings such as "lfs.url" that override what would be otherwise constructed based on the remote's URL. We expect to add documentation to this effect in a subsequent PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, if you have a remote with a file URL but have
lfs.urlset to point to a non-file URL, your transfers will fail. This occurs because we look up whether to use a transfer adapter based on the remote URL, but we also look up whether to default to the built-in transfer adapter based on the remote URL, not the endpoint URL. We then attempt to pass the HTTP URL to the standalone transfer adapter, which doesn't work.Look up the endpoint URL as well and pass that to the code which determines whether we should use the builtin transfer adapter. If the actual endpoint is not a file URL, then we won't suggest the builtin transfer adapter.
As a note to the harried reviewer, the
Endpointfunction looks up all LFS endpoints and is more complete than theRemoteEndpointfunction, which only looks up the endpoint based on the remote. We want to look up options in the config based on the remote (to work more like Git), but synthesize our own based only on the actual endpoint.Fixes #3893.
/cc @ulilaube as reporter