Merged
Conversation
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the simpler NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the simpler NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument. Third, the TestAdapterRegButBasicOnly() function, which passes without changes, no longer fully performs the checks it was intended to make, since the NewDownloadAdapter() and NewUploadAdapter() methods now always return a non-nil value, so using a non-nil response from them to prove that the "test" adapter is found is insufficient. We therefore update the test to confirm that the returned value from these functions is a "test" adapter, as expected, and not just a "basic" one. We also replace the use of the BasicAdapterName variable with the "basic" string to align with the other tests.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Jul 27, 2023
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() methods revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the underlying NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument. Third, although the TestAdapterRegButBasicOnly() function passes without changes, it no longer fully performs the checks it was intended to make, since the NewDownloadAdapter() and NewUploadAdapter() methods now always return a non-nil value, so using a non-nil response from them to prove that the "test" adapter was found is insufficient. We therefore update the test to confirm that the returned value from these functions is a "test" adapter, as expected, and not just a "basic" one. We also replace the use of the BasicAdapterName variable with the "basic" string to align with the other tests.
bartvdbraak
pushed a commit
to bartvdbraak/git-lfs
that referenced
this pull request
Jan 20, 2026
The three test functions in the tq/transfer_test.go source file are all named with the prefix "test" rather than "Test", and as a result, do not actually execute. This oversight dates from the original introduction of these tests in the "transfer" package in commit 10623f5 of PR git-lfs#1265. (The package was later renamed to the current "tq" package in commit 891db97 of PR git-lfs#1780.) We therefore change the test function names to begin with "Test", and resolve several test regressions which have accumulated since the tests were first added. First, the TestBasicAdapterExists() function calls the GetDownloadAdapterNames() and GetUploadAdapterNames() methods of the Manifest structure, and these now return the names of three transfer adapter implementations rather than just the original "basic" one, so we allow for all three names to appear in any order. (The "lfs-standalone-file" adapter was added in commit bb05cf5 of PR git-lfs#3748, and the "ssh" adapter was added in commit 594f8e3 of PR git-lfs#4446.) Second, the TestAdapterRegAndOverride() function expects the NewDownloadAdapter() and NewUploadAdapter() methods of the Manifest structure to return nil if the provided name argument does not match that of any registered transfer adapter. However, this has not been the behaviour of those methods since commit c5c2a75 of PR git-lfs#1279, shortly after the tests were first introduced in PR git-lfs#1265. In that commit, the NewAdapterOrDefault() method was added, and the NewDownloadAdapter() and NewUploadAdapter() methods revised to call it, so they return the default "basic" adapter if the requested name does not match a registered adapter. We therefore revise and expand the test to account for this behaviour, and also make sure to directly test the underlying NewAdapter() method, which retains the originally intended behaviour and returns nil if it does not find a matching adapter for the provided name argument. Third, although the TestAdapterRegButBasicOnly() function passes without changes, it no longer fully performs the checks it was intended to make, since the NewDownloadAdapter() and NewUploadAdapter() methods now always return a non-nil value, so using a non-nil response from them to prove that the "test" adapter was found is insufficient. We therefore update the test to confirm that the returned value from these functions is a "test" adapter, as expected, and not just a "basic" one. We also replace the use of the BasicAdapterName variable with the "basic" string to align with the other tests.
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
1 participant
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.
Extends #1265 (diff will shrink once that's merged)
Extends the batch API to include transfer adapter negotiation. On each request a list of available transfer adapters is included, and if the server includes a preferred adapter in the response then that is used to construct the adapter for uploading / downloading.
All new API fields are optional. As I've discovered though, the 1.0 schema prohibited any extra fields in the json so servers which validate against that schema, including GitHub, will need to update their request schema file even if they don't implement any custom transfer features. The schema change has been backported to 1.2.1 so hopefully will be adopted long before this client is an official version. Servers which ignore extra fields in the request work already.
This PR completes Phase 1 of the transfer enhancements discussed in https://github.com/github/git-lfs/blob/experimental/transfer-features-p2/docs/proposals/transfer_adapters.md