locking: ignore missing files after unlock - #6244
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes an inconsistency where git lfs unlock --json <path> could successfully remove a server lock but still report "unlocked": false due to a local chmod/stat failure when the target file didn’t exist in the current worktree (notably in multi-clone scenarios with lockable patterns).
Changes:
- Skip the post-unlock “make read-only” chmod step when the unlocked path does not exist locally.
- Add a regression test covering the “two clones + lockable pattern + missing file in the unlocking clone” scenario.
- Preserve successful unlock reporting in
--jsonoutput when the server unlock succeeds.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
locking/locks.go |
Avoids failing a successful unlock due to attempting to chmod a missing local file. |
t/t-unlock.sh |
Adds regression coverage for unlocking a lockable file that’s missing from the current clone’s worktree. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
chrisd8088
left a comment
There was a problem hiding this comment.
Thanks so much for this helpful bug fix and for including a comprehensive test!
I made some suggestions regarding the test, but only to try to simplify its setup steps and add a check of the exit code from the git lfs unlock command. Please let me know what you think of those, and thanks again for this PR!
In a prior commit in this PR we resolved the issue reported in git-lfs#6167 so that the "git lfs unlock" command no longer reports an error if the --json option is specified and the command is used to unlock an extant lock for a file which does not exist in the current working directory. At the same time, we also added a new "unlocking a missing lockable file (--json)" test to our "t/t-unlock.sh" shell test script to verify that the changes we made are effective. At present, this test is based on the reproduction steps from git-lfs#6167 and so it creates a test repository and then clones the repository twice, using one clone to lock a local file and one clone to try to unlock the file even though it does not exist in the second clone. As discussed in PR review, we can simplify this new test because we can create the necessary test conditions in a single repository just by deleting the locked file after creating the lock: git-lfs#6244 (review) We do, though, add a check that the "git lfs pull" command returns a zero exit code, which our test would not otherwise validate because we run the command in a pipeline, and the exit code from the pipeline will be that of the tee(1) command.
chrisd8088
left a comment
There was a problem hiding this comment.
Thanks again for fixing this bug!
Fixes #6167
When one clone has the lockable pattern but not the newly locked file,
git lfs unlock --json <path>can remove the server lock and then turn that successful unlock into a local failure while trying to make the missing path read-only.Before this change, the unlock POST succeeded but the command exited with
unlocked:falseand a localstat ... no such file or directoryreason.After this change, a successful unlock stays successful, and the post-unlock chmod step is skipped when the file is not present in the current worktree.
I also added a regression in
t/t-unlock.shthat reproduces the two-clone lockable-pattern case from the issue.Validation:
make -C t t-unlock.shgo test ./locking