{"thread":{"id":"51505","subject":"[PATCH] grep: skip UTF8 checks explicitally","startedAt":"2019-07-21T18:31:19Z","lastAt":"2019-09-09T18:49:40Z","messageCount":26,"participants":["Carlo Marcelo Arenas Belón","Eric Sunshine","Johannes Schindelin","Ævar Arnfjörð Bjarmason","Carlo Arenas","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"379139","messageId":"20190721183115.14985-1-carenas@gmail.com","threadId":"51505","inReplyTo":null,"subject":"[PATCH] grep: skip UTF8 checks explicitally","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-21T18:31:15Z","receivedAt":"2019-07-21T18:31:19Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Usually PCRE is compiled with JIT support, and therefore the code\npath used includes calling pcre2_jit_match (for PCRE2), that ignores\ninvalid UTF-8 in the corpus.\n\nMake that option explicit so it can be also used when JIT is not\nenabled and pcre2_match is called instead, preventing `git grep`\nto abort when hitting the first binary blob in a fixed match\nafter ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex fc0ed73ef3..146093f590 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n-\tint ovector[30], ret, flags = 0;\n+\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n \n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\n@@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n-\tint ret, flags = 0;\n+\tint ret, flags = PCRE2_NO_UTF_CHECK;\n \tPCRE2_SIZE *ovector;\n \tPCRE2_UCHAR errbuf[256];\n \n-- \n2.22.0\n\n"},{"id":"379140","messageId":"CAPig+cTXNvOXfLMgu-b36xzdBabwQuV-hJNNdDYYC_bj11DJJQ@mail.gmail.com","threadId":"51505","inReplyTo":"20190721183115.14985-1-carenas@gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-07-21T18:34:34Z","receivedAt":"2019-07-21T18:34:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jul 21, 2019 at 2:31 PM Carlo Marcelo Arenas Belón\n wrote:\n> grep: skip UTF8 checks explicitally\n\ns/explicitally/explicitly/\n\n> Usually PCRE is compiled with JIT support, and therefore the code\n> path used includes calling pcre2_jit_match (for PCRE2), that ignores\n> invalid UTF-8 in the corpus.\n>\n> Make that option explicit so it can be also used when JIT is not\n> enabled and pcre2_match is called instead, preventing `git grep`\n> to abort when hitting the first binary blob in a fixed match\n> after ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n"},{"id":"379143","messageId":"nycvar.QRO.7.76.6.1907221340320.47@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"20190721183115.14985-1-carenas@gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-22T11:55:30Z","receivedAt":"2019-07-22T11:55:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Sun, 21 Jul 2019, Carlo Marcelo Arenas Belón wrote:\n\n> Usually PCRE is compiled with JIT support, and therefore the code\n> path used includes calling pcre2_jit_match (for PCRE2), that ignores\n> invalid UTF-8 in the corpus.\n>\n> Make that option explicit so it can be also used when JIT is not\n> enabled and pcre2_match is called instead, preventing `git grep`\n> to abort when hitting the first binary blob in a fixed match\n> after ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n\nGood idea.\n\nThe flag has been in PCRE1 since at least March 5, 2007, when the\npcre.h.in file was first recorded in their Subversion repository:\nhttps://vcs.pcre.org/pcre/code/trunk/pcre.h.in?view=log\n\nIt also was part of PCRE2 from the first revision (rev 4, in fact, where\npcre2.h.in was added):\nhttps://vcs.pcre.org/pcre2/code/trunk/src/pcre2.h.in?view=log\n\nSo I am fine with this patch.\n\nThanks,\nDscho\n\n> ---\n> grep.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index fc0ed73ef3..146093f590 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n> \t\tregmatch_t *match, int eflags)\n> {\n> -\tint ovector[30], ret, flags = 0;\n> +\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n>\n> \tif (eflags & REG_NOTBOL)\n> \t\tflags |= PCRE_NOTBOL;\n> @@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n> \t\tregmatch_t *match, int eflags)\n> {\n> -\tint ret, flags = 0;\n> +\tint ret, flags = PCRE2_NO_UTF_CHECK;\n> \tPCRE2_SIZE *ovector;\n> \tPCRE2_UCHAR errbuf[256];\n>\n> --\n> 2.22.0\n>\n>\n"},{"id":"379146","messageId":"20190722144350.46458-1-carenas@gmail.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907221340320.47@tvgsbejvaqbjf.bet","subject":"[PATCH] grep: skip UTF8 checks explicitly","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-07-22T14:43:50Z","receivedAt":"2019-07-22T14:43:54Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Usually PCRE is compiled with JIT support, and therefore the code\npath used includes calling pcre2_jit_match (for PCRE2), that ignores\ninvalid UTF-8 in the corpus.\n\nMake that option explicit so it can be also used when JIT is not\nenabled and pcre2_match is called instead, preventing `git grep`\nto abort when hitting the first binary blob in a fixed match\nafter ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n\nReviewed-by: Johannes Schindelin \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nV2: spelling fixes from Eric Sunshine\n\n grep.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex fc0ed73ef3..146093f590 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n-\tint ovector[30], ret, flags = 0;\n+\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n \n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\n@@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n-\tint ret, flags = 0;\n+\tint ret, flags = PCRE2_NO_UTF_CHECK;\n \tPCRE2_SIZE *ovector;\n \tPCRE2_UCHAR errbuf[256];\n \n-- \n2.22.0\n\n"},{"id":"379156","messageId":"87muh57t5r.fsf@evledraar.gmail.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907221340320.47@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-22T19:42:24Z","receivedAt":"2019-07-22T19:42:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 22 2019, Johannes Schindelin wrote:\n\n> Hi Carlo,\n>\n> On Sun, 21 Jul 2019, Carlo Marcelo Arenas Belón wrote:\n>\n>> Usually PCRE is compiled with JIT support, and therefore the code\n>> path used includes calling pcre2_jit_match (for PCRE2), that ignores\n>> invalid UTF-8 in the corpus.\n>>\n>> Make that option explicit so it can be also used when JIT is not\n>> enabled and pcre2_match is called instead, preventing `git grep`\n>> to abort when hitting the first binary blob in a fixed match\n>> after ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n>\n> Good idea.\n>\n> The flag has been in PCRE1 since at least March 5, 2007, when the\n> pcre.h.in file was first recorded in their Subversion repository:\n> https://vcs.pcre.org/pcre/code/trunk/pcre.h.in?view=log\n>\n> It also was part of PCRE2 from the first revision (rev 4, in fact, where\n> pcre2.h.in was added):\n> https://vcs.pcre.org/pcre2/code/trunk/src/pcre2.h.in?view=log\n\nThanks for digging, that portability indeed sounds just fine.\n\n> So I am fine with this patch.\n\nI'm not, not because I dislike the approach. I haven't made up my mind\nyet.\n\nI stopped paying attention to this error-with-not-JIT discussion when I\nheard that some other series going into next for Windows fixed that\nissue[1]\n\nBut now we have it again in some form? My ab/no-kwset has a lot of tests\nfor encodings & locales combined with grep, don't some of those trigger\nthis? If so we should make any such failure a test & part of this patch.\n\nRight now we don't have the info of whether we're really using the JIT\nor not, but that would be easy to add to grep's --debug mode for use in\na test prereq.\n\nAs noted in [2] I'd be inclined to go the other way, if we indeed have\nsome cases where PCRE skips its own checks does not dying actually give\nus anything useful? I'd think not, so just ignoring the issue seems like\nthe wrong thing to do.\n\nSurely we're not producing useful grep results at that point, so just\nnot dying and mysteriously returning either nothing or garbage isn't\ngoing to help much...\n\n1. https://public-inbox.org/git/xmqq4l3wxk8j.fsf@gitster-ct.c.googlers.com/\n2. https://public-inbox.org/git/87pnms7kv0.fsf@evledraar.gmail.com/\n\n> Thanks,\n> Dscho\n>\n>> ---\n>> grep.c | 4 ++--\n>> 1 file changed, 2 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/grep.c b/grep.c\n>> index fc0ed73ef3..146093f590 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n>> static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n>> \t\tregmatch_t *match, int eflags)\n>> {\n>> -\tint ovector[30], ret, flags = 0;\n>> +\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n>>\n>> \tif (eflags & REG_NOTBOL)\n>> \t\tflags |= PCRE_NOTBOL;\n>> @@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>> static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n>> \t\tregmatch_t *match, int eflags)\n>> {\n>> -\tint ret, flags = 0;\n>> +\tint ret, flags = PCRE2_NO_UTF_CHECK;\n>> \tPCRE2_SIZE *ovector;\n>> \tPCRE2_UCHAR errbuf[256];\n>>\n>> --\n>> 2.22.0\n>>\n>>\n"},{"id":"379164","messageId":"CAPUEspg1nUoPApTk5J2r_-9psxTTSC7nRAPw_X9no+2sFVSxAA@mail.gmail.com","threadId":"51505","inReplyTo":"87muh57t5r.fsf@evledraar.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-23T03:50:27Z","receivedAt":"2019-07-23T03:50:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jul 22, 2019 at 12:42 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> On Mon, Jul 22 2019, Johannes Schindelin wrote:\n>\n> > So I am fine with this patch.\n>\n> I'm not, not because I dislike the approach. I haven't made up my mind\n> yet.\n\nmy bad, I should had explained better the regression I was trying to\nfix with this patch :\n\n$ git version\ngit version 2.22.0.709.g102302147b.dirty\n$ git grep \"Nguyễn Thái\"\n.mailmap:Nguyễn Thái Ngọc Duy \nfatal: pcre2_match failed with error code -22: UTF-8 error: isolated\nbyte with 0x80 bit set\n$ git grep -I \"Nguyễn Thái\"\n.mailmap:Nguyễn Thái Ngọc Duy \npo/TEAMS:Members: Nguyễn Thái Ngọc Duy \npo/vi.po:# Nguyễn Thái Ngọc Duy , 2012.\nt/t1302-repo-version.sh:# Copyright (c) 2007 Nguyễn Thái Ngọc Duy\nt/t2104-update-index-skip-worktree.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\nfatal: pcre2_match failed with error code -8: UTF-8 error: byte 2 top\nbits not 0x80\n\n> I stopped paying attention to this error-with-not-JIT discussion when I\n> heard that some other series going into next for Windows fixed that\n> issue[1]\n>\n> But now we have it again in some form? My ab/no-kwset has a lot of tests\n> for encodings & locales combined with grep, don't some of those trigger\n> this? If so we should make any such failure a test & part of this patch.\n\nI don't have a windows environment to test, and I admit I wasn't\nfollowing clearly\nthe whole conversation but at least in my case, I never got any test\nto fail, and I haven't\nseen any test with broken UTF-8.\n\nI apologize for not sending a test and will correct that, but I am\nconcerned that\naccording to PCRE documentation the behaviour is undefined and therefore the\ntest might be flacky.\n\n> Right now we don't have the info of whether we're really using the JIT\n> or not, but that would be easy to add to grep's --debug mode for use in\n> a test prereq.\n\nI would think that extending testtool would be a better option, I have indeed\na patch checking for UTF-8 support using git directly which I hadn't sent just\nbecause I thought it looked too hacky, but agree it will be nice to have all\nthose dependencies clear and the corresponding tests otherwise\n\nIt is interesting to note that because of the conservative way we enable UTF-8\nthen it will depend on several factors if the problematic code gets triggered\n\n> As noted in [2] I'd be inclined to go the other way, if we indeed have\n> some cases where PCRE skips its own checks does not dying actually give\n> us anything useful? I'd think not, so just ignoring the issue seems like\n> the wrong thing to do.\n\nwe could reenable the checks by moving out of the JIT fast path in pcre so\nthat pcre2_match is used for all matches (with JIT compiled as an optimization)\nand that might have the added benefit of solving the PCRE errors when JIT is\nbroken[1]\n\n$ git version\ngit version 2.22.0\n$ git grep \"Nguyễn Thái\"\n.mailmap:Nguyễn Thái Ngọc Duy \npo/TEAMS:Members: Nguyễn Thái Ngọc Duy \npo/vi.po:# Nguyễn Thái Ngọc Duy , 2012.\nt/t1302-repo-version.sh:# Copyright (c) 2007 Nguyễn Thái Ngọc Duy\nt/t2104-update-index-skip-worktree.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\nt/t7011-skip-worktree-reading.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\nt/t7012-skip-worktree-writing.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n\nimportant to note that at least on my system (macOS 10.14.6) the output of all\nengines for grep is the same for a UTF-8 pattern match, even if using invalid\nUTF-8 in the corpus, and I would expect that to be a better indicative\nof correctness\n\nFWIW GNU grep (with -P) also ignores UTF-8 errors using the same flag and\neven PCRE seems to be going in the other direction with the recent addition\nof PCRE2_MATCH_INVALID_UTF[1]\n\n$ od -b int.p\n0000000 102 145 154 303 263 156 012 303\n$ pcre2grep -U 'Beló' int.p\nBelón\n\nCarlo\n\n[1] https://public-inbox.org/git/20181209230024.43444-1-carenas@gmail.com/\n[2] https://lists.exim.org/lurker/message/20190528.141422.2af1e386.gl.html\n"},{"id":"379165","messageId":"nycvar.QRO.7.76.6.1907231444250.47@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"CAPUEspg1nUoPApTk5J2r_-9psxTTSC7nRAPw_X9no+2sFVSxAA@mail.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-23T12:46:39Z","receivedAt":"2019-07-23T12:47:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Mon, 22 Jul 2019, Carlo Arenas wrote:\n\n> On Mon, Jul 22, 2019 at 12:42 PM Ævar Arnfjörð Bjarmason\n> wrote:\n> >\n> > On Mon, Jul 22 2019, Johannes Schindelin wrote:\n> >\n> > > So I am fine with this patch.\n> >\n> > I'm not, not because I dislike the approach. I haven't made up my mind\n> > yet.\n>\n> my bad, I should had explained better the regression I was trying to\n> fix with this patch :\n>\n> $ git version\n> git version 2.22.0.709.g102302147b.dirty\n> $ git grep \"Nguyễn Thái\"\n> .mailmap:Nguyễn Thái Ngọc Duy \n> fatal: pcre2_match failed with error code -22: UTF-8 error: isolated\n> byte with 0x80 bit set\n> $ git grep -I \"Nguyễn Thái\"\n> .mailmap:Nguyễn Thái Ngọc Duy \n> po/TEAMS:Members: Nguyễn Thái Ngọc Duy \n> po/vi.po:# Nguyễn Thái Ngọc Duy , 2012.\n> t/t1302-repo-version.sh:# Copyright (c) 2007 Nguyễn Thái Ngọc Duy\n> t/t2104-update-index-skip-worktree.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n> fatal: pcre2_match failed with error code -8: UTF-8 error: byte 2 top\n> bits not 0x80\n\nThis is worrisome.\n\nThe entire reason why we switch on Unicode mode is because it matters\nfor regular expressions. Take for example this one:\n\n\tNguyễ*n\n\nSee how the ễ is supposed to be present an arbitrary number of times\n(including 0 times)? If PCRE2 was to interpret this as UTF-8, it would\nunderstand that the bytes 0xe1 0xbb 0x85 had to be repeated (or be\nmissing), but if it interprets it _not_ as UTF-8, then it would handle\n_only the last_ byte, 0x85, that way, which will not work correctly.\n\nSo when PCRE2 complains about the top two bits not being 0x80, it fails\nto parse the bytes correctly (byte 2 is 0xbb, whose two top bits are\nindeed 0x80).\n\nMaybe this is a bug in your PCRE2 version? Mine is 10.33... and this\ndoes not happen here... But then, I don't need the `-I` option, and my\noutput looks like this:\n\n-- snip --\n$ git grep --perl-regexp \"Nguyễn Thái\"\n.mailmap:Nguyễn Thái Ngọc Duy \npo/TEAMS:Members: Nguyễn Thái Ngọc Duy \npo/vi.po:# Nguyễn Thái Ngọc Duy , 2012.\nt/t1302-repo-version.sh:# Copyright (c) 2007 Nguyễn Thái Ngọc Duy\nt/t2104-update-index-skip-worktree.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\nt/t7011-skip-worktree-reading.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\nt/t7012-skip-worktree-writing.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n-- snap --\n\nNo error, you see?\n\nOr maybe it is a problem with the locale? Is your locale using an\nencoding different than UTF-8?\n\n> > I stopped paying attention to this error-with-not-JIT discussion when I\n> > heard that some other series going into next for Windows fixed that\n> > issue[1]\n> >\n> > But now we have it again in some form? My ab/no-kwset has a lot of tests\n> > for encodings & locales combined with grep, don't some of those trigger\n> > this? If so we should make any such failure a test & part of this patch.\n>\n> I don't have a windows environment to test, and I admit I wasn't\n> following clearly the whole conversation but at least in my case, I\n> never got any test to fail, and I haven't seen any test with broken\n> UTF-8.\n\nThe problem was not Windows-specific. It was specific to PCRE2 versions\ncompiled without JIT support (which was the case in Git for Windows'\nSDK, but I fixed this on July 5th).\n\nYour patch adequately addresses this problem: instead of erroring out in\nthe non-JIT version and passing in the JIT version, it will let also the\nnon-JIT version pass.\n\nBut still, I think the issue you found merits some deeper investigation,\nmethinks.\n\n> > As noted in [2] I'd be inclined to go the other way, if we indeed have\n> > some cases where PCRE skips its own checks does not dying actually give\n> > us anything useful? I'd think not, so just ignoring the issue seems like\n> > the wrong thing to do.\n>\n> we could reenable the checks by moving out of the JIT fast path in pcre so\n> that pcre2_match is used for all matches (with JIT compiled as an optimization)\n> and that might have the added benefit of solving the PCRE errors when JIT is\n> broken[1]\n>\n> $ git version\n> git version 2.22.0\n> $ git grep \"Nguyễn Thái\"\n> .mailmap:Nguyễn Thái Ngọc Duy \n> po/TEAMS:Members: Nguyễn Thái Ngọc Duy \n> po/vi.po:# Nguyễn Thái Ngọc Duy , 2012.\n> t/t1302-repo-version.sh:# Copyright (c) 2007 Nguyễn Thái Ngọc Duy\n> t/t2104-update-index-skip-worktree.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n> t/t7011-skip-worktree-reading.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n> t/t7012-skip-worktree-writing.sh:# Copyright (c) 2008 Nguyễn Thái Ngọc Duy\n>\n> important to note that at least on my system (macOS 10.14.6) the output of all\n> engines for grep is the same for a UTF-8 pattern match, even if using invalid\n> UTF-8 in the corpus, and I would expect that to be a better indicative\n> of correctness\n\nRight. It is _inconsistent_ at the moment, and with your patch it would\nat least be consistent again.\n\nSo I still would prefer your patch over \"going the other way\".\n\n> FWIW GNU grep (with -P) also ignores UTF-8 errors using the same flag and\n> even PCRE seems to be going in the other direction with the recent addition\n> of PCRE2_MATCH_INVALID_UTF[1]\n\nGood point!\n\nCiao,\nDscho\n\n>\n> $ od -b int.p\n> 0000000 102 145 154 303 263 156 012 303\n> $ pcre2grep -U 'Beló' int.p\n> Belón\n>\n> Carlo\n>\n> [1] https://public-inbox.org/git/20181209230024.43444-1-carenas@gmail.com/\n> [2] https://lists.exim.org/lurker/message/20190528.141422.2af1e386.gl.html\n>\n"},{"id":"379193","messageId":"CAPUEspgytjywYHywGT4yAwYpQbMUicN4bpHfoD+M_HMe8YdPtg@mail.gmail.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907231444250.47@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-24T01:47:38Z","receivedAt":"2019-07-24T01:47:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jul 23, 2019 at 5:47 AM Johannes Schindelin\n wrote:\n>\n> So when PCRE2 complains about the top two bits not being 0x80, it fails\n> to parse the bytes correctly (byte 2 is 0xbb, whose two top bits are\n> indeed 0x80).\n\nthe error is confusing but it is not coming from the pattern, but from\nwhat PCRE2 calls\nthe subject.\n\nmeaning that while going through the repository it found content that\nit tried to match but\nthat it is not valid UTF-8, like all the png and a few txt files that\nare not encoded as\nUTF-8 (ex: t/t3900/ISO8859-1.txt).\n\n> Maybe this is a bug in your PCRE2 version? Mine is 10.33... and this\n> does not happen here... But then, I don't need the `-I` option, and my\n> output looks like this:\n\n-I was just an attempt to workaround the obvious binary files (like\nPNG); I'll assume you\nshould be able to reproduce if using a non JIT enabled PCRE2,\nregardless of version.\n\nmy point was that unlike in your report, I didn't have any test cases\nfailing, because\nAFAIK there are no test cases using broken UTF-8 (the ones with binary data are\nactually valid zero terminated UTF-8 strings)\n\nCarlo\n"},{"id":"379194","messageId":"xmqqa7d4rxm9.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"20190722144350.46458-1-carenas@gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-24T02:10:22Z","receivedAt":"2019-07-24T02:10:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> Usually PCRE is compiled with JIT support, and therefore the code\n> path used includes calling pcre2_jit_match (for PCRE2), that ignores\n> invalid UTF-8 in the corpus.\n>\n> Make that option explicit so it can be also used when JIT is not\n> enabled and pcre2_match is called instead, preventing `git grep`\n> to abort when hitting the first binary blob in a fixed match\n> after ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n>\n> Reviewed-by: Johannes Schindelin \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> V2: spelling fixes from Eric Sunshine\n\nGood. I was expecting fallouts like this from our recent push to\naggressively use pcre and that was why I merged ab/no-kwset before\nI felt comfortable, so that we can have longer exposure. It seems\nto be paying off.\n\nSo with JIT, PCRE_NO_UTF8_CHECK is on by default, but without, we\nneed to give the option explicitly, and because it is on by default\nin the JIT case, it would not hurt to explicitly pass it? \n\nThat makes perfect sense to me.\n\n> grep.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index fc0ed73ef3..146093f590 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n> \t\tregmatch_t *match, int eflags)\n> {\n> -\tint ovector[30], ret, flags = 0;\n> +\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n> \n> \tif (eflags & REG_NOTBOL)\n> \t\tflags |= PCRE_NOTBOL;\n> @@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n> \t\tregmatch_t *match, int eflags)\n> {\n> -\tint ret, flags = 0;\n> +\tint ret, flags = PCRE2_NO_UTF_CHECK;\n> \tPCRE2_SIZE *ovector;\n> \tPCRE2_UCHAR errbuf[256];\n"},{"id":"379198","messageId":"nycvar.QRO.7.76.6.1907241245540.21907@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"CAPUEspgytjywYHywGT4yAwYpQbMUicN4bpHfoD+M_HMe8YdPtg@mail.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-24T10:47:28Z","receivedAt":"2019-07-24T10:47:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Carlo,\n\nOn Tue, 23 Jul 2019, Carlo Arenas wrote:\n\n> On Tue, Jul 23, 2019 at 5:47 AM Johannes Schindelin\n> wrote:\n> >\n> > So when PCRE2 complains about the top two bits not being 0x80, it fails\n> > to parse the bytes correctly (byte 2 is 0xbb, whose two top bits are\n> > indeed 0x80).\n>\n> the error is confusing but it is not coming from the pattern, but from\n> what PCRE2 calls\n> the subject.\n>\n> meaning that while going through the repository it found content that\n> it tried to match but\n> that it is not valid UTF-8, like all the png and a few txt files that\n> are not encoded as\n> UTF-8 (ex: t/t3900/ISO8859-1.txt).\n>\n> > Maybe this is a bug in your PCRE2 version? Mine is 10.33... and this\n> > does not happen here... But then, I don't need the `-I` option, and my\n> > output looks like this:\n>\n> -I was just an attempt to workaround the obvious binary files (like\n> PNG); I'll assume you\n> should be able to reproduce if using a non JIT enabled PCRE2,\n> regardless of version.\n>\n> my point was that unlike in your report, I didn't have any test cases\n> failing, because\n> AFAIK there are no test cases using broken UTF-8 (the ones with binary data are\n> actually valid zero terminated UTF-8 strings)\n\nThank you for this explanation. I think it makes a total lot of sense.\n\nSo your motivation for this patch is actually a different one than mine,\nand I would like to think that this actually strengthens the case _in\nfavor_ of it. The patch kind of kills two birds with one stone.\n\nThanks,\nDscho\n"},{"id":"379199","messageId":"nycvar.QRO.7.76.6.1907241248000.21907@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"xmqqa7d4rxm9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-24T10:50:17Z","receivedAt":"2019-07-24T10:50:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 23 Jul 2019, Junio C Hamano wrote:\n\n> Carlo Marcelo Arenas Belón writes:\n>\n> > Usually PCRE is compiled with JIT support, and therefore the code\n> > path used includes calling pcre2_jit_match (for PCRE2), that ignores\n> > invalid UTF-8 in the corpus.\n> >\n> > Make that option explicit so it can be also used when JIT is not\n> > enabled and pcre2_match is called instead, preventing `git grep`\n> > to abort when hitting the first binary blob in a fixed match\n> > after ed0479ce3d (\"Merge branch 'ab/no-kwset' into next\", 2019-07-15)\n> >\n> > Reviewed-by: Johannes Schindelin \n> > Signed-off-by: Carlo Marcelo Arenas Belón \n> > ---\n> > V2: spelling fixes from Eric Sunshine\n>\n> Good. I was expecting fallouts like this from our recent push to\n> aggressively use pcre and that was why I merged ab/no-kwset before\n> I felt comfortable, so that we can have longer exposure. It seems\n> to be paying off.\n\nI agree: it pays off quite nicely.\n\n> So with JIT, PCRE_NO_UTF8_CHECK is on by default, but without, we\n> need to give the option explicitly, and because it is on by default\n> in the JIT case, it would not hurt to explicitly pass it?\n>\n> That makes perfect sense to me.\n\nMy reading of the situation is slightly different. I think\nPCRE_NO_UTF8_CHECK is off by default, but it only makes a difference in\nthe non-JIT'ed code path. Since we use PCRE2's JIT when possible\n(because it leads to a quite nice performance improvement), we usually\ndon't see those warnings. Carlo's patch makes the non-JIT'ed code path\nbehave the same as our preferred code path.\n\nThanks,\nDscho\n\n>\n> > grep.c | 4 ++--\n> > 1 file changed, 2 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/grep.c b/grep.c\n> > index fc0ed73ef3..146093f590 100644\n> > --- a/grep.c\n> > +++ b/grep.c\n> > @@ -409,7 +409,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n> > static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n> > \t\tregmatch_t *match, int eflags)\n> > {\n> > -\tint ovector[30], ret, flags = 0;\n> > +\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n> >\n> > \tif (eflags & REG_NOTBOL)\n> > \t\tflags |= PCRE_NOTBOL;\n> > @@ -554,7 +554,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> > static int pcre2match(struct grep_pat *p, const char *line, const char *eol,\n> > \t\tregmatch_t *match, int eflags)\n> > {\n> > -\tint ret, flags = 0;\n> > +\tint ret, flags = PCRE2_NO_UTF_CHECK;\n> > \tPCRE2_SIZE *ovector;\n> > \tPCRE2_UCHAR errbuf[256];\n>\n"},{"id":"379208","messageId":"xmqq5znrs9d0.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907241248000.21907@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-24T16:08:59Z","receivedAt":"2019-07-24T16:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> My reading of the situation is slightly different. I think\n> PCRE_NO_UTF8_CHECK is off by default, but it only makes a difference in\n> the non-JIT'ed code path. Since we use PCRE2's JIT when possible\n> (because it leads to a quite nice performance improvement), we usually\n> don't see those warnings. Carlo's patch makes the non-JIT'ed code path\n> behave the same as our preferred code path.\n\nYou're right and we do want to make both codepaths behave the same\nway. The case we are having trouble with is without JIT, where the\nmachinery to look for needle in a non-UTF8 binary haystack barfs and\ndies, which is unacceptable (imagine \"git log --grep=...\"), and we\nwant the machinery to ignore random binary gunk just like the JIT\ncodepath does.\n\nThanks.\n\n"},{"id":"379213","messageId":"87lfwn70nb.fsf@evledraar.gmail.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907241245540.21907@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-24T18:22:48Z","receivedAt":"2019-07-24T18:22:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jul 24 2019, Johannes Schindelin wrote:\n\n> Hi Carlo,\n>\n> On Tue, 23 Jul 2019, Carlo Arenas wrote:\n>\n>> On Tue, Jul 23, 2019 at 5:47 AM Johannes Schindelin\n>> wrote:\n>> >\n>> > So when PCRE2 complains about the top two bits not being 0x80, it fails\n>> > to parse the bytes correctly (byte 2 is 0xbb, whose two top bits are\n>> > indeed 0x80).\n>>\n>> the error is confusing but it is not coming from the pattern, but from\n>> what PCRE2 calls\n>> the subject.\n>>\n>> meaning that while going through the repository it found content that\n>> it tried to match but\n>> that it is not valid UTF-8, like all the png and a few txt files that\n>> are not encoded as\n>> UTF-8 (ex: t/t3900/ISO8859-1.txt).\n>>\n>> > Maybe this is a bug in your PCRE2 version? Mine is 10.33... and this\n>> > does not happen here... But then, I don't need the `-I` option, and my\n>> > output looks like this:\n>>\n>> -I was just an attempt to workaround the obvious binary files (like\n>> PNG); I'll assume you\n>> should be able to reproduce if using a non JIT enabled PCRE2,\n>> regardless of version.\n>>\n>> my point was that unlike in your report, I didn't have any test cases\n>> failing, because\n>> AFAIK there are no test cases using broken UTF-8 (the ones with binary data are\n>> actually valid zero terminated UTF-8 strings)\n>\n> Thank you for this explanation. I think it makes a total lot of sense.\n>\n> So your motivation for this patch is actually a different one than mine,\n> and I would like to think that this actually strengthens the case _in\n> favor_ of it. The patch kind of kills two birds with one stone.\n\nThis patch is really the wrong thing to do. Don't get me wrong, I'm\nsympathetic to the *problem* and it should be solved, but this isn't the\nsolution.\n\nThe PCRE2_NO_UTF_CHECK flag means \"I have checked that this is a valid\nUTF-8 string so you, PCRE, don't need to re-check it\". To quote\npcre2api(3):\n\n If you know that your pattern is a valid UTF string, and you want to\n skip this check for performance reasons, you can set the\n PCRE2_NO_UTF_CHECK option. When it is set, the effect of passing an\n in‐ valid UTF string as a pattern is undefined. It may cause your\n program to crash or loop.\n\n(Later it's discussed that \"pattern\" here is also \"subject string\" in\nthe context of pcre2_{jit_,}match()).\n\nI know almost nothing about the internals of PCRE's engine, but much of\nit's based on Perl's, which I know way better. Doing the equivalent of\nthis in perl (setting the UTF8 flag on a SV) *will* cause asserts to\nfail and possibly segfaults.\n\nIt's likely through dumb luck that this is \"working\". I.e. yes the JIT\nmode is less anal about these checks, so if you say grep for \"Nguyễn\nThái\" in UTF-8 mode and there's binary data you're satisfied not to find\nanything in that binary data.\n\nBut if you are I'm willing to bet this ruins your day, e.g PCRE would\n\"skip ahead\" a character 4-byte character because it sees a telltale\nU+10000 through U+10FFFF start sequence, except that wasn't a character,\nit was some arbitrary binary.\n\nNow, what is the solution? I don't have any patches yet, but things I\nintend to look at:\n\n 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n reported here. I.e. there's no reason I can think of for why a\n fixed-string pattern should need PCRE2_UTF set when not combined\n with --ignore-case. We can just not do that, but maybe I'm missing\n something there.\n\n 2) We can do \"try utf8, and fallback\". A more advanced version of this\n is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n does. I was thinking something closer to just carrying two compiled\n patterns, and falling back on the ~PCRE2_UTF one if we get a\n PCRE2_ERROR_UTF8_* error.\n\nOne reason we can't \"just\" go back to the pre-ab/no-kwset behavior is\nthat one thing it does is fix a long-standing bug where we'd do the\nwrong thing under locales && -i && UTF-8 string/pattern. More precisely\nwe'd punt it to the C library's matching function, which would probably\ndo the wrong thing.\n"},{"id":"379217","messageId":"xmqqpnlzqh3d.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"87lfwn70nb.fsf@evledraar.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-24T21:04:54Z","receivedAt":"2019-07-24T21:04:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The PCRE2_NO_UTF_CHECK flag means \"I have checked that this is a valid\n> UTF-8 string so you, PCRE, don't need to re-check it\".\n\nOK, in short, barfing and stopping is a problem, but that flag is\nnot the right knob to tweak. And the right knob ...\n\n> 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n> reported here. I.e. there's no reason I can think of for why a\n> fixed-string pattern should need PCRE2_UTF set when not combined\n> with --ignore-case. We can just not do that, but maybe I'm missing\n> something there.\n>\n> 2) We can do \"try utf8, and fallback\". A more advanced version of this\n> is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n> does. I was thinking something closer to just carrying two compiled\n> patterns, and falling back on the ~PCRE2_UTF one if we get a\n> PCRE2_ERROR_UTF8_* error.\n\n... lies somewhere along that line. I think that is very sensible.\nLet's make sure this gets sorted out soonish.\n\nThanks.\n\n"},{"id":"379249","messageId":"nycvar.QRO.7.76.6.1907251143430.21907@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"xmqqpnlzqh3d.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-25T09:48:16Z","receivedAt":"2019-07-25T09:48:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 24 Jul 2019, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n> > The PCRE2_NO_UTF_CHECK flag means \"I have checked that this is a valid\n> > UTF-8 string so you, PCRE, don't need to re-check it\".\n>\n> OK, in short, barfing and stopping is a problem, but that flag is\n> not the right knob to tweak. And the right knob ...\n>\n> > 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n> > reported here. I.e. there's no reason I can think of for why a\n> > fixed-string pattern should need PCRE2_UTF set when not combined\n> > with --ignore-case. We can just not do that, but maybe I'm missing\n> > something there.\n> >\n> > 2) We can do \"try utf8, and fallback\". A more advanced version of this\n> > is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n> > does. I was thinking something closer to just carrying two compiled\n> > patterns, and falling back on the ~PCRE2_UTF one if we get a\n> > PCRE2_ERROR_UTF8_* error.\n>\n> ... lies somewhere along that line. I think that is very sensible.\n\nI am glad that everybody agrees with my original comment on ab/no-kwset\nwhere I suggested that we should use our knowledge of the encoding of\nthe haystack and convert it to UTF-8 if we detect that the pattern is\nUTF-8 encoded, and then pass the PCRE2_UTF flag only when applicable\n(i.e. when we know that either needle or haystack is non-ASCII, and then\nmaking sure that we convert to UTF-8 whenever necessary).\n\nOkay, that came over a bit more sarcastic than I originally intended,\nbut if you try to filter that out, I think that is still the better\nsolution than to paper over the issue.\n\nAfter all, PCRE2_MATCH_INVALID_UTF is only marginally better than what\nCarlo suggested. _Marginally_. Not really worth considering, in my mind,\neven.\n\n> Let's make sure this gets sorted out soonish.\n\nI agree,\nDscho\n"},{"id":"379272","messageId":"xmqqlfwmqnb5.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907251143430.21907@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-25T13:02:54Z","receivedAt":"2019-07-25T13:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> OK, in short, barfing and stopping is a problem, but that flag is\n>> not the right knob to tweak. And the right knob ...\n>>\n>> > 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n>> > reported here. I.e. there's no reason I can think of for why a\n>> > fixed-string pattern should need PCRE2_UTF set when not combined\n>> > with --ignore-case. We can just not do that, but maybe I'm missing\n>> > something there.\n>> >\n>> > 2) We can do \"try utf8, and fallback\". A more advanced version of this\n>> > is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n>> > does. I was thinking something closer to just carrying two compiled\n>> > patterns, and falling back on the ~PCRE2_UTF one if we get a\n>> > PCRE2_ERROR_UTF8_* error.\n>>\n>> ... lies somewhere along that line. I think that is very sensible.\n>\n> I am glad that everybody agrees with my original comment on ab/no-kwset\n> where I suggested that we should use our knowledge of the encoding of\n> the haystack and convert it to UTF-8 if we detect that the pattern is\n> UTF-8 encoded,...\n\nPlease do not count me among \"everybody\", then. I did not think\nthat Ævar meant to iconv the haystack when I wrote the message you\nare responding to, but if that was what he meant, I would not have\nsaid \"very sensible\".\n\n"},{"id":"379305","messageId":"nycvar.QRO.7.76.6.1907252021230.21907@tvgsbejvaqbjf.bet","threadId":"51505","inReplyTo":"xmqqlfwmqnb5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-07-25T18:22:09Z","receivedAt":"2019-07-25T18:22:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Thu, 25 Jul 2019, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> >> OK, in short, barfing and stopping is a problem, but that flag is\n> >> not the right knob to tweak. And the right knob ...\n> >>\n> >> > 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n> >> > reported here. I.e. there's no reason I can think of for why a\n> >> > fixed-string pattern should need PCRE2_UTF set when not combined\n> >> > with --ignore-case. We can just not do that, but maybe I'm missing\n> >> > something there.\n> >> >\n> >> > 2) We can do \"try utf8, and fallback\". A more advanced version of this\n> >> > is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n> >> > does. I was thinking something closer to just carrying two compiled\n> >> > patterns, and falling back on the ~PCRE2_UTF one if we get a\n> >> > PCRE2_ERROR_UTF8_* error.\n> >>\n> >> ... lies somewhere along that line. I think that is very sensible.\n> >\n> > I am glad that everybody agrees with my original comment on ab/no-kwset\n> > where I suggested that we should use our knowledge of the encoding of\n> > the haystack and convert it to UTF-8 if we detect that the pattern is\n> > UTF-8 encoded,...\n>\n> Please do not count me among \"everybody\", then. I did not think\n> that Ævar meant to iconv the haystack when I wrote the message you\n> are responding to, but if that was what he meant, I would not have\n> said \"very sensible\".\n\nOkay, but in that case I cannot agree with your assessment that it is\nvery sensible.\n\nIf we're already deciding to paper over things, I'd much rather prefer\nthe simpler patch, i.e. Carlo's.\n\nCiao,\nDscho\n"},{"id":"379354","messageId":"87ef2c7roy.fsf@evledraar.gmail.com","threadId":"51505","inReplyTo":"nycvar.QRO.7.76.6.1907252021230.21907@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-26T15:15:25Z","receivedAt":"2019-07-26T15:15:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 25 2019, Johannes Schindelin wrote:\n\n> Hi Junio,\n>\n> On Thu, 25 Jul 2019, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>>\n>> >> OK, in short, barfing and stopping is a problem, but that flag is\n>> >> not the right knob to tweak. And the right knob ...\n>> >>\n>> >> > 1) We're oversupplying PCRE2_UTF now, and one such case is what's being\n>> >> > reported here. I.e. there's no reason I can think of for why a\n>> >> > fixed-string pattern should need PCRE2_UTF set when not combined\n>> >> > with --ignore-case. We can just not do that, but maybe I'm missing\n>> >> > something there.\n>> >> >\n>> >> > 2) We can do \"try utf8, and fallback\". A more advanced version of this\n>> >> > is what the new PCRE2_MATCH_INVALID_UTF flag (mentioned upthread)\n>> >> > does. I was thinking something closer to just carrying two compiled\n>> >> > patterns, and falling back on the ~PCRE2_UTF one if we get a\n>> >> > PCRE2_ERROR_UTF8_* error.\n>> >>\n>> >> ... lies somewhere along that line. I think that is very sensible.\n>> >\n>> > I am glad that everybody agrees with my original comment on ab/no-kwset\n>> > where I suggested that we should use our knowledge of the encoding of\n>> > the haystack and convert it to UTF-8 if we detect that the pattern is\n>> > UTF-8 encoded,...\n>>\n>> Please do not count me among \"everybody\", then. I did not think\n>> that Ævar meant to iconv the haystack when I wrote the message you\n>> are responding to, but if that was what he meant, I would not have\n>> said \"very sensible\".\n>\n> Okay, but in that case I cannot agree with your assessment that it is\n> very sensible.\n\nFWIW what I meant was not that we'd run around and iconv() things, it\nwouldn't make much sense to e.g. iconv() some PNG data to be \"UTF-8\nvalid\", which presumably would be the end result of something like that.\n\nRather that this model of assuming that a UTF-8 pattern means we can\nconsider everything in the repo UTF-8 in git-grep doesn't make sense. My\nkwset patches *revealed* that problem in a painful way, but it was there\nalready.\n\nI'm not sure what a real fix for that is. Part of it is probably 8/8 in\nthe series I mention below, but more generally we'd need to be more\nencoding aware at a much higher callsite than \"grep\". So e.g. we'd know\nthat we match \"binary\" data as not-UTF-8. Now we just throw arbitrary\nbytes around and hope something sticks.\n\n> If we're already deciding to paper over things, I'd much rather prefer\n> the simpler patch, i.e. Carlo's.\n\nAs I noted upthread PCRE's own docs promise undefined behavior and fire\nand brimstone if that patch is applied. Those last two not\nguaranteed. So we need another solution.\n\nI've submitted\nhttps://public-inbox.org/git/20190726150818.6373-1-avarab@gmail.com/\njust now. See what you think about it.\n"},{"id":"379377","messageId":"CAPUEspgseYvKTNN6EkqjKo1zQXRjUzyiLMe0kJwnNu=F3ATmOA@mail.gmail.com","threadId":"51505","inReplyTo":"87ef2c7roy.fsf@evledraar.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-07-26T15:53:49Z","receivedAt":"2019-07-26T15:54:03Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Jul 26, 2019 at 8:15 AM Ævar Arnfjörð Bjarmason\n wrote:\n> I'm not sure what a real fix for that is. Part of it is probably 8/8 in\n> the series I mention below, but more generally we'd need to be more\n> encoding aware at a much higher callsite than \"grep\". So e.g. we'd know\n> that we match \"binary\" data as not-UTF-8. Now we just throw arbitrary\n> bytes around and hope something sticks.\n\nI haven't look yet at your proposed changes, but my gut feeling is that\nthe work to support invalid UTF in the yet unreleased PCRE version would\nbe needed as part of it, and therefore it might be better to keep PCRE\nout of the main path until that gets released and can be relied upon.\n\nkwset is not going away with this series anyway, regardless of the no-kwset\nname on the branch.\n\n> > If we're already deciding to paper over things, I'd much rather prefer\n> > the simpler patch, i.e. Carlo's.\n>\n> As I noted upthread PCRE's own docs promise undefined behavior and fire\n> and brimstone if that patch is applied. Those last two not\n> guaranteed. So we need another solution.\n\nin my original reply I mentioned I explicitly didn't do a test because of this\n\"undefined behavior\", but I think it should be fair to mention that we are\nalready affected by that because using the JIT fast path does skip any\nUTF-8 validations and is currently possible to get git into an infinite loop\nor make it segfault when using PCRE.\n\nin that line, I am not sure I understand the pushback against making that\nexplicit since it only makes both codepaths behave the same (bugs and\nrisks of burning alike)\n\nCarlo\n"},{"id":"379378","messageId":"xmqqy30kojj1.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"87ef2c7roy.fsf@evledraar.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-26T16:19:46Z","receivedAt":"2019-07-26T16:19:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> FWIW what I meant was not that we'd run around and iconv() things, it\n> wouldn't make much sense to e.g. iconv() some PNG data to be \"UTF-8\n> valid\", which presumably would be the end result of something like that.\n>\n> Rather that this model of assuming that a UTF-8 pattern means we can\n> consider everything in the repo UTF-8 in git-grep doesn't make sense. My\n> kwset patches *revealed* that problem in a painful way, but it was there\n> already.\n\nWe already do assume that pathnames are UTF-8 (pathspecs on MacOS\nare converted and then they are matched assuming that property).\nFurther, with the same mechanism, I think there is an assumption\nthat anything that comes from the command line is UTF-8 (and if I\nrecall correctly, doesn't the Windows port of Git force us to use\nthe same assumption---I recall we needed tests tweak for that).\n\nIn the very very longer term, I do not think we would want to keep\nthe assumption that the text encoding of blobs is always UTF-8, and\nit would be nice to extend the system, so that blob data could be\nmarked in some way to say \"I'm in Big-5, and not in UTF-8, so please\ntreat me as such\" and magically the needle and the haystack can be\nmade to agree, with iconv() either one of them. \n\nBut I do not think the current topic to fix the immediate/imminent\nbreakage should not be distracted by that. Let's keep assuming that\nany blob, when it is text, is UTF-8.\n\nAnd from that point of view, I think the two pieces of idea in your\nearlier message does make sense. We can try to match as binary most\nof the time, as UTF-8 would not let a valid UTF-8 needle match in\nthe haystack starting in the middle of a character. When the user\nis trying to match case-insensitively, we know the haystack in which\nthe user is interested in finding the needle is text, even though\nthere may be non-text blobs as well.\n\nFor example, \"git grep -i 'foo' t/\" may find a few png files under\nthe t/ directory. We do not care if they happen to contain Foo and\nwe do not mind if they appear or do not appear in the result. The\nonly two things we care about are (1) foo, Foo, FOO are found in the\ntext files under t/ and (2) the command does not die in the middle,\nbefore processing all the files, only because a png file it found\nwere not UTF-8 valid.\n"},{"id":"379387","messageId":"87d0hw7ffv.fsf@evledraar.gmail.com","threadId":"51505","inReplyTo":"xmqqy30kojj1.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-26T19:40:04Z","receivedAt":"2019-07-26T19:40:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 26 2019, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> FWIW what I meant was not that we'd run around and iconv() things, it\n>> wouldn't make much sense to e.g. iconv() some PNG data to be \"UTF-8\n>> valid\", which presumably would be the end result of something like that.\n>>\n>> Rather that this model of assuming that a UTF-8 pattern means we can\n>> consider everything in the repo UTF-8 in git-grep doesn't make sense. My\n>> kwset patches *revealed* that problem in a painful way, but it was there\n>> already.\n>\n> We already do assume that pathnames are UTF-8 (pathspecs on MacOS\n> are converted and then they are matched assuming that property).\n> Further, with the same mechanism, I think there is an assumption\n> that anything that comes from the command line is UTF-8 (and if I\n> recall correctly, doesn't the Windows port of Git force us to use\n> the same assumption---I recall we needed tests tweak for that).\n>\n> In the very very longer term, I do not think we would want to keep\n> the assumption that the text encoding of blobs is always UTF-8, and\n> it would be nice to extend the system, so that blob data could be\n> marked in some way to say \"I'm in Big-5, and not in UTF-8, so please\n> treat me as such\" and magically the needle and the haystack can be\n> made to agree, with iconv() either one of them.\n>\n> But I do not think the current topic to fix the immediate/imminent\n> breakage should not be distracted by that. Let's keep assuming that\n> any blob, when it is text, is UTF-8.\n>\n> And from that point of view, I think the two pieces of idea in your\n> earlier message does make sense. We can try to match as binary most\n> of the time, as UTF-8 would not let a valid UTF-8 needle match in\n> the haystack starting in the middle of a character.\n\n*nod*\n\n> When the user is trying to match case-insensitively, we know the\n> haystack in which the user is interested in finding the needle is\n> text, even though there may be non-text blobs as well.\n>\n> For example, \"git grep -i 'foo' t/\" may find a few png files under\n> the t/ directory. We do not care if they happen to contain Foo and\n> we do not mind if they appear or do not appear in the result. The\n> only two things we care about are (1) foo, Foo, FOO are found in the\n> text files under t/ and (2) the command does not die in the middle,\n> before processing all the files, only because a png file it found\n> were not UTF-8 valid.\n\nI think this part's a step too far, and not how e.g. GNU grep\nworks. Peeking into binary data in a text grep is what people expect,\ne.g. because you might want to recursively grep mixed text/mp3s for an\nauthor. The text part of the mp3s means that metadata will be grepped\nfor inside the binary files.\n\nGetting that right is hard around the edges though...\n"},{"id":"379390","messageId":"87blxg7e9g.fsf@evledraar.gmail.com","threadId":"51505","inReplyTo":"CAPUEspgseYvKTNN6EkqjKo1zQXRjUzyiLMe0kJwnNu=F3ATmOA@mail.gmail.com","subject":"Re: [PATCH] grep: skip UTF8 checks explicitally","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-07-26T20:05:31Z","receivedAt":"2019-07-26T20:05:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 26 2019, Carlo Arenas wrote:\n\n> On Fri, Jul 26, 2019 at 8:15 AM Ævar Arnfjörð Bjarmason\n> wrote:\n>> I'm not sure what a real fix for that is. Part of it is probably 8/8 in\n>> the series I mention below, but more generally we'd need to be more\n>> encoding aware at a much higher callsite than \"grep\". So e.g. we'd know\n>> that we match \"binary\" data as not-UTF-8. Now we just throw arbitrary\n>> bytes around and hope something sticks.\n>\n> I haven't look yet at your proposed changes, but my gut feeling is that\n> the work to support invalid UTF in the yet unreleased PCRE version would\n> be needed as part of it, and therefore it might be better to keep PCRE\n> out of the main path until that gets released and can be relied upon.\n\nI'm hoping my 8-part series is good enough to move it forward, but as\n48de2a768c (\"grep: remove the kwset optimization\", .2019-07-01) shows we\ncan always just fall back on using regcomp instead.\n\n> kwset is not going away with this series anyway, regardless of the no-kwset\n> name on the branch.\n\nThe larger context here is that this is the 1st step of a 2-step series\nto get rid of kwset. If I can pull that off successfully is another\nmatter, but that's the plan. After it's applied we just use it in the\npickaxe code, and it's relatively straightforward to convert that. See:\nhttps://public-inbox.org/git/87v9x793qi.fsf@evledraar.gmail.com/\n\n>> > If we're already deciding to paper over things, I'd much rather prefer\n>> > the simpler patch, i.e. Carlo's.\n>>\n>> As I noted upthread PCRE's own docs promise undefined behavior and fire\n>> and brimstone if that patch is applied. Those last two not\n>> guaranteed. So we need another solution.\n>\n> in my original reply I mentioned I explicitly didn't do a test because of this\n> \"undefined behavior\", but I think it should be fair to mention that we are\n> already affected by that because using the JIT fast path does skip any\n> UTF-8 validations and is currently possible to get git into an infinite loop\n> or make it segfault when using PCRE.\n\nRight, this is a good point that we should take notice of. I.e. this is\n*not* a new bug per-se, you can do this on master and get a UTF-8 bug\nfrom git.git:\n\n git grep -P '(*NO_JIT)[æ]'\n\n> in that line, I am not sure I understand the pushback against making that\n> explicit since it only makes both codepaths behave the same (bugs and\n> risks of burning alike)\n\nBecause with my kwset series we're getting a lot more users of this\nuntil-now obscure code, so we're finding old-but-new-to-us bugs.\n\nWe've had this bug dating all the way back to Duy's 18547aacf5\n(\"grep/pcre: support utf-8\", 2016-06-25). It was first released with git\n2.10.\n\nSo why are we getting list discussion about it *now*? Because my kwset\nseries got merged to \"next\", and we apparently have a lot of users who'd\nuse fixed-string git-grep under locales, but never used PCRE via -P\nexplicitly before.\n\nSo it's worth getting the semantics right. As noted in the E-Mail I\nlinked to earlier my ulterior motive here is to get to a point where\nwe'll funnel all regex matching through PCRE implicitly if it's\navailable.\n\nWe need to get these UTF-8 edge cases right. I don't know if my recent\n8-part series gets us 100% there, but hopefully it at least gets us\ncloser to it.\n"},{"id":"381441","messageId":"20190828145444.31588-1-carenas@gmail.com","threadId":"51505","inReplyTo":"xmqq5znrs9d0.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] grep: skip UTF8 checks explicitly","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2019-08-28T14:54:44Z","receivedAt":"2019-08-28T14:54:50Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"18547aacf5 (\"grep/pcre: support utf-8\", 2016-06-25) that was released\nwith git 2.10 added the PCRE_UTF8 flag to PCRE1 matching including a\ncall to has_non_ascii() to try to avoid breakage if there was non-utf8\nencoded content in the haystack.\n\nUsually PCRE is compiled with JIT support (even if is not the default),\nand therefore the codepath used includes calling pcre_jit_exec, which\nskips UTF-8 validation by design (which might result in crashes or hangs)\nbut when JIT support wasn't compiled we use pcre_exec instead with the\nposibility that grep might be aborted if invalid UTF-8 is found in the\nhaystack.\n\nPCRE1 provides a flag since Mar 5, 2007 that could be used to skip the\nchecks explicitly so use that to make both codepaths equivalent (the\nflag is ignored by pcre1_jit_exec)\n\nthis fix is only implemented for PCRE1 because PCRE2 is likely to have\na better solution (without the risks) instead in the future\n\nHelped-by: Johannes Schindelin \nHelped-by: Eric Sunshine \nHelped-by: Ævar Arnfjörð Bjarmason \nSuggested-by: Junio C Hamano \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nV2:\n* drop PCRE2 support\n* add backward compatibility define\n\n grep.c | 4 ++--\n grep.h | 3 +++\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f7c3a5803e..69ef69516e 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -421,7 +421,7 @@ static void compile_pcre1_regexp(struct grep_pat *p, const struct grep_opt *opt)\n static int pcre1match(struct grep_pat *p, const char *line, const char *eol,\n \t\tregmatch_t *match, int eflags)\n {\n-\tint ovector[30], ret, flags = 0;\n+\tint ovector[30], ret, flags = PCRE_NO_UTF8_CHECK;\n \n \tif (eflags & REG_NOTBOL)\n \t\tflags |= PCRE_NOTBOL;\ndiff --git a/grep.h b/grep.h\nindex 1875880f37..9c8797a017 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -3,6 +3,9 @@\n #include \"color.h\"\n #ifdef USE_LIBPCRE1\n #include \n+#ifndef PCRE_NO_UTF8_CHECK\n+#define PCRE_NO_UTF8_CHECK 0\n+#endif\n #ifdef PCRE_CONFIG_JIT\n #if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32\n #ifndef NO_LIBPCRE1_JIT\n-- \n2.23.0\n"},{"id":"381460","messageId":"CAPUEsphatmoBg3jV9p_xLF_zYe0qn=S=5UTYBCHKjnuNZm4_VA@mail.gmail.com","threadId":"51505","inReplyTo":"20190828145444.31588-1-carenas@gmail.com","subject":"Re: [PATCH v2] grep: skip UTF8 checks explicitly","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-08-28T16:57:39Z","receivedAt":"2019-08-28T16:57:55Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"FWIW, the changes in grep.h are IMHO optional and hadn't been really\ntested as I couldn't find a version of PCRE1 old enough to trigger it\nbut I am hoping are simple enough to work.\n\nAs in my original proposal, there is no test because this is going to\n(as documented) trigger undefined behaviour and so I don't see how we\ncan reliably test that. Goes without saying tthough, that the same is\ntriggered when JIT is enabled and the JIT fast path is being used, so\nthis is not a regression and will be more visible once\nab/pcre-jit-fixes gets merged because we are moving out of the JIT\nfast path with a patch[0] in that series\n\nWhile Ævar made a point[1] that this wasn't a solution to the problem,\nit was because PCRE2 could have a better one (still missing but based\non ab/pcre-jit-fixes), and it was expected that PCRE2 will be used a\nlot more and so it wasn't a good idea for it (something that doesn't\napply to PCRE1)\n\nthe patch was based on maint (like all bugfixes) but applies cleanly\nto master and next, it will conflict with pu but for easy of merge I'd\napplied it on top of cb/pcre1-cleanup and make it available in\nGitHub[2]; that branch could be used as well as a reroll for that\ntopic (if that is preferred)\n\nthe error message hasn't been changed on this patch, as it might make\nsense to improve it as well for PCRE2, but at least shouldn't be\ntriggered anymore (ex, from running a freshly built git without the\npatch and linked against a non JIT enabled PCRE1):\n\n$ ./git-grep -P 'Nguyễn Thái.Ngọc'\n.mailmap:Nguyễn Thái Ngọc Duy \nfatal: pcre_exec failed with error code -10\n\nCarlo\n\n[0] 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26)\n[1] https://public-inbox.org/git/87lfwn70nb.fsf@evledraar.gmail.com/\n[2] https://github.com/carenas/git/tree/pcre1-cleanup\n"},{"id":"382067","messageId":"CAPUEspi+OXR8pjcJgTdqCCGMxE0dv8L6y-0SaX8jzB6z9mXaRw@mail.gmail.com","threadId":"51505","inReplyTo":"CAPUEsphatmoBg3jV9p_xLF_zYe0qn=S=5UTYBCHKjnuNZm4_VA@mail.gmail.com","subject":"Re: [PATCH v2] grep: skip UTF8 checks explicitly","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2019-09-09T15:07:29Z","receivedAt":"2019-09-09T15:07:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"ping\n\nany feedback on code/approach highly appreciated\n\nCarlo\n\nOn Wed, Aug 28, 2019 at 9:57 AM Carlo Arenas wrote:\n>\n> FWIW, the changes in grep.h are IMHO optional and hadn't been really\n> tested as I couldn't find a version of PCRE1 old enough to trigger it\n> but I am hoping are simple enough to work.\n>\n> As in my original proposal, there is no test because this is going to\n> (as documented) trigger undefined behaviour and so I don't see how we\n> can reliably test that. Goes without saying tthough, that the same is\n> triggered when JIT is enabled and the JIT fast path is being used, so\n> this is not a regression and will be more visible once\n> ab/pcre-jit-fixes gets merged because we are moving out of the JIT\n> fast path with a patch[0] in that series\n>\n> While Ævar made a point[1] that this wasn't a solution to the problem,\n> it was because PCRE2 could have a better one (still missing but based\n> on ab/pcre-jit-fixes), and it was expected that PCRE2 will be used a\n> lot more and so it wasn't a good idea for it (something that doesn't\n> apply to PCRE1)\n>\n> the patch was based on maint (like all bugfixes) but applies cleanly\n> to master and next, it will conflict with pu but for easy of merge I'd\n> applied it on top of cb/pcre1-cleanup and make it available in\n> GitHub[2]; that branch could be used as well as a reroll for that\n> topic (if that is preferred)\n>\n> the error message hasn't been changed on this patch, as it might make\n> sense to improve it as well for PCRE2, but at least shouldn't be\n> triggered anymore (ex, from running a freshly built git without the\n> patch and linked against a non JIT enabled PCRE1):\n>\n> $ ./git-grep -P 'Nguyễn Thái.Ngọc'\n> .mailmap:Nguyễn Thái Ngọc Duy \n> fatal: pcre_exec failed with error code -10\n>\n> Carlo\n>\n> [0] 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26)\n> [1] https://public-inbox.org/git/87lfwn70nb.fsf@evledraar.gmail.com/\n> [2] https://github.com/carenas/git/tree/pcre1-cleanup\n"},{"id":"382087","messageId":"xmqq5zm1joec.fsf@gitster-ct.c.googlers.com","threadId":"51505","inReplyTo":"CAPUEspi+OXR8pjcJgTdqCCGMxE0dv8L6y-0SaX8jzB6z9mXaRw@mail.gmail.com","subject":"Re: [PATCH v2] grep: skip UTF8 checks explicitly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T18:49:31Z","receivedAt":"2019-09-09T18:49:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Arenas writes:\n\n> ping\n>\n> any feedback on code/approach highly appreciated\n\nI'd prefer to see others weigh in before starting to merge the pcre\nstuff to 'next'.\n\nI do not mind taking this updated one that limits the scope to pcre1\nas a pure regressino fix, if others agree. Thanks for keeping a tab\non these pcre issues.\n\n>> While Ævar made a point[1] that this wasn't a solution to the problem,\n>> it was because PCRE2 could have a better one (still missing but based\n>> on ab/pcre-jit-fixes), and it was expected that PCRE2 will be used a\n>> lot more and so it wasn't a good idea for it (something that doesn't\n>> apply to PCRE1)\n>>\n>> the patch was based on maint (like all bugfixes) but applies cleanly\n>> to master and next, it will conflict with pu but for easy of merge I'd\n>> applied it on top of cb/pcre1-cleanup and make it available in\n>> GitHub[2]; that branch could be used as well as a reroll for that\n>> topic (if that is preferred)\n>>\n>> the error message hasn't been changed on this patch, as it might make\n>> sense to improve it as well for PCRE2, but at least shouldn't be\n>> triggered anymore (ex, from running a freshly built git without the\n>> patch and linked against a non JIT enabled PCRE1):\n>>\n>> $ ./git-grep -P 'Nguyễn Thái.Ngọc'\n>> .mailmap:Nguyễn Thái Ngọc Duy \n>> fatal: pcre_exec failed with error code -10\n>>\n>> Carlo\n>>\n>> [0] 685668faaa (grep: stop using a custom JIT stack with PCRE v1, 2019-07-26)\n>> [1] https://public-inbox.org/git/87lfwn70nb.fsf@evledraar.gmail.com/\n>> [2] https://github.com/carenas/git/tree/pcre1-cleanup\n"}]}