send remote refspec for the other lock commands#2773
Merged
Conversation
9 tasks
ttaylorr
reviewed
Dec 8, 2017
Contributor
ttaylorr
left a comment
There was a problem hiding this comment.
These changes look good to me, but should we update the locking API documentation to show this new parameter?
Contributor
Author
|
API docs are on the checklist: #2712 |
Merged
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 19, 2023
In PR git-lfs#2773 the option to include a Git reference specification in Git LFS locking API requests was introduced, and several tests were added to the t/t-unlock.sh test suite and others modified so as to help validate this behaviour. As well, the lfstest-gitserver.go test server was altered to check for an expected "refspec" query argument in lock list requests, but only if the test repository name had the trailing suffix "branch-required". This permitted the test server to mimic existing production services whose APIs did not expect or check the "refspec" query parameter, but also emulate those services which chose to fully implement the new API feature and only list locks matching the given "refspec". Four tests of the "git lfs unlock" command were added or changed in order to check the command's behaviour when support for the "refspec" argument was added in PR git-lfs#2773. When the --id option is not used with this command, it makes an initial GET request to the Git LFS locking API to retrieve the lock's ID, if any, based on the file path provided to the command. With the changes in the PR, the current "refspec" was also sent to the API to be used in the lookup operation. In commit 257df97, the "unlocking a lock by path" test was revised into the "unlocking a lock by path with good ref" test, and the "unlocking a lock by path with tracked ref", "unlocking a lock by path with bad ref", and "unlocking a lock by id with bad ref" tests were added. All of these tests used test repository names with the "branch-required" suffix so as to cause the test API service to require a "refspec" query argument in client GET requests, and check that its value matched the branch name extracted from the test repository name as the word which preceded the suffix (e.g., "main" in the test repository name "unlock-by-path-main-branch-required"). But then in commit 9027b94 of the same PR, the three "unlocking a lock by path*" tests were all changed to actually provide a lock ID with the --id argument to the "git lfs unlock" command. The test names and test repository names were not updated, though. This change meant that the "unlocking a lock by path with bad ref" test was identical to the "unlocking a lock by id with bad ref" test, except for its names; it also meant that no tests validated the command's behaviour when only a file path was provided and the API service was configured to require a "refspec". Further, no tests were added which explicitly tested the "git lfs unlock" command against an API service which was configured to not require a "refspec" (i.e., one emulating an existing production service which did not implement the new API feature) and which tried to use only a file path to find an existing lock. The consequence of this omission was that no tests detected the fact that the "git lfs unlock" command did not send a "refspec" argument to the API service even when attempting to look up a lock by file path. As this oversight in the command's implementation has now been corrected in a prior commit in this PR, we want to ensure that our test suite validates that the command operates successfully with both older API service, which do not check "refspec"s when searching for locks, and those which do. As a first step, we change the "unlocking a lock by path with good ref" and "unlocking a lock by path with tracked ref" tests to actually use a file path rather than an ID in their "git lfs unlock" commands, while replicating them as "unlocking a lock by id*" tests which continue to use the --id option. We also alter the "unlocking a lock by path with bad ref" test to actually use a file path as well. With these changes we now have a set of tests which confirm that when the API service expects a "refspec" in a lock search request, the "git lfs unlock" command sends one. Note that without the fix to the lockIdFromPath() method of the Client structure in the "locking" package from the prior commit in this PR, our new "unlocking a lock by path with good ref", "unlocking a lock by path with tracked ref", and "unlocking a lock by path with bad ref" tests all fail. Previously the tests with these names all succeeded despite the bug because they all actually used the --id option in their "git lfs unlock" commands, as described above.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 19, 2023
As described in a previous commit in this PR, since PR git-lfs#2773 the lfstest-gitserver.go test server can mimic a Git LFS locking API service which either does or does not check for and use a "refspec" query argument when searching for a lock matching a specific file path. Four tests of the "git lfs unlock" command were added or modified in that PR which all require the test server to check for a "refspec" query argument when responding to a request to list matching locks. These tests signal the test server to perform the "refspec" argument check by using test repository names that contain the special keyword suffix "branch-required". In the previous commit in this PR where we described these tests and their history, we also corrected them and added two more of the same kind. However, as we noted in that commit, no tests were added in PR git-lfs#2773 which explicitly tested the "git lfs unlock" command against an API service that did not require the "refspec" query argument when searching for matching locks. Some tests, such as the "unlocking a lock (--json)" test, do so implicitly, but none check all aspects of the expected behaviour in this case. We therefore add a pair of tests, the first of which simply performs a "git lfs unlock" command using a file path argument and confirming it succeeds when the test server emulates an older API service that does not check the "refspec" query arugment in the lock listing request. This test is akin to others, like the "unlocking a lock (--json)" test, but explicitly named to make its intent clear. The second new test we add checks that a "git lfs unlock" command which uses a file path argument succeeds and removes a lock even if it passes a "refspec" query argument to the server that does not match the ref name sent in the original lock request, so long as the test server is emulating an older API service which does not recognize or check the "refspec" argument in lock listing requests. Note that this new "unlocking a lock by path with bad ref without a ref required" test is similar to the "unlocking a lock by path with bad ref" test, but because it configures the test server to behave like an older API service, it succeeds in removing the lock, whereas the "unlocking a lock by path with bad ref" test expects to not remove the lock due to the mismatched refs in its lock and unlock requests.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 19, 2023
In PR git-lfs#2773 the option to include a Git reference specification in Git LFS locking API requests was introduced, and several tests were added to the t/t-unlock.sh test suite and others modified so as to help validate this behaviour. As well, the lfstest-gitserver.go test server was altered to check for an expected "refspec" query parameter in lock listing requests, but only if the test repository name had the trailing suffix "branch-required". This permitted the test server to mimic existing production services whose APIs did not expect or check the "refspec" query parameter, but also emulate those services which chose to fully implement the new API feature and only list locks matching the given "refspec". Four tests of the "git lfs unlock" command were added or changed in order to check the command's behaviour when support for the "refspec" argument was added in PR git-lfs#2773. When the --id option is not used with this command, it makes an initial GET request to the Git LFS locking API to retrieve the lock's ID, if any, based on the file path provided to the command. With the changes in the PR, the current "refspec" was also sent to the API to be used in the lookup operation. In commit 257df97, the "unlocking a lock by path" test was revised into the "unlocking a lock by path with good ref" test, and the "unlocking a lock by path with tracked ref", "unlocking a lock by path with bad ref", and "unlocking a lock by id with bad ref" tests were added. All of these tests used test repository names with the "branch-required" suffix so as to cause the test API service to require a "refspec" query parameter in client GET requests, and check that its value matched the branch name extracted from the test repository name as the word which preceded the suffix (e.g., "main" in the test repository name "unlock-by-path-main-branch-required"). But then in commit 9027b94 of the same PR, the three "unlocking a lock by path*" tests were all changed to actually provide a lock ID with the --id argument to the "git lfs unlock" command. The test names and test repository names were not updated, though. This change meant that the "unlocking a lock by path with bad ref" test was identical to the "unlocking a lock by id with bad ref" test, except for its names; it also meant that no tests validated the command's behaviour when only a file path was provided on the command line, not a lock ID, and the API service was configured to require a "refspec". The consequence of this omission was that no tests detected the fact that the "git lfs unlock" command did not send a "refspec" argument to the API service even when attempting to look up a lock by file path. As this oversight in the command's implementation has now been corrected in a prior commit in this PR, we want to ensure that our test suite validates that the command operates successfully with both older API services, which do not filter on "refspec" query parameters when searching for locks, and those which do. As a first step, we change the "unlocking a lock by path with good ref" and "unlocking a lock by path with tracked ref" tests to actually use a file path rather than an ID in their "git lfs unlock" commands, while replicating them as "unlocking a lock by id*" tests which continue to use the --id option. We also alter the "unlocking a lock by path with bad ref" test to actually use a file path as well. With these changes we now have a set of tests which confirm that the "git lfs unlock" command, when supplied with a file path and not a lock ID, sends a "refspec" query parameter in its lock listing request, and either succeeds or fails depending on whether the "refspec" matches that of the lock recorded by the API service. Note that without the fix to the lockIdFromPath() method of the Client structure in the "locking" package from the prior commit in this PR, our new "unlocking a lock by path with good ref", "unlocking a lock by path with tracked ref", and "unlocking a lock by path with bad ref" tests all fail. Previously the tests with these names all succeeded despite the problem in the "git lfs unlock" command because the tests all used the command with the --id option and so did not exercise the incorrect implementation of the file path option.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Sep 19, 2023
As described in a previous commit in this PR, since PR git-lfs#2773 the lfstest-gitserver.go test server can mimic a Git LFS locking API service which either does or does not read a "refspec" query parameter and use it to filter results when searching for a lock matching a specific file path. Four tests of the "git lfs unlock" command were added or modified in that PR which all require the test server to check for a "refspec" query parameter when responding to a request to list matching locks. These tests signal the test server to perform the "refspec" argument check by using test repository names that contain the special keyword suffix "branch-required". (However, these tests did not exercise that test server behaviour because they all passed the --id option to the "git lfs unlock" command, thus causing it to make no lock listing requests to the test server.) In the previous commit in this PR where we described these tests and their history, we corrected them and added two more of the same kind which also require the test server to filter search results using the "refspec" query parameter. However, no tests were added in PR git-lfs#2773 which explicitly test the "git lfs unlock" command against an API service that did not require the "refspec" query parameter when searching for matching locks. Some tests, such as the "unlocking a lock (--json)" test, do so implicitly, but none check all aspects of the expected behaviour in this case. We therefore add a pair of tests, the first of which simply performs a "git lfs unlock" command using a file path argument and confirming it succeeds when the test server emulates an older API service that does not check the "refspec" query arugment in lock listing requests. This test is akin to others, like the "unlocking a lock (--json)" test, but explicitly named to make its intent clear. The second new test we add checks that a "git lfs unlock" command which uses a file path argument succeeds and removes a lock even if it passes a "refspec" query parameter to the server that does not match the ref name sent in the original lock request, so long as the test server is emulating an older API service and does not recognize or check the "refspec" argument in lock listing requests. Note that this new "unlocking a lock by path with bad ref without a ref required" test is similar to the "unlocking a lock by path with bad ref" test, but because it configures the test server to behave like an older API service, it succeeds in removing the lock, whereas the "unlocking a lock by path with bad ref" test expects to not remove the lock due to the mismatched refs in its lock and unlock requests.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 4, 2023
In PR git-lfs#2773 the option to include a Git reference specification in Git LFS locking API requests was introduced, and several tests were added to the t/t-unlock.sh test suite and others modified so as to help validate this behaviour. As well, the lfstest-gitserver.go test server was altered to check for an expected "refspec" query parameter in lock listing requests, but only if the test repository name had the trailing suffix "branch-required". This permitted the test server to mimic existing production services whose APIs did not expect or check the "refspec" query parameter, but also emulate those services which chose to fully implement the new API feature and only list locks matching the given "refspec". Four tests of the "git lfs unlock" command were added or changed in order to check the command's behaviour when support for the "refspec" argument was added in PR git-lfs#2773. When the --id option is not used with this command, it makes an initial GET request to the Git LFS locking API to retrieve the lock's ID, if any, based on the file path provided to the command. With the changes in the PR, the current "refspec" was also sent to the API to be used in the lookup operation. In commit 257df97, the "unlocking a lock by path" test was revised into the "unlocking a lock by path with good ref" test, and the "unlocking a lock by path with tracked ref", "unlocking a lock by path with bad ref", and "unlocking a lock by id with bad ref" tests were added. All of these tests used test repository names with the "branch-required" suffix so as to cause the test API service to require a "refspec" query parameter in client GET requests, and check that its value matched the branch name extracted from the test repository name as the word which preceded the suffix (e.g., "main" in the test repository name "unlock-by-path-main-branch-required"). But then in commit 9027b94 of the same PR, the three "unlocking a lock by path*" tests were all changed to actually provide a lock ID with the --id argument to the "git lfs unlock" command. The test names and test repository names were not updated, though. This change meant that the "unlocking a lock by path with bad ref" test was identical to the "unlocking a lock by id with bad ref" test, except for its names; it also meant that no tests validated the command's behaviour when only a file path was provided on the command line, not a lock ID, and the API service was configured to require a "refspec". The consequence of this omission was that no tests detected the fact that the "git lfs unlock" command did not send a "refspec" argument to the API service even when attempting to look up a lock by file path. As this oversight in the command's implementation has now been corrected in a prior commit in this PR, we want to ensure that our test suite validates that the command operates successfully with both older API services, which do not filter on "refspec" query parameters when searching for locks, and those which do. As a first step, we change the "unlocking a lock by path with good ref" and "unlocking a lock by path with tracked ref" tests to actually use a file path rather than an ID in their "git lfs unlock" commands, while replicating them as "unlocking a lock by id*" tests which continue to use the --id option. We also alter the "unlocking a lock by path with bad ref" test to actually use a file path as well. With these changes we now have a set of tests which confirm that the "git lfs unlock" command, when supplied with a file path and not a lock ID, sends a "refspec" query parameter in its lock listing request, and either succeeds or fails depending on whether the "refspec" matches that of the lock recorded by the API service. Note that without the fix to the lockIdFromPath() method of the Client structure in the "locking" package from the prior commit in this PR, our new "unlocking a lock by path with good ref", "unlocking a lock by path with tracked ref", and "unlocking a lock by path with bad ref" tests all fail. Previously the tests with these names all succeeded despite the problem in the "git lfs unlock" command because the tests all used the command with the --id option and so did not exercise the incorrect implementation of the file path option.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 4, 2023
As described in a previous commit in this PR, since PR git-lfs#2773 the lfstest-gitserver.go test server can mimic a Git LFS locking API service which either does or does not read a "refspec" query parameter and use it to filter results when searching for a lock matching a specific file path. Four tests of the "git lfs unlock" command were added or modified in that PR which all require the test server to check for a "refspec" query parameter when responding to a request to list matching locks. These tests signal the test server to perform the "refspec" argument check by using test repository names that contain the special keyword suffix "branch-required". (However, these tests did not exercise that test server behaviour because they all passed the --id option to the "git lfs unlock" command, thus causing it to make no lock listing requests to the test server.) In the previous commit in this PR where we described these tests and their history, we corrected them and added two more of the same kind which also require the test server to filter search results using the "refspec" query parameter. However, no tests were added in PR git-lfs#2773 which explicitly test the "git lfs unlock" command against an API service that did not require the "refspec" query parameter when searching for matching locks. Some tests, such as the "unlocking a lock (--json)" test, do so implicitly, but none check all aspects of the expected behaviour in this case. We therefore add a pair of tests, the first of which simply performs a "git lfs unlock" command using a file path argument and confirming it succeeds when the test server emulates an older API service that does not check the "refspec" query arugment in lock listing requests. This test is akin to others, like the "unlocking a lock (--json)" test, but explicitly named to make its intent clear. The second new test we add checks that a "git lfs unlock" command which uses a file path argument succeeds and removes a lock even if it passes a "refspec" query parameter to the server that does not match the ref name sent in the original lock request, so long as the test server is emulating an older API service and does not recognize or check the "refspec" argument in lock listing requests. Note that this new "unlocking a lock by path with bad ref without a ref required" test is similar to the "unlocking a lock by path with bad ref" test, but because it configures the test server to behave like an older API service, it succeeds in removing the lock, whereas the "unlocking a lock by path with bad ref" test expects to not remove the lock due to the mismatched refs in its lock and unlock requests.
chrisd8088
added a commit
to chrisd8088/git-lfs
that referenced
this pull request
Oct 4, 2023
As described in a previous commit in this PR, since PR git-lfs#2773 the lfstest-gitserver.go test server can mimic a Git LFS locking API service which either does or does not read a "refspec" query parameter and use it to filter results when searching for a lock matching a specific file path. Four tests of the "git lfs unlock" command were added or modified in that PR which all require the test server to check for a "refspec" query parameter when responding to a request to list matching locks. These tests signal the test server to perform the "refspec" argument check by using test repository names that contain the special keyword suffix "branch-required". (However, these tests did not exercise that test server behaviour because they all passed the --id option to the "git lfs unlock" command, thus causing it to make no lock listing requests to the test server.) In the previous commit in this PR where we described these tests and their history, we corrected them and added two more of the same kind which also require the test server to filter search results using the "refspec" query parameter. However, no tests were added in PR git-lfs#2773 which explicitly test the "git lfs unlock" command against an API service that did not require the "refspec" query parameter when searching for matching locks. Some tests, such as the "unlocking a lock (--json)" test, do so implicitly, but none check all aspects of the expected behaviour in this case. We therefore add a pair of tests, the first of which simply performs a "git lfs unlock" command using a file path argument and confirming it succeeds when the test server emulates an older API service that does not check the "refspec" query argument in lock listing requests. This test is akin to others, like the "unlocking a lock (--json)" test, but explicitly named to make its intent clear. The second new test we add checks that a "git lfs unlock" command which uses a file path argument succeeds and removes a lock even if it passes a "refspec" query parameter to the server that does not match the ref name sent in the original lock request, so long as the test server is emulating an older API service and does not recognize or check the "refspec" argument in lock listing requests. Note that this new "unlocking a lock by path with bad ref without a ref required" test is similar to the "unlocking a lock by path with bad ref" test, but because it configures the test server to behave like an older API service, it succeeds in removing the lock, whereas the "unlocking a lock by path with bad ref" test expects to not remove the lock due to the mismatched refs in its lock and unlock requests.
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
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.
Implements #2712 for
git lfs [ lock | unlock | locks ].