{"thread":{"id":"62779","subject":"git grep: ^$ false match at end of file","startedAt":"2025-01-10T00:35:16Z","lastAt":"2025-01-13T06:26:03Z","messageCount":5,"participants":["Olly Betts","Jeff King","Andreas Schwab"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"510274","messageId":"20250109235255.GA3418@survex.com","threadId":"62779","inReplyTo":null,"subject":"git grep: ^$ false match at end of file","fromName":"Olly Betts","fromEmail":"olly@survex.com","sentAt":"2025-01-09T23:52:55Z","receivedAt":"2025-01-10T00:35:16Z","isPatch":false,"sender":{"key":"olly@survex.com","avatar":null},"body":"git grep '^$' seems to match at the end of the file, reporting a line\nnumber one greater than the number of lines in that file.  This does\nnot match the behaviour of grep.\n\nTo reproduce:\n\n$ git init -q git-grep-bug\n$ cd git-grep-bug\n$ echo test > test.txt\n$ git add test.txt\n$ git commit -m test\n[master (root-commit) 55b48b26] test\n 1 file changed, 1 insertion(+)\n create mode 100644 test.txt\n$ git grep -n '^$'\ntest.txt:2:\n$ grep -n '^$' test.txt\n$\n\n(The -n option isn't required to trigger it.)\n\nI'm using the git 1:2.47.1-1 packages from Debian unstable.  I can also\nreproduce with git 1:2.48.0~rc1+next.20250101-1 from Debian\nexperimental.\n\nCheers,\n    Olly\n"},{"id":"510307","messageId":"20250110114308.GB1014503@coredump.intra.peff.net","threadId":"62779","inReplyTo":"20250109235255.GA3418@survex.com","subject":"Re: git grep: ^$ false match at end of file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-10T11:43:08Z","receivedAt":"2025-01-10T11:43:11Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 09, 2025 at 11:52:55PM +0000, Olly Betts wrote:\n\n> git grep '^$' seems to match at the end of the file, reporting a line\n> number one greater than the number of lines in that file.  This does\n> not match the behaviour of grep.\n> \n> To reproduce:\n> \n> $ git init -q git-grep-bug\n> $ cd git-grep-bug\n> $ echo test > test.txt\n> $ git add test.txt\n> $ git commit -m test\n> [master (root-commit) 55b48b26] test\n>  1 file changed, 1 insertion(+)\n>  create mode 100644 test.txt\n> $ git grep -n '^$'\n> test.txt:2:\n> $ grep -n '^$' test.txt\n\nInteresting case. Bisection shows that it started doing that in\n34349bea60 (Merge branch 'jc/grep-lookahead', 2010-01-20). So it has\nbeen that way for quite a long time. But it is doubly curious, since\nneither of the parent trees exhibit the behavior. It is the merge itself\nwhich causes the problem.\n\nIn the first-parent tree 34349bea60^1, we are still calling external\n\"grep\", which could explain why we don't see any problem. But building\nwith NO_EXTERNAL_GREP (and confirming that it uses the internal code),\nit doesn't show the problem either!\n\nSo where did the bug come from? Puzzled.\n\nThat branch itself contains a merge, e2d2e383d8 (Merge branch\n'jc/maint-1.6.4-grep-lookahead' into jc/maint-grep-lookahead,\n2010-01-12). If we merge that into 34349bea60^1, the innocent\nfirst-parent, then the bug appears. And that brings in a bunch of\nlookahead code that could plausibly be the problem.\n\nI'm still confused why 34349bea60^2 (which does have the lookahead code)\ndoesn't show the bug. I guess there's some bad interaction with what had\nhappened in the meantime along the first-parent branch.\n\nLooking at:\n\n  git diff 34349bea60^2 34349bea60 -- grep.c builtin-grep.c\n\nturns up:\n\ndiff --git a/builtin-grep.c b/builtin-grep.c\nindex 12833733db..da854fa94f 100644\n--- a/builtin-grep.c\n+++ b/builtin-grep.c\n@@ -182,8 +182,6 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\t\terror(\"'%s': %s\", filename, strerror(errno));\n \t\treturn 0;\n \t}\n-\tif (!st.st_size)\n-\t\treturn 0; /* empty file -- no grep hit */\n \tif (!S_ISREG(st.st_mode))\n \t\treturn 0;\n \tsz = xsize_t(st.st_size);\n@@ -198,6 +196,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\treturn 0;\n \t}\n \tclose(i);\n+\tdata[sz] = 0;\n \tif (opt->relative && opt->prefix_length)\n \t\tfilename = quote_path_relative(filename, -1, &buf, opt->prefix);\n \ti = grep_buffer(opt, filename, data, sz);\n@@ -223,7 +222,7 @@ static int grep_cache(struct grep_opt *opt, const char **paths, int cached)\n \t\t * are identical, even if worktree file has been modified, so use\n \t\t * cache version instead\n \t\t */\n-\t\tif (cached || (ce->ce_flags & CE_VALID)) {\n+\t\tif (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {\n \t\t\tif (ce_stage(ce))\n \t\t\t\tcontinue;\n \t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0);\n\nAh. That middle hunk seems to be the culprit. But that probably means we\nwere looking at uninitialized memory before, and the lookahead code was\nalways wrong (but got lucky when there was a non-zero byte in that final\nslot). :-/\n\nSo probably the issue is the changes from a26345b608 (grep: optimize\nbuilt-in grep by skipping lines that do not hit, 2010-01-10).\n\nI'll stop digging on it for now (but adding Junio to the cc as the\nauthor there). Probably it would have been faster just to start with a\ndebugger than to look through the history. ;)\n\n-Peff\n"},{"id":"510309","messageId":"20250110120223.GC1014503@coredump.intra.peff.net","threadId":"62779","inReplyTo":"20250110114308.GB1014503@coredump.intra.peff.net","subject":"Re: git grep: ^$ false match at end of file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-10T12:02:23Z","receivedAt":"2025-01-10T12:02:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 10, 2025 at 06:43:08AM -0500, Jeff King wrote:\n\n> I'll stop digging on it for now (but adding Junio to the cc as the\n> author there). Probably it would have been faster just to start with a\n> debugger than to look through the history. ;)\n\nOK, my curiosity got the better of me. This fixes it:\n\ndiff --git a/grep.c b/grep.c\nindex 4e155ee9e6..9eac3dd95d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1470,10 +1470,12 @@ static int look_ahead(struct grep_opt *opt,\n \t\thit = patmatch(p, bol, bol + *left_p, &m, 0);\n \t\tif (hit < 0)\n \t\t\treturn -1;\n \t\tif (!hit || m.rm_so < 0 || m.rm_eo < 0)\n \t\t\tcontinue;\n+\t\tif (m.rm_so == *left_p)\n+\t\t\tcontinue; /* don't match nothing */\n \t\tif (earliest < 0 || m.rm_so < earliest)\n \t\t\tearliest = m.rm_so;\n \t}\n \n \tif (earliest < 0) {\n\nbut it is weird to me that patmatch() will match \"^$\" to the end of the\nbuffer at all. It is just calling regexec_buf() behind the scenes, so I\nguess this is just a weird special case there, and may even depend on\nthe regex implementation. If I pass \"-P\" to use pcre instead, the\nproblem goes away even without my patch.\n\nIf we skip look-ahead the problem also goes away. I'd have thought\nmatch_line() would have the same problem, but there we process line by\nline, and regexec_buf() never even sees the newline.\n\nSo I guess the rationale is: some regexec implementations are weird\nabout this special regex, and we should not trust their result with it\non a whole buffer with newlines.\n\n-Peff\n"},{"id":"510323","messageId":"87r05ahljt.fsf@igel.home","threadId":"62779","inReplyTo":"20250110120223.GC1014503@coredump.intra.peff.net","subject":"Re: git grep: ^$ false match at end of file","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2025-01-10T12:59:18Z","receivedAt":"2025-01-10T12:59:27Z","isPatch":false,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Jan 10 2025, Jeff King wrote:\n\n> but it is weird to me that patmatch() will match \"^$\" to the end of the\n> buffer at all. It is just calling regexec_buf() behind the scenes, so I\n> guess this is just a weird special case there, and may even depend on\n> the regex implementation.\n\nShouldn't the matcher be called with REG_NOTEOL in that case?\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"510388","messageId":"20250113062601.GD767856@coredump.intra.peff.net","threadId":"62779","inReplyTo":"87r05ahljt.fsf@igel.home","subject":"Re: git grep: ^$ false match at end of file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-13T06:26:01Z","receivedAt":"2025-01-13T06:26:03Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 10, 2025 at 01:59:18PM +0100, Andreas Schwab wrote:\n\n> On Jan 10 2025, Jeff King wrote:\n> \n> > but it is weird to me that patmatch() will match \"^$\" to the end of the\n> > buffer at all. It is just calling regexec_buf() behind the scenes, so I\n> > guess this is just a weird special case there, and may even depend on\n> > the regex implementation.\n> \n> Shouldn't the matcher be called with REG_NOTEOL in that case?\n\nPerhaps. If regexec_buf() is assuming we are feeding lines, then without\nREG_NOTEOL it thinks the end of the buffer is the end of a line. Which\nmakes sense, but trips up this case because we are not feeding lines,\nbut rather a whole buffer. So the final newline is not the start of an\nempty line, but the true end of the buffer.\n\nBut what if the buffer doesn't end in a newline? In the example, the\nfile is something like \"content\\n\".  But what if it was just \"content\"?\nThen the end of the buffer really is the end of a line, isn't it? And\nREG_NOTEOL would not be appropriate.\n\nSo without REG_NOTEOL:\n\n  [this is wrong, per the report]\n  $ echo content >file.txt\n  $ git grep --no-index -n '^$' file.txt\n  file.txt:2:\n\n  [this is right]\n  $ printf content >file.txt\n  $ git grep --no-index -n '^$' file.txt\n  $ echo $?\n  1\n\nand with it, like this patch:\n\ndiff --git a/grep.c b/grep.c\nindex 4e155ee9e6..7e3b6d9474 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1467,7 +1467,7 @@ static int look_ahead(struct grep_opt *opt,\n \t\tint hit;\n \t\tregmatch_t m;\n \n-\t\thit = patmatch(p, bol, bol + *left_p, &m, 0);\n+\t\thit = patmatch(p, bol, bol + *left_p, &m, REG_NOTEOL);\n \t\tif (hit < 0)\n \t\t\treturn -1;\n \t\tif (!hit || m.rm_so < 0 || m.rm_eo < 0)\n\nwe get:\n\n  [this is now right]\n  $ git grep --no-index -n '^$' file.txt\n  $ echo $?\n  1\n\n  [and this stays right]\n  $ printf content >file.txt\n  $ git grep --no-index -n '^$' file.txt\n  $ echo $?\n  1\n\nbut:\n\n  [without REG_NOTEOL, this matches]\n  $ printf content >file.txt\n  $ git grep --no-index -n 't$' file.txt\n  file.txt:1:content\n\n  [but with that flag, it no longer does]\n  $ printf content >file.txt\n  $ git grep --no-index -n 't$' file.txt\n  $ echo $?\n  1\n\nSo I do think \"\\n\" at the end of the buffer is a special case. Perhaps\nwe should always omit it, and then leave REG_NOTEOL unset, making the\nend of the buffer consistently the end of the final line. Like this,\nwhich no longer matches \"^$\" but does match \"t$\":\n\ndiff --git a/grep.c b/grep.c\nindex 4e155ee9e6..c4bb9f1081 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1646,6 +1646,8 @@ static int grep_source_1(struct grep_opt *opt, struct grep_source *gs, int colle\n \n \tbol = gs->buf;\n \tleft = gs->size;\n+\tif (left && gs->buf[left-1] == '\\n')\n+\t\tleft--;\n \twhile (left) {\n \t\tconst char *eol;\n \t\tint hit;\n\n-Peff\n"}]}