You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
As shown in the first of the reproduction examples below, the git lfs unlock command permits users to unlock modified files without supplying the --force option in some cases. Specifically, if the current working directory is not the root of the Git working tree, the git lfs unlock command permits locked files to be unlocked without the --force option even if the files have local modifications that have not yet been committed.
The other two reproduction examples below demonstrate that if lockable files' names (or parent directories' names) contain quotation marks or certain other non-alphanumberic characters, or if the current commit lacks parents in the Git history, the Git LFS hooks do not change the files' write permissions.
As well, the git lfs unlock command permits locked files to be unlocked without the --force option even if the files have local modifications that have not yet been committed, if the files' names or paths contain certain special characters like quotation marks.
To Reproduce
Steps to reproduce the behaviour when the current working directory is not the root of the Git working tree:
$ git clone https://github.com/chrisd8088/test
$ cd test
$ git lfs track --lockable '*.bin'
$ git add .gitattributes
$ git commit -m attrs
$ mkdir sub
$ echo foo >sub/foo.bin
$ git add sub/foo.bin
$ git commit -m foo
$ git check-attr lockable -- sub/foo.bin
sub/foo.bin: lockable: set
$ git lfs lock sub/foo.bin
$ echo bar >sub/foo.bin
$ git lfs unlock sub/foo.bin
Cannot unlock file with uncommitted changes
$ cd sub
$ git lfs unlock foo.bin
Unlocked sub/foo.bin
Steps to reproduce the behaviour when the current commit is an initial commit in the Git history:
$ git init test && cd test
$ git lfs track --lockable '*.bin'
$ echo foo >foo.bin
$ git add foo.bin
$ git commit -m foo
$ git check-attr lockable -- foo.bin
foo.bin: lockable: set
$ ls -l foo.bin | awk '{ print $1 " " $9 }'
-rw-r--r-- foo.bin
$ echo bar >bar.bin
$ git add bar.bin
$ git commit -m bar
$ git check-attr lockable -- bar.bin
bar.bin: lockable: set
$ ls -l bar.bin | awk '{ print $1 " " $9 }'
-r--r--r-- bar.bin
Steps to reproduce the behaviour when a filename contains special characters:
In all cases, the git lfs unlock command should not allow a modified file to be unlocked unless the --force option is provided, regardless of the current working directory or any special characters in the file's name or path.
And in all cases, the write permissions of lockable files should be correctly set or reset, regardless of any special characters in the files' names or paths, or whether the current commit has a parent or not.
System environment
The examples above were performed on macOS, but should behave the same way on any platform.
Output of git lfs env
git-lfs/3.5.1 (GitHub; darwin arm64; go 1.22.1)
git version 2.46.0
Additional context
The git lfs unlock command relies on the IsFileModified() function in our git package, which executes a git status --porcelain -- <pathspec> command, passing the full path to the file as the <pathspec> argument. In the example above, this would be sub/foo.bin. However, the git status command respects the current working directory and prepends any portions of that path which are within the Git working tree to the <pathspec> argument. Thus, running git status --porcelain -- sub/foo.bin from within the sub directory results in no output except a warning: could not open directory 'sub/sub/': No such file or directory message. As a result, our IsFileModified() command incorrectly asserts that the file is not modified, and the git lfs unlock command proceeds and allows a user to unlock a modified file without supplying the --force option.
To fix this issue we need to prefix the <pathspec> we pass to the git status command with the Git "magic word" top, e.g., :(top)sub/foo.bin.
The git lfs post-commit command utilizes the GetFilesChanged() function in our git package, passing just the single ref HEAD, which runs a git diff-tree HEAD command, expecting that the command will return details of the differences between the current commit and its parent(s). However, if the current commit has no parent, nothing is returned. In this case, the git lfs post-commit command just passes an empty list to the FixLockableFileWriteFlags() method of the Client structure in our locking package.
To resolve this issue we need to pass the --root option to the git diff-tree command so that it will diff an initial commit against a NULL tree and, as per the Git documentation, treat it as a "big creation event".
The git lfs post-commit command, the git lfs post-checkout command, and the git lfs unlock command use the IsFileModified() and GetFilesChanged() functions in our git package, which as described above, run the git status and git diff-tree commands, respectively. Both functions pass the core.quotePath=false configuration option to the Git commands with the explicitcomment that this should "handle special chars in filenames". However, while the Git documentation explains that this setting means that bytes higher than 0x80 will not be quoted, this does not mean that "raw" filenames are output:
Double-quotes, backslash and control characters are always escaped regardless of the setting of this variable.
In the example above, the output of git status with the core.quotePath option set to false therefore returns:
$ git -c core.quotepath=false status --porcelain --
M "bar\"bar.bin"
Since our IsFileModified() and GetFilesChanged() functions do not process the returned output to unescape special characters like double-quotes, they fail to match the Git output with the actual filenames, and as a result, the git lfs unlock command proceeds and allows a user to unlock a modified file without supplying the --force option, while the git lfs post-commit and git lfs post-checkout commands fail to remove the write permissions on those files.
To fix these issues we need to drop the core.quotePath=false Git configuration option in favour of the -z option, as we already use in many other functions. We then need to split the Git command output on null bytes, again following our existing usage in other functions.
Further, we likely want to prepend the literal "magic word" to the <pathspec> we pass to git status in our IsFileModified() function, along with the top magic word discussed above, e.g., :(top,literal)sub/foo.bin. This will ensure that if the file name or path contains * or ? characters that they are treated literally and not as "glob" matches, which might return multiple results.
The text was updated successfully, but these errors were encountered:
Describe the bug
As shown in the first of the reproduction examples below, the
git lfs unlock
command permits users to unlock modified files without supplying the--force
option in some cases. Specifically, if the current working directory is not the root of the Git working tree, thegit lfs unlock
command permits locked files to be unlocked without the--force
option even if the files have local modifications that have not yet been committed.The other two reproduction examples below demonstrate that if lockable files' names (or parent directories' names) contain quotation marks or certain other non-alphanumberic characters, or if the current commit lacks parents in the Git history, the Git LFS hooks do not change the files' write permissions.
As well, the
git lfs unlock
command permits locked files to be unlocked without the--force
option even if the files have local modifications that have not yet been committed, if the files' names or paths contain certain special characters like quotation marks.To Reproduce
Steps to reproduce the behaviour when the current working directory is not the root of the Git working tree:
Steps to reproduce the behaviour when the current commit is an initial commit in the Git history:
Steps to reproduce the behaviour when a filename contains special characters:
Expected behaviour
In all cases, the
git lfs unlock
command should not allow a modified file to be unlocked unless the--force
option is provided, regardless of the current working directory or any special characters in the file's name or path.And in all cases, the write permissions of lockable files should be correctly set or reset, regardless of any special characters in the files' names or paths, or whether the current commit has a parent or not.
System environment
The examples above were performed on macOS, but should behave the same way on any platform.
Output of
git lfs env
Additional context
The
git lfs unlock
command relies on theIsFileModified()
function in ourgit
package, which executes agit status --porcelain -- <pathspec>
command, passing the full path to the file as the<pathspec>
argument. In the example above, this would besub/foo.bin
. However, thegit status
command respects the current working directory and prepends any portions of that path which are within the Git working tree to the<pathspec>
argument. Thus, runninggit status --porcelain -- sub/foo.bin
from within thesub
directory results in no output except awarning: could not open directory 'sub/sub/': No such file or directory
message. As a result, ourIsFileModified()
command incorrectly asserts that the file is not modified, and thegit lfs unlock
command proceeds and allows a user to unlock a modified file without supplying the--force
option.To fix this issue we need to prefix the
<pathspec>
we pass to thegit status
command with the Git "magic word"top
, e.g.,:(top)sub/foo.bin
.The
git lfs post-commit
command utilizes theGetFilesChanged()
function in ourgit
package, passing just the single refHEAD
, which runs agit diff-tree HEAD
command, expecting that the command will return details of the differences between the current commit and its parent(s). However, if the current commit has no parent, nothing is returned. In this case, thegit lfs post-commit
command just passes an empty list to theFixLockableFileWriteFlags()
method of theClient
structure in ourlocking
package.To resolve this issue we need to pass the
--root
option to thegit diff-tree
command so that it will diff an initial commit against aNULL
tree and, as per the Git documentation, treat it as a "big creation event".The
git lfs post-commit
command, thegit lfs post-checkout
command, and thegit lfs unlock
command use theIsFileModified()
andGetFilesChanged()
functions in ourgit
package, which as described above, run thegit status
andgit diff-tree
commands, respectively. Both functions pass thecore.quotePath=false
configuration option to the Git commands with the explicit comment that this should "handle special chars in filenames". However, while the Git documentation explains that this setting means that bytes higher than 0x80 will not be quoted, this does not mean that "raw" filenames are output:In the example above, the output of
git status
with thecore.quotePath
option set tofalse
therefore returns:Since our
IsFileModified()
andGetFilesChanged()
functions do not process the returned output to unescape special characters like double-quotes, they fail to match the Git output with the actual filenames, and as a result, thegit lfs unlock
command proceeds and allows a user to unlock a modified file without supplying the--force
option, while thegit lfs post-commit
andgit lfs post-checkout
commands fail to remove the write permissions on those files.To fix these issues we need to drop the
core.quotePath=false
Git configuration option in favour of the-z
option, as we already use in many other functions. We then need to split the Git command output on null bytes, again following our existing usage in other functions.Further, we likely want to prepend the
literal
"magic word" to the<pathspec>
we pass togit status
in ourIsFileModified()
function, along with thetop
magic word discussed above, e.g.,:(top,literal)sub/foo.bin
. This will ensure that if the file name or path contains*
or?
characters that they are treated literally and not as "glob" matches, which might return multiple results.The text was updated successfully, but these errors were encountered: