Ignore files: don't ignore whitespace#5076
Conversation
4cab6bd to
212e1c7
Compare
pks-t
left a comment
There was a problem hiding this comment.
The old rule always holds: whenever you think you knew everything about ignore rules, there's some new edge case coming up that you didn't expect. The question that came to my mind though is whether this behaviour is really intended by git.git. It just seems too weird :/
Some minor comments, otherwise this looks good to me. Thanks for working on it!
| assert_is_ignored("b.test"); | ||
| refute_is_ignored("c.test"); | ||
| assert_is_ignored("\tc.test"); | ||
| assert_is_ignored("d.test"); |
There was a problem hiding this comment.
This test suite always tends to confuse me. We first create files, but then afterwards those files don't matter at all as we're testing the ignored-status for nonexisting files anyway? If those files don't matter after all, couldn't we just move the test to "attr::ignore" and avoid expensive file creation?
There was a problem hiding this comment.
Huh. This is a good question. Let's punt this to a test fixup series though...
| "# this is a comment\n" | ||
| "b.test\n" | ||
| "\tc.test\n" | ||
| " # not a comment\n" |
| (*pattern == '\r' && *(pattern + 1) == '\n')) { | ||
| *base = git__next_line(pattern); | ||
| return GIT_ENOTFOUND; | ||
| } |
There was a problem hiding this comment.
This changes the path for pattern == '\0', but in fact we're still doing the same as before. Previously, we were skipping the next two blocks and then set scan = pattern, immediately aborting the loop. As scan - pattern == 0 because of that, we'd have returned GIT_ENOTFOUND, same as with your change. The only difference is that we didn't call git__next_line before, but git__next_line("\0") will just return a pointer to "\0".
Just to say that this made me wonder, but it is in fact correct.
| for (scan = pattern; *scan != '\0'; ++scan) { | ||
| /* scan until (non-escaped) white space */ | ||
| if (git__isspace(*scan) && *(scan - 1) != '\\') { | ||
| if (git__isspace(*scan) && scan > pattern && *(scan - 1) != '\\') { |
There was a problem hiding this comment.
A comment would help here. It's non-obvious at first when scan can ever be smaller than pattern, but in fact this condition's only intention is to skip the first character in case scan == pattern.
Ensure that leading whitespace is treated as being part of the filename, eg ` foo` in an ignore file indicates that a file literally named ` foo` is ignored.
Comments must have a '#' at the beginning of the line. For compatibility with git, '#' after a whitespace is a literal part of the filename.
When `allow_space` is unset, ensure that leading whitespace is not skipped.
Unlike ignore files, gitattribute files can have flexible whitespace at the beginning of the line. Ensure that by adding new ignore rules that we have not impeded correct parsing of attribute files.
212e1c7 to
4bcebe2
Compare
|
Why does this commit includes a |
|
On Sun, Jun 09, 2019 at 11:07:16AM -0700, csware wrote:
Why does this commit includes a `.bak` file?
This was by accident and is getting fixed by #5097.
|
Whitespace is significant in ignore files. Per the documentation, "trailing spaces are ignored". But leading spaces are not. Ensure that we do not skip leading spaces in ignore files.
(However, in a cruel yet typical joke, for attribute files, "leading and trailing whitespaces are ignored". So I've added a test to ensure that we didn't break that.)
Fixes #5069