git: match patterns, not file names, for tracked files #5423
Conversation
Right now, when we run `git lfs track`, we warn the user if the pattern they've provided matches an existing file name. However, when we invoke that command with `--filename` and a pattern with brackets on Windows, we get an error because the escaped pattern contains backslashes, which results in broken behaviour on Windows. The real cause of this problem, however, is that we're ultimately asking for a file name and not a pattern. Our invocation happens to work on Unix, but we need it to also work on Windows. To solve this, let's instead ask for ignored cached files and specify a single pattern. Since we've asked for a pattern, this results in a different behaviour from before and avoids the error. We would have caught this on Windows if our weird file name code had worked there, but since we included asterisks and question marks, which are not valid on Windows, the test couldn't run there. Instead, let's leave the brackets and run the test on all platforms. We still have the test below which does the same thing with spaces on Unix, so we're not really regressing our tests there. In addition, since we don't have any tests for this verbose logging that prints the messages that trigger the problem, let's add some, both with more common patterns and some unusual ones as well.
chrisd8088
left a comment
chrisd8088
left a comment
There was a problem hiding this comment.
Thanks for the diagnosis and the fix! I think this is much pretty much a strict improvement over the current state.
I did notice that the Git documentation on git-ls-files(1) notes:
gitignore(5)specifies the format of exclude patterns
But, of course, the rules created with git lfs track go into .gitattributes, where certain gitignore(5) patterns don't apply; in particular, foo/ doesn't match anything in the directory the way it does for a .gitignore rule.
This means we get a technically incorrect match and consequent output from git lfs track, and it will unnecessarily touch files when given a foo/ pattern.
$ git init test
$ cd test
$ mkdir foo
$ echo foo >foo/foo.bin
$ git lfs track '*.bin'
$ git add .gitattributes foo
$ git commit -m foo
$ git-lfs track -v foo/
Tracking "foo/"
Searching for files matching pattern: foo/
Found 1 files previously added to Git matching pattern: foo/
Touching "foo/foo.bin"
This behaviour exists already, though, because passing foo/ to git ls-files as a filename rather than an exclude pattern also matches the existing file in the directory. Since that's the case, this PR doesn't introduce any regression in this regard, as far as I can tell.
|
Yeah, I agree we could do better here, but I think, as you said, that this is a strict improvement, so I'm going to merge it. |
2 participants
Right now, when we run
git lfs track, we warn the user if the pattern they've provided matches an existing file name. However, when we invoke that command with--filenameand a pattern with brackets on Windows, we get an error because the escaped pattern contains backslashes, which results in broken behaviour on Windows.The real cause of this problem, however, is that we're ultimately asking for a file name and not a pattern. Our invocation happens to work on Unix, but we need it to also work on Windows. To solve this, let's instead ask for ignored cached files and specify a single pattern. Since we've asked for a pattern, this results in a different behaviour from before and avoids the error.
We would have caught this on Windows if our weird file name code had worked there, but since we included asterisks and question marks, which are not valid on Windows, the test couldn't run there. Instead, let's leave the brackets and run the test on all platforms. We still have the test below which does the same thing with spaces on Unix, so we're not really regressing our tests there.
In addition, since we don't have any tests for this verbose logging that prints the messages that trigger the problem, let's add some, both with more common patterns and some unusual ones as well.
Fixes #5409
/cc @zhoushaokun as reporter