Skip to content

send remote refspec for the other lock commands#2773

Merged
ttaylorr merged 8 commits intomasterfrom
lock-send-ref
Dec 14, 2017
Merged

send remote refspec for the other lock commands#2773
ttaylorr merged 8 commits intomasterfrom
lock-send-ref

Conversation

@technoweenie
Copy link
Contributor

Implements #2712 for git lfs [ lock | unlock | locks ].

@technoweenie technoweenie mentioned this pull request Dec 7, 2017
9 tasks
Copy link
Contributor

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good to me, but should we update the locking API documentation to show this new parameter?

@technoweenie
Copy link
Contributor Author

API docs are on the checklist: #2712

@ttaylorr ttaylorr merged commit e5bc6b9 into master Dec 14, 2017
@ttaylorr ttaylorr deleted the lock-send-ref branch December 14, 2017 21:41
@technoweenie technoweenie mentioned this pull request Jan 17, 2018
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants