{"thread":{"id":"59048","subject":"[PATCH] grep: correctly identify utf-8 characters with \\{b,w} in -P","startedAt":"2023-01-08T06:26:16Z","lastAt":"2023-01-18T23:24:38Z","messageCount":17,"participants":["Carlo Marcelo Arenas Belón","Junio C Hamano","Ævar Arnfjörð Bjarmason","Paul Eggert","Carlo Arenas"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"469903","messageId":"20230108062335.72114-1-carenas@gmail.com","threadId":"59048","inReplyTo":null,"subject":"[PATCH] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-08T06:23:35Z","receivedAt":"2023-01-08T06:26:16Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When UTF is enabled for a PCRE match, the corresponding flags are\nadded to the pcre2_compile() call, but PCRE2_UCP wasn't included.\n\nThis prevents extending the meaning of the character classes to\ninclude those new valid characters and therefore result in failed\nmatches for expressions that rely on that extention, for ex:\n\n  $ git grep -P '\\bÆvar'\n\nAdd PCRE2_UCP so that \\w will include Æ and therefore \\b could\ncorrectly match the beginning of that word.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/grep.c b/grep.c\nindex 06eed69493..1687f65b64 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\toptions |= PCRE2_CASELESS;\n \t}\n \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n-\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n+\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n \n #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\n-- \n2.37.1 (Apple Git-137.1)\n\n"},{"id":"469905","messageId":"xmqqbkn9zg0j.fsf@gitster.g","threadId":"59048","inReplyTo":"20230108062335.72114-1-carenas@gmail.com","subject":"Re: [PATCH] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-08T06:39:40Z","receivedAt":"2023-01-08T06:39:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón  <carenas@gmail.com> writes:\n\n> When UTF is enabled for a PCRE match, the corresponding flags are\n> added to the pcre2_compile() call, but PCRE2_UCP wasn't included.\n\nWould the same performance concern as\n\nhttps://discourse.julialang.org/t/regex-pcre2-and-the-pcre2-ucp-ucp-flag/10930\n\napply to us as well?\n\n\n\n>  \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n> -\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> +\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n>  \n>  #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n>  \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\n"},{"id":"469922","messageId":"20230108155217.2817-1-carenas@gmail.com","threadId":"59048","inReplyTo":"20230108062335.72114-1-carenas@gmail.com","subject":"[PATCH v2] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-08T15:52:17Z","receivedAt":"2023-01-08T15:54:08Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When UTF is enabled for a PCRE match, the corresponding flags are\nadded to the pcre2_compile() call, but PCRE2_UCP wasn't included.\n\nThis prevents extending the meaning of the character classes to\ninclude those new valid characters and therefore result in failed\nmatches for expressions that rely on that extention, for ex:\n\n  $ git grep -P '\\bÆvar'\n\nAdd PCRE2_UCP so that \\w will include Æ and therefore \\b could\ncorrectly match the beginning of that word.\n\nThis has an impact on performance that has been estimated to be\nbetween 20% to 40% and that is shown through the added performance\ntest.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n grep.c                              |  2 +-\n t/perf/p7822-grep-perl-character.sh | 42 +++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n create mode 100755 t/perf/p7822-grep-perl-character.sh\n\ndiff --git a/grep.c b/grep.c\nindex 06eed69493..1687f65b64 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\toptions |= PCRE2_CASELESS;\n \t}\n \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n-\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n+\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n \n #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\ndiff --git a/t/perf/p7822-grep-perl-character.sh b/t/perf/p7822-grep-perl-character.sh\nnew file mode 100755\nindex 0000000000..87009c60df\n--- /dev/null\n+++ b/t/perf/p7822-grep-perl-character.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description=\"git-grep's perl regex\n+\n+If GIT_PERF_GREP_THREADS is set to a list of threads (e.g. '1 4 8'\n+etc.) we will test the patterns under those numbers of threads.\n+\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+if test -n \"$GIT_PERF_GREP_THREADS\"\n+then\n+\ttest_set_prereq PERF_GREP_ENGINES_THREADS\n+fi\n+\n+for pattern in \\\n+\t'\\\\bhow' \\\n+\t'\\\\bÆvar' \\\n+\t'\\\\d+ \\\\bÆvar' \\\n+\t'\\\\bBelón\\\\b' \\\n+\t'\\\\w{12}\\\\b'\n+do\n+\techo '$pattern' >pat\n+\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n+\tthen\n+\t\ttest_perf \"grep -P '$pattern'\" --prereq PCRE \"\n+\t\t\tgit -P grep -f pat || :\n+\t\t\"\n+\telse\n+\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\tdo\n+\t\t\ttest_perf \"grep -P '$pattern' with $threads threads\" --prereq PTHREADS,PCRE \"\n+\t\t\t\tgit -c grep.threads=$threads -P grep -f pat || :\n+\t\t\t\"\n+\t\tdone\n+\tfi\n+done\n+\n+test_done\n-- \n2.39.0.199.g555ddd67e6\n\n"},{"id":"469966","messageId":"230109.86v8lf297g.gmgdl@evledraar.gmail.com","threadId":"59048","inReplyTo":"20230108155217.2817-1-carenas@gmail.com","subject":"Re: [PATCH v2] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-09T11:35:05Z","receivedAt":"2023-01-09T12:18:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Jan 08 2023, Carlo Marcelo Arenas Belón wrote:\n\n> When UTF is enabled for a PCRE match, the corresponding flags are\n> added to the pcre2_compile() call, but PCRE2_UCP wasn't included.\n>\n> This prevents extending the meaning of the character classes to\n> include those new valid characters and therefore result in failed\n> matches for expressions that rely on that extention, for ex:\n>\n>   $ git grep -P '\\bÆvar'\n>\n> Add PCRE2_UCP so that \\w will include Æ and therefore \\b could\n> correctly match the beginning of that word.\n>\n> This has an impact on performance that has been estimated to be\n> between 20% to 40% and that is shown through the added performance\n> test.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  grep.c                              |  2 +-\n>  t/perf/p7822-grep-perl-character.sh | 42 +++++++++++++++++++++++++++++\n>  2 files changed, 43 insertions(+), 1 deletion(-)\n>  create mode 100755 t/perf/p7822-grep-perl-character.sh\n>\n> diff --git a/grep.c b/grep.c\n> index 06eed69493..1687f65b64 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \t\toptions |= PCRE2_CASELESS;\n>  \t}\n>  \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n> -\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> +\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n\nI have a definite bias towards liking this change, it would help my find\nmyself :)\n\nBut I don't think it's safe to change the default behavior \"git-grep\",\nit's not a mere bug fix, but a major behavior change for existing users\nof grep.patternType=perl. E.g. on git.git:\n\t\n\t$ diff <(git -P grep -P '\\d+') <(git -P grep -P '(*UCP)\\d')\n\t53360a53361,53362\n\t> git-gui/po/ja.po:\"- 第１行: 何をしたか、を１行で要約。\\n\"\n\t> git-gui/po/ja.po:\"- 第２行: 空白\\n\"\n\nSo, it will help \"do the right thing\" on e.g. \"\\bÆ\", but it will also\nfind e.g. CJK numeric characters for \\d etc.\n\nI see per the discussion on\nhttps://github.com/PCRE2Project/pcre2/issues/185 and\nhttps://lists.gnu.org/archive/html/bug-grep/2023-01/threads.html that\nyou submitted similar fixes to GNU grep & PCRE itself.\n\nI see that GNU grep integrated it a couple of days ago as\nhttps://git.savannah.gnu.org/cgit/grep.git/commit/?id=5e3b760f65f13856e5717e5b9d935f5b4a615be3\n\nAs most discussions about PCRE will eventually devolve into \"what does\nPerl do?\": \"Perl\" itself will promiscuously use this behavior by\ndefault.\n\nE.g. here the same \"１\" character (not the ASCII digit \"1\") will be\nmatched from the command-line:\n\n\t$ perl -Mre=debug -CA -wE 'shift =~ /\\d/' \"１\"\n\tCompiling REx \"\\d\"\n\tFinal program:\n\t   1: POSIXU[\\d] (2)\n\t   2: END (0)\n\tstclass POSIXU[\\d] minlen 1\n\tMatching REx \"\\d\" against \"%x{ff11}\"\n\tUTF-8 string...\n\tMatching stclass POSIXU[\\d] against \"%x{ff11}\" (3 bytes)\n\t   0 <> <%x{ff11}>           |   0| 1:POSIXU[\\d](2)\n\t   3 <%x{ff11}> <>           |   0| 2:END(0)\n\tMatch successful!\n\tFreeing REx: \"\\d\"\n\nBut I don't think it makes sense for \"git grep\" (or GNU \"grep\") to\nfollow Perl in this particular case.\n\nFor those not familiar with its Unicode model it doesn't assume by\ndefault that strings are Unicode, they have to be explicitly marked as\nsuch. in the above example I'm declaring that all of \"argv\" is UTF-8\n(via the \"-CA\" flag).\n\nIf I didn't supply that flag the string wouldn't have the UTF-8 flag,\nand wouldn't match, as the Perl regex engine won't use Unicode semantics\nexcept on Unicode target strings.\n\nEven for Perl, this behavior has been troublesome. Opinions differ, but\nI think many would agree (and I've CC'd the main authority on Perl's\nregex engine) that doing this by default was *probably* a mistake.\n\nYou almost never want \"everything Unicode considers a digit\", and if you\ndo using e.g. \\p{Nd} instead of \\d would be better in terms of\nexpressing your intent. I see you're running into this on the PCRE\ntracker, where you're suggesting that the equivalent of /a (or /aa)\nwould be needed.\n\n\thttps://github.com/PCRE2Project/pcre2/issues/185#issuecomment-1374796393\n\n\nWhich brings me home to the seeming digression about \"Perl\"\nabove.\n\nUnlike a programming language where you'll typically \"mark\" your data as\nit comes in, natural text as UTF-8, binary data as such etc., a \"grep\"\nutility has to operate on more of an \"all or nothing\" basis (except in\nthe case of \"-a\"). I.e. we're usually searching through unknown data.\n\nEnabling this by default means that we'll pick up characters most people\nprobably wouldn't expect, particularly from near-binary data formats\n(those that won't require \"-a\", but contain non-Unicode non-ASCII\nsequences).\n\nI don't have some completely holistic view of what we should do in every\ncase, e.g. we turned on PCRE2_UTF so that things like \"-i\" would Just\nWork, but even case-insensitivity has its own unexpected edge cases in\nUnicode.\n\nBut I don't think those edge cases are nearly as common as those we'd\nrun into by enabling PCRE2_UCP. Rather than trying to opt-out with \"/a\"\nor \"/aa\" I think this should be opt-in.\n\nAs the example at the start shows you can already do this with \"(*UCP)\"\nin the pattern, so perhaps we should just link to the pcre2pattern(3)\nmanual from git-grep(1)?\n"},{"id":"469997","messageId":"d6814350-10a3-55c0-68da-7e691976cd45@cs.ucla.edu","threadId":"59048","inReplyTo":"230109.86v8lf297g.gmgdl@evledraar.gmail.com","subject":"Re: bug#60690: [PATCH v2] grep: correctly identify utf-8 characters with \\{b, w} in -P","fromName":"Paul Eggert","fromEmail":"eggert@cs.ucla.edu","sentAt":"2023-01-09T18:40:16Z","receivedAt":"2023-01-09T18:45:57Z","isPatch":true,"sender":{"key":"eggert@cs.ucla.edu","avatar":"https://avatars.githubusercontent.com/u/572024?v=4"},"body":"On 1/9/23 03:35, Ævar Arnfjörð Bjarmason wrote:\n\n> You almost never want \"everything Unicode considers a digit\", and if you\n> do using e.g. \\p{Nd} instead of \\d would be better in terms of\n> expressing your intent.\n\nFor GNU grep, PCRE2_UCP is needed because of examples like what Gro-Tsen \nand Karl Petterssen supplied. If there's some diagreement about how \\d \nshould behave with UTF-8 data the GNU grep hackers should let the Perl \ncommunity decide that; that is, GNU grep can simply follow PCRE2's lead. \nBut GNU grep does need PCRE2_UCP for \\b etc.\n\n> \t$ diff <(git -P grep -P '\\d+') <(git -P grep -P '(*UCP)\\d')\n> \t53360a53361,53362\n> \t> git-gui/po/ja.po:\"- 第１行: 何をしたか、を１行で要約。\\n\"\n> \t> git-gui/po/ja.po:\"- 第２行: 空白\\n\"\n\nAlthough I don't speak Japanese I have dealt with quite a bit of \nJapanese text in a previous job, and personally I would prefer \\d to \nmatch those two lines as they do contain digits. So to me this \nparticular case is not a good argument that git grep should not match \nthose lines.\n\nOf course other people might prefer differently, and there are cases \nwhere I want to match only ASCII digits. I've learned in the past to use \n[0-9] for that. I hope PCRE2 never changes [0-9] to match anything but \nASCII digits when searching UTF-8 text.\n"},{"id":"470010","messageId":"230109.865ydf1mdu.gmgdl@evledraar.gmail.com","threadId":"59048","inReplyTo":"d6814350-10a3-55c0-68da-7e691976cd45@cs.ucla.edu","subject":"Re: bug#60690: [PATCH v2] grep: correctly identify utf-8 characters with \\{b, w} in -P","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-09T19:51:00Z","receivedAt":"2023-01-09T20:30:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 09 2023, Paul Eggert wrote:\n\n> On 1/9/23 03:35, Ævar Arnfjörð Bjarmason wrote:\n>\n>> You almost never want \"everything Unicode considers a digit\", and if you\n>> do using e.g. \\p{Nd} instead of \\d would be better in terms of\n>> expressing your intent.\n>\n> For GNU grep, PCRE2_UCP is needed because of examples like what\n> Gro-Tsen and Karl Petterssen supplied.\n\n[For reference, referring to this Twitter thread:\nhttps://twitter.com/gro_tsen/status/1610972356972875777]\n\nThose examples compared -E and -P. I think it's correct that UCP brings\nthe behavior closer to -E, but it's also different in various ways.\n\nE.g. on emacs.git (which I've been finding to be quite a nice test case)\na comparison of the two, with \"git grep\" because I found it easier to\ntest, but GNU grep will presumably find the same for those files:\n\t\n\tfor c in b s w\n\tdo\n\t\tfor pfx in '' '(*UCP)'\n\t\tdo\n\t\t\techo \"$pfx/$c:\" &&\n\t\t\tdiff -u <(git -P grep -E \"\\\\$c\") <(git -P grep -P \"$pfx\\\\$c\") | wc -l\n\t\tdone\n\tdone\n\nYields:\n\n\t/b:\n\t155781\n\t(*UCP)/b:\n\t46035\n\t/s:\n\t0\n\t(*UCP)/s:\n\t0\n\t/w:\n\t142468\n\t(*UCP)/w:\n\t9706\n\nSo the output still differs, and some of those differences may or may\nnot be wanted.\n\n> If there's some diagreement\n> about how \\d should behave with UTF-8 data the GNU grep hackers should\n> let the Perl community decide that; that is, GNU grep can simply\n> follow PCRE2's lead.\n\nPCRE2 tends to follow Perl, I'm mainly trying to point out here that it\nisn't a-priory clear how \"let Perl decide\" is supposed to map to the of\na \"grep\"-like utility, since the Perl behavior is inherently tied up\nwith knowing the encoding of the target data.\n\nFor GNU grep and \"git grep\" that's more of an all-or-nothing with\nlocales, although in this case being as close as possible to -E is\nprobably more correct than not.\n\n>> \t$ diff <(git -P grep -P '\\d+') <(git -P grep -P '(*UCP)\\d')\n>> \t53360a53361,53362\n>> \t> git-gui/po/ja.po:\"- 第１行: 何をしたか、を１行で要約。\\n\"\n>> \t> git-gui/po/ja.po:\"- 第２行: 空白\\n\"\n>\n> Although I don't speak Japanese I have dealt with quite a bit of\n> Japanese text in a previous job, and personally I would prefer \\d to\n> match those two lines as they do contain digits. So to me this\n> particular case is not a good argument that git grep should not match\n> those lines.\n\nI'm mainly raising the backwards compatibility concern, which GNU grep\nand git grep may or may not want to handle differently, but let's at\nleast be aware of the various edge cases.\n\nFor \\b I think it mostly does the right thing.\n\nFor \\w and \\d in particular I'm mainly noting that yes, sometimes you\nwant to match [0-9], and sometimes you'd want to match Japanese numbers,\nbut you rarely (or at least I haven't) want to match everything Unicode\nconsiders X, unless you're doing some self-reflection on Unicode itself.\n\nE.g. for \\d it's at least (up from just 10):\n\n\t$ perl -CO -wE 'for (1..2**20) { say chr if chr =~ /\\d/ }'|wc -l\n\t650\n\nFor \\w you similarly go from ~60 to ~130k:\n\n\t$ perl -CO -wE 'for (1..2**24) { say chr if chr =~ /\\w/ }'|wc -l\n\t134564\n\nIf all you're doing is matching either ASCII or Japanese text and you\nwant \"locale-aware numbers\" it might do the wrong thing.\n\nBut I've found it to be too promiscuous when casting a wider net, which\nis the usual use-case with 'grep\".\n\n> Of course other people might prefer differently, and there are cases\n> where I want to match only ASCII digits. I've learned in the past to\n> use [0-9] for that. I hope PCRE2 never changes [0-9] to match anything\n> but ASCII digits when searching UTF-8 text.\n\nI think that'll never change.\n"},{"id":"470015","messageId":"80b42740-c85b-cef2-622c-c5b2450e264c@cs.ucla.edu","threadId":"59048","inReplyTo":"230109.865ydf1mdu.gmgdl@evledraar.gmail.com","subject":"Re: bug#60690: [PATCH v2] grep: correctly identify utf-8 characters with \\{b, w} in -P","fromName":"Paul Eggert","fromEmail":"eggert@cs.ucla.edu","sentAt":"2023-01-09T23:12:23Z","receivedAt":"2023-01-09T23:12:29Z","isPatch":true,"sender":{"key":"eggert@cs.ucla.edu","avatar":"https://avatars.githubusercontent.com/u/572024?v=4"},"body":"On 1/9/23 11:51, Ævar Arnfjörð Bjarmason wrote:\n\n> \t/b:\n> \t155781\n> \t(*UCP)/b:\n> \t46035\n> \t/s:\n> \t0\n> \t(*UCP)/s:\n> \t0\n> \t/w:\n> \t142468\n> \t(*UCP)/w:\n> \t9706\n> \n> So the output still differs, and some of those differences may or may\n> not be wanted.\n\nI took a look at the output, and by and large I'd want the differences; \nthat is, I'd want the UCP version, which generates less output. This is \nbecause several Emacs source files are not UTF-8, and \\b has nonsense \nmatches when searching text files encoded via Shift-JIS or Big 5 or \nwhatever. For this sort of thing, the fewer matches the better.\n\n\n> If all you're doing is matching either ASCII or Japanese text and you\n> want \"locale-aware numbers\" it might do the wrong thing.\n\nI'm not seeing much of a problem here. When searching Japanese text, I \nwould expect \\d and [0-9０-９] (using both ASCII and full-width digits) to \nbe equivalent so (assuming UCP) it's not a big deal as to which regex \nyou use, since Japanese text won't contain Bengali (or whatever) digits. \nAnd when searching binary data, I'd expect a bunch of garbage no matter \nhow \\d is interpreted.\n\nHere I'm assuming [０-９] (using full-width digits) has the expected \nmeaning in PCRE2, i.e., that PCRE2 didn't make the same mistake that \nPOSIX made.\n"},{"id":"470020","messageId":"CAPUEsphgxU4R6PTEd1N7VwQ+Da1CRRvyNkeas0k2gn0WkDA+2A@mail.gmail.com","threadId":"59048","inReplyTo":"230109.86v8lf297g.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-10T04:49:20Z","receivedAt":"2023-01-10T04:49:39Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jan 9, 2023 at 4:17 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>Rather than trying to opt-out with \"/a\" or \"/aa\" I think this should be opt-in.\n>\n> As the example at the start shows you can already do this with \"(*UCP)\"\n> in the pattern, so perhaps we should just link to the pcre2pattern(3)\n> manual from git-grep(1)?\n\nConsidering that PCRE is used internally even for cases that don't\nspecify -P how would that opt-in work?\n\nFor example, in a repository with code that uses utf identifiers, the\nfollowing will fail:\n\n  $ git grep -w -E motion\n  u.c:  int émotion = 0;\n  $ git grep -w -E '(*UCP)motion'\n  fatal: command line, '(*UCP)motion': Invalid preceding regular expression\n  $ git -P grep -P -w '(*UCP)motion'\n  u.c:  int émotion = 0;\n\nCarlo\n\nCC removed gnu and the obsoleted PCRE developer list (if really needed\nwould be better to use the documented pcre2-dev@googlegroups.com,\ninstead)\n"},{"id":"470457","messageId":"xmqqr0vub3yy.fsf@gitster.g","threadId":"59048","inReplyTo":"230109.86v8lf297g.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-16T20:48:37Z","receivedAt":"2023-01-16T20:48:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> But I don't think it's safe to change the default behavior \"git-grep\",\n> it's not a mere bug fix, but a major behavior change for existing users\n> of grep.patternType=perl.\n> ...\n> Even for Perl, this behavior has been troublesome. Opinions differ, but\n> I think many would agree (and I've CC'd the main authority on Perl's\n> regex engine) that doing this by default was *probably* a mistake.\n> ...\n> As the example at the start shows you can already do this with \"(*UCP)\"\n> in the pattern, so perhaps we should just link to the pcre2pattern(3)\n> manual from git-grep(1)?\n\nSo, now do we have a final verdict on this patch?  If we are not\ntaking the \"unconditonally enable ucp\" patch (which I tend to agree\nwith a safer choice for now), it may make sense to mention (*UCP) in\nour documentation somewhere, perhaps?\n\n\n"},{"id":"470487","messageId":"20230117105123.58328-1-carenas@gmail.com","threadId":"59048","inReplyTo":"20230108155217.2817-1-carenas@gmail.com","subject":"[PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2023-01-17T10:51:23Z","receivedAt":"2023-01-17T10:52:22Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"When UTF is enabled for a PCRE match, the PCRE2_UTF flag is used\nby the pcre2_compile() call, but that would only allow for the\nuse of Unicode character properties when caseless is required\nbut not to include the additional UTF characters for all other\nclass matches.\n\nThis would result in failed matches for expressions that rely\non those properties, for ex:\n\n  $ git grep -P '\\bÆvar'\n\nAdd a configuration that could be used to enable the PCRE2_UCP\nflag to correctly match those cases, when required.\n\nThe use of this has an impact on performance that has been estimated\nto be significant.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\nChanges since v2:\n* make setting UCP and opt-in as suggested by Ævar\n* remove performance test and instead add a test\n\n Documentation/config/grep.txt |  6 ++++++\n grep.c                        | 11 ++++++++++-\n grep.h                        |  1 +\n t/t7810-grep.sh               | 13 +++++++++++++\n 4 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex e521f20390..8848db7311 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -26,3 +26,9 @@ grep.fullName::\n grep.fallbackToNoIndex::\n \tIf set to true, fall back to git grep --no-index if git grep\n \tis executed outside of a git repository.  Defaults to false.\n+\n+pcre.ucp::\n+\tIf set to true, will use all Unicode Character Properties when matching\n+\t`\\w`, `\\b`, `\\d` or the POSIX classes (ex: `[:alnum:]`) and PCRE is used\n+\tas the underlying engine. If PCRE is not being used it is ignored.\n+\tDefaults to false\ndiff --git a/grep.c b/grep.c\nindex 06eed69493..ceafb8937d 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -102,6 +102,12 @@ int grep_config(const char *var, const char *value, void *cb)\n \t\t\treturn config_error_nonbool(var);\n \t\treturn color_parse(value, color);\n \t}\n+\n+\tif (!strcmp(var, \"pcre.ucp\")) {\n+\t\topt->pcre_ucp = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -292,8 +298,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\t}\n \t\toptions |= PCRE2_CASELESS;\n \t}\n-\tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n+\tif (!opt->ignore_locale && is_utf8_locale() && !literal) {\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n+\t\tif (opt->pcre_ucp)\n+\t\t\toptions |= PCRE2_UCP;\n+\t}\n \n #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\ndiff --git a/grep.h b/grep.h\nindex 6075f997e6..082bd3a0c7 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -171,6 +171,7 @@ struct grep_opt {\n \tint file_break;\n \tint heading;\n \tint max_count;\n+\tint pcre_ucp;\n \tvoid *priv;\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 8eded6ab27..a99a967060 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -95,6 +95,7 @@ test_expect_success setup '\n \tthen\n \t\techo \"¿\" >reverse-question-mark\n \tfi &&\n+\techo \"émotion\" >ucp &&\n \tgit add . &&\n \ttest_tick &&\n \tgit commit -m initial\n@@ -1474,6 +1475,18 @@ test_expect_success PCRE 'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE\n \ttest_cmp hello_world actual\n '\n \n+test_expect_success PCRE 'grep -c pcre.ucp -P fixes \\b' '\n+\tcat >expected <<-\\EOF &&\n+\tucp:émotion\n+\tEOF\n+\tcat >pattern <<-\\EOF &&\n+\t\\bémotion\n+\tEOF\n+\tLC_ALL=en_US.UTF-8 git -c pcre.ucp=true grep -P -f pattern >actual &&\n+\ttest_cmp expected actual &&\n+\tLC_ALL=en_US.UTF-8 test_must_fail git grep -P -f pattern\n+'\n+\n test_expect_success 'grep -G invalidpattern properly dies ' '\n \ttest_must_fail git grep -G \"a[\"\n '\n-- \n2.37.1 (Apple Git-137.1)\n\n"},{"id":"470488","messageId":"230117.865yd5z4ke.gmgdl@evledraar.gmail.com","threadId":"59048","inReplyTo":"20230117105123.58328-1-carenas@gmail.com","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-17T12:38:50Z","receivedAt":"2023-01-17T13:14:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 17 2023, Carlo Marcelo Arenas Belón wrote:\n\n> When UTF is enabled for a PCRE match, the PCRE2_UTF flag is used\n> by the pcre2_compile() call, but that would only allow for the\n> use of Unicode character properties when caseless is required\n> but not to include the additional UTF characters for all other\n> class matches.\n>\n> This would result in failed matches for expressions that rely\n> on those properties, for ex:\n>\n>   $ git grep -P '\\bÆvar'\n>\n> Add a configuration that could be used to enable the PCRE2_UCP\n> flag to correctly match those cases, when required.\n>\n> The use of this has an impact on performance that has been estimated\n> to be significant.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n> Changes since v2:\n> * make setting UCP and opt-in as suggested by Ævar\n\nTo argue with myself here, I'm not so sure that just making this the\ndefault isn't the right move, especially as the GNU grep maintainer\nseems to be convinced that that's the right thing for grep(1).\n\nWe've usually just followed GNU grep semantics, so if it's doing X and\nwe're doing Y after this it's probably better to unify our behavior with\ntheirs.\n        \nI was mainly concerned with the behavior change sneaking in as a mere\nbugfix, I think it's OK if we change the behavior, as long as we're\ngoing into it with our eyes open...\n\n> * remove performance test and instead add a test\n\n...I didn't follow the thread(s) where this may have been discussed, but\nI for one would like to see a perf test with this, but maybe it was\nremoved for a good reason that I'm not aware of...\n\n\n\n>  Documentation/config/grep.txt |  6 ++++++\n>  grep.c                        | 11 ++++++++++-\n>  grep.h                        |  1 +\n>  t/t7810-grep.sh               | 13 +++++++++++++\n>  4 files changed, 30 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\n> index e521f20390..8848db7311 100644\n> --- a/Documentation/config/grep.txt\n> +++ b/Documentation/config/grep.txt\n> @@ -26,3 +26,9 @@ grep.fullName::\n>  grep.fallbackToNoIndex::\n>  \tIf set to true, fall back to git grep --no-index if git grep\n>  \tis executed outside of a git repository.  Defaults to false.\n> +\n> +pcre.ucp::\n> +\tIf set to true, will use all Unicode Character Properties when matching\n> +\t`\\w`, `\\b`, `\\d` or the POSIX classes (ex: `[:alnum:]`) and PCRE is used\n> +\tas the underlying engine. If PCRE is not being used it is ignored.\n> +\tDefaults to false\n\nThere's a couple of exceptions to this, but we tend to stick config docs\nin their corresponding Documentation/config/<namespace>.txt, so this\nshould be in a new Documentation/config/pcre.txt if we're adding this\nname.\n\nBut I'd rather that we don't expose the implementation detail that we're\nusing PCRE, which we haven't done so far. We just have a\n\"grep.patternType=perl\", which (and that's another issue, in any case)\nwe should explicitly mention here, i.e. that this is for use with that\nconfig (and corresponding option(s)).\n\nI think calling this e.g.:\n\n\tgrep.perl.Unicode=<bool>\n\tgrep.patternTypePerl.Unicode=<bool>\n\nOr even:\n\n\tgrep.patternTypePerl.Flags=u\n\nWould be better, i.e. PCRE's C API is really just mapping to the flags\nyou can find in \"perldoc perlre\" (https://perldoc.perl.org/perlre). In\nthis case the /u flag maps to the \"PCRE2_UCP\" API flag.\n\nThat we happen to use PCRE to give ourselves \"Perl\" semantics is an\nimplementation detail we should avoid exposing, so we could either give\nour config generic names, or literally map to the perl /flags/.\n\nFor now we could just die on any \"Flags\" value that isn't \"u\".\n\nOf course all of this is predicated on us wanting to leave this as an\nopt-in, which I'm not so sure about. If it's opt-out we'll avoid this\nentire question,\n\n> diff --git a/grep.c b/grep.c\n> index 06eed69493..ceafb8937d 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -102,6 +102,12 @@ int grep_config(const char *var, const char *value, void *cb)\n>  \t\t\treturn config_error_nonbool(var);\n>  \t\treturn color_parse(value, color);\n>  \t}\n> +\n> +\tif (!strcmp(var, \"pcre.ucp\")) {\n> +\t\topt->pcre_ucp = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \treturn 0;\n>  }\n>  \n> @@ -292,8 +298,11 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \t\t}\n>  \t\toptions |= PCRE2_CASELESS;\n>  \t}\n> -\tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n> +\tif (!opt->ignore_locale && is_utf8_locale() && !literal) {\n>  \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> +\t\tif (opt->pcre_ucp)\n> +\t\t\toptions |= PCRE2_UCP;\n> +\t}\n\nThis interaction with locale settings etc. is probably correct, but if\nwe're keeping the config etc. we should really document how this\ninteracts with those.\n\nI.e. you might expect \"-c grep.patternType=perl -c\n<whatever_the_setting_is>=true\" to give you UCP semantics, but we'll\nignore it based on these other criteria.\n\n>  #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n>  \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\n> diff --git a/grep.h b/grep.h\n> index 6075f997e6..082bd3a0c7 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -171,6 +171,7 @@ struct grep_opt {\n>  \tint file_break;\n>  \tint heading;\n>  \tint max_count;\n> +\tint pcre_ucp;\n>  \tvoid *priv;\n\nThe reason for why we have some \"bool\"-like settings (like \"int\nignore_case\") as an \"int\" as opposed to an \"unsigned int <name>:1\"\nbitfield is because we need to take their address via the\nparse_options() API.\n\nBut in this case it's a purely internal field, so (and again, if we're\nkeeping the option) let's use \"unsigned int ...:1\" here instead?\n\nUnless that is, we're expecting a corresponding command-line option.\n\n>  \n>  \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> index 8eded6ab27..a99a967060 100755\n> --- a/t/t7810-grep.sh\n> +++ b/t/t7810-grep.sh\n> @@ -95,6 +95,7 @@ test_expect_success setup '\n>  \tthen\n>  \t\techo \"¿\" >reverse-question-mark\n>  \tfi &&\n> +\techo \"émotion\" >ucp &&\n\nHere we carry this file through the entirety ouf our tests...\n\n>  \tgit add . &&\n>  \ttest_tick &&\n>  \tgit commit -m initial\n> @@ -1474,6 +1475,18 @@ test_expect_success PCRE 'grep -P backreferences work (the PCRE NO_AUTO_CAPTURE\n>  \ttest_cmp hello_world actual\n>  '\n>  \n> +test_expect_success PCRE 'grep -c pcre.ucp -P fixes \\b' '\n> +\tcat >expected <<-\\EOF &&\n> +\tucp:émotion\n\n...only to use it in this one test, it clearly didn't harm anything (or\nrather, I didn't run this, but I expect you did and it passed), but how\nabout avoiding more global state here doing:\n\n\ttest_when_finished \"rm -rf repo\" &&\n\tgit init repo &&\n\ttest_commit -C repo msg ucp émotion &&\n\t[...]\n\n> +\tEOF\n> +\tcat >pattern <<-\\EOF &&\n> +\t\\bémotion\n> +\tEOF\n> +\tLC_ALL=en_US.UTF-8 git -c pcre.ucp=true grep -P -f pattern >actual &&\n> +\ttest_cmp expected actual &&\n> +\tLC_ALL=en_US.UTF-8 test_must_fail git grep -P -f pattern\n\nThis will break on platforms that don't have en_US.UTF-8 (and that's not\nhypothetical, some systems will skip installing locales for various\nreasons).\n\nI see we have some almost recent breakage 1819ad327b7 (grep: fix\nmultibyte regex handling under macOS, 2022-08-26), but that one adds a\nnew MB_REGEX prereq, which presumably fails if we don't have this\nlocale.\n\nYou can just use the existing \"GETTEXT_LOCALE\", which will piggy-back on\nour existing locale tests, or we could add a corresponding one for\nen_US.UTF-8 and refactor the existing MB_REGEX to be in lib-gettext.sh\nwhere it arguably belongs...\n\n> +'\n> +\n>  test_expect_success 'grep -G invalidpattern properly dies ' '\n>  \ttest_must_fail git grep -G \"a[\"\n>  '\n\n"},{"id":"470498","messageId":"xmqqr0vt9oj9.fsf@gitster.g","threadId":"59048","inReplyTo":"230117.865yd5z4ke.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-17T15:19:38Z","receivedAt":"2023-01-17T15:20:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> To argue with myself here, I'm not so sure that just making this the\n> default isn't the right move, especially as the GNU grep maintainer\n> seems to be convinced that that's the right thing for grep(1).\n\nOK.\n\n> I think calling this e.g.:\n>\n> \tgrep.perl.Unicode=<bool>\n> \tgrep.patternTypePerl.Unicode=<bool>\n>\n> Or even:\n>\n> \tgrep.patternTypePerl.Flags=u\n>\n> Would be better, i.e. PCRE's C API is really just mapping to the flags\n> you can find in \"perldoc perlre\" (https://perldoc.perl.org/perlre). In\n> this case the /u flag maps to the \"PCRE2_UCP\" API flag.\n>\n> That we happen to use PCRE to give ourselves \"Perl\" semantics is an\n> implementation detail we should avoid exposing, so we could either give\n> our config generic names, or literally map to the perl /flags/.\n>\n> For now we could just die on any \"Flags\" value that isn't \"u\".\n>\n> Of course all of this is predicated on us wanting to leave this as an\n> opt-in, which I'm not so sure about. If it's opt-out we'll avoid this\n> entire question,\n\nMaking it opt-out would also require a similar knob to turn the\n\"flag\" off, be it a configuration variable or a command line option,\nwouldn't it?  I tend to agree with you that it makes sense to make\nit a goal to take us closer to \"grep -P\" from GNU---do they have\nsuch an opt-out knob?  If not, let's make it simple by turning it\nalways on, which would be the simplest ;-)\n\nAgain, thanks for a careful review with concrete points.\n"},{"id":"470587","messageId":"CAPUEspgzrW63GgbjXhKuvjpKXjEhiKaC7jtupiB-3AhcKTba8A@mail.gmail.com","threadId":"59048","inReplyTo":"xmqqr0vt9oj9.fsf@gitster.g","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2023-01-18T07:35:45Z","receivedAt":"2023-01-18T08:03:13Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Jan 17, 2023 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n> > To argue with myself here, I'm not so sure that just making this the\n> > default isn't the right move, especially as the GNU grep maintainer\n> > seems to be convinced that that's the right thing for grep(1).\n>\n> OK.\n\nI think that is definitely the right thing to do for grep, because the\ncurrent behaviour can only be described as a bug (and a bad one at\nit), but after all the push back and performance testing, I am also\nnot convinced anymore it needs to be the default for git, because the\nnegatives outweigh the positives.\n\nFirst there is the performance hit, which is inevitable because there\nare just a lot more characters to match when UCP tables are being\nused, and second there is the fact that PCRE2_UCP itself might not be\nwhat you want when matching code, because for example numbers are\nnever going to be using digits outside what ASCII provides, and\nidentifiers have a narrow set of characters as valid than what you\nwould expect from all written human languages in history.\n\nLastly, even with PCRE2_UCP enabled, our current logic for word\nmatches is still broken, because the current code still uses a\ndefinition of word that was done outside what the regex engines\nprovide and that roughly matches what you would expect of identifiers\nfrom C in the ASCII times.\n\n> > Of course all of this is predicated on us wanting to leave this as an\n> > opt-in, which I'm not so sure about. If it's opt-out we'll avoid this\n> > entire question,\n>\n> Making it opt-out would also require a similar knob to turn the\n> \"flag\" off, be it a configuration variable or a command line option,\n> wouldn't it?  I tend to agree with you that it makes sense to make\n> it a goal to take us closer to \"grep -P\" from GNU---do they have\n> such an opt-out knob?  If not, let's make it simple by turning it\n> always on, which would be the simplest ;-)\n\nGNU grep -P has no knob and would likely never have one.\n\nSo for now, I think we should acknowledge the bug, provide an option\nfor people that might need the fix, and fix all other problems we\nhave, which will include changes in PCRE2 as well to better fit our\nuse case.\n\nCarlo\n"},{"id":"470600","messageId":"230118.86tu0ovyvj.gmgdl@evledraar.gmail.com","threadId":"59048","inReplyTo":"CAPUEspgzrW63GgbjXhKuvjpKXjEhiKaC7jtupiB-3AhcKTba8A@mail.gmail.com","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-18T11:49:36Z","receivedAt":"2023-01-18T12:35:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Jan 17 2023, Carlo Arenas wrote:\n\n> On Tue, Jan 17, 2023 at 7:19 AM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>>\n>> > To argue with myself here, I'm not so sure that just making this the\n>> > default isn't the right move, especially as the GNU grep maintainer\n>> > seems to be convinced that that's the right thing for grep(1).\n>>\n>> OK.\n>\n> I think that is definitely the right thing to do for grep, because the\n> current behaviour can only be described as a bug (and a bad one at\n> it), but after all the push back and performance testing, I am also\n> not convinced anymore it needs to be the default for git, because the\n> negatives outweigh the positives.\n>\n> First there is the performance hit, which is inevitable because there\n> are just a lot more characters to match when UCP tables are being\n> used,[...]\n\nI'm less concerned about the performance, we should aim for correctness\nfirst. We can always provide an opt-out (and the locale setting is\nalready that opt-out).\n\n> and second there is the fact that PCRE2_UCP itself might not be\n> what you want when matching code, because for example numbers are\n> never going to be using digits outside what ASCII provides, and\n> identifiers have a narrow set of characters as valid than what you\n> would expect from all written human languages in history.\n\n[0-9] will be ASCII, but \\d will use [^0-9] Unicode numbers.\n\nI agree it might not be expected by some, but I can't really square that\nview in my mind with the desire to match \"\\bÆvar\" :). After all that \"Æ\"\nis also arbitrary byte garbage in the ASCII-view of the world.\n\nI can see how it might be more practical in some cases to have \"\\b\" have\nUnicode semantics, but to specifically make \"\\d\" an exception. But the\nship has sailed on that in Perl & PCRE land years (or more than a decade\nago). I think us coming up with some exception to that would probably\nsuck more than going with their behavior.\n\n> Lastly, even with PCRE2_UCP enabled, our current logic for word\n> matches is still broken, because the current code still uses a\n> definition of word that was done outside what the regex engines\n> provide and that roughly matches what you would expect of identifiers\n> from C in the ASCII times.\n\nYes, FWIW I have some WIP patches somewhere to get rid of that bit of\ngrep.c if we're using PCRE. I.e. the \"-w\" should be powered by just\nadding \"\\b\" to the start/end of the provided string.\n\nThat'll then be correct, and faster.\n\nI can't remember if there were some subtle bugs in that, or why I didn't\nfinish that...\n\n>> > Of course all of this is predicated on us wanting to leave this as an\n>> > opt-in, which I'm not so sure about. If it's opt-out we'll avoid this\n>> > entire question,\n>>\n>> Making it opt-out would also require a similar knob to turn the\n>> \"flag\" off, be it a configuration variable or a command line option,\n>> wouldn't it?  I tend to agree with you that it makes sense to make\n>> it a goal to take us closer to \"grep -P\" from GNU---do they have\n>> such an opt-out knob?  If not, let's make it simple by turning it\n>> always on, which would be the simplest ;-)\n>\n> GNU grep -P has no knob and would likely never have one.\n\nI think the general knob in not just GNU grep but GNU utils and the\nwider *nix landscape is \"tweak your LC_ALL and/or other locale\nvaribales\".\n\nWhich works for it, and will work for us once we're using PCRE2_UCP too.\n\n> So for now, I think we should acknowledge the bug, provide an option\n> for people that might need the fix, and fix all other problems we\n> have, which will include changes in PCRE2 as well to better fit our\n> use case.\n\nHrm, what are those PCRE2 changes? The one I saw so far (or was it a\nproposal) was to just make its \"grep\" utility use the PCRE2_UCP like GNU\ngrep is now doing in its unreleased version in its git repo...\n"},{"id":"470640","messageId":"xmqq7cxj6chn.fsf@gitster.g","threadId":"59048","inReplyTo":"230118.86tu0ovyvj.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T16:20:20Z","receivedAt":"2023-01-18T16:22:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> GNU grep -P has no knob and would likely never have one.\n>\n> I think the general knob in not just GNU grep but GNU utils and the\n> wider *nix landscape is \"tweak your LC_ALL and/or other locale\n> varibales\".\n>\n> Which works for it, and will work for us once we're using PCRE2_UCP too.\n>\n>> So for now, I think we should acknowledge the bug, provide an option\n>> for people that might need the fix, and fix all other problems we\n>> have, which will include changes in PCRE2 as well to better fit our\n>> use case.\n>\n> Hrm, what are those PCRE2 changes? The one I saw so far (or was it a\n> proposal) was to just make its \"grep\" utility use the PCRE2_UCP like GNU\n> grep is now doing in its unreleased version in its git repo...\n\nYeah, I didn't understand Carlo's comment in that paragraph at all.\n\nIn short, it sounds to me that the earlier one that added PCRE2_UCP\nunconditionally would be the best alternative among those that have\nbeen discussed.\n\n\n"},{"id":"470680","messageId":"230119.86cz7bwign.gmgdl@evledraar.gmail.com","threadId":"59048","inReplyTo":"xmqq7cxj6chn.fsf@gitster.g","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2023-01-18T23:06:43Z","receivedAt":"2023-01-18T23:06:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Jan 18 2023, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n>\n>>> GNU grep -P has no knob and would likely never have one.\n>>\n>> I think the general knob in not just GNU grep but GNU utils and the\n>> wider *nix landscape is \"tweak your LC_ALL and/or other locale\n>> varibales\".\n>>\n>> Which works for it, and will work for us once we're using PCRE2_UCP too.\n>>\n>>> So for now, I think we should acknowledge the bug, provide an option\n>>> for people that might need the fix, and fix all other problems we\n>>> have, which will include changes in PCRE2 as well to better fit our\n>>> use case.\n>>\n>> Hrm, what are those PCRE2 changes? The one I saw so far (or was it a\n>> proposal) was to just make its \"grep\" utility use the PCRE2_UCP like GNU\n>> grep is now doing in its unreleased version in its git repo...\n>\n> Yeah, I didn't understand Carlo's comment in that paragraph at all.\n>\n> In short, it sounds to me that the earlier one that added PCRE2_UCP\n> unconditionally would be the best alternative among those that have\n> been discussed.\n\nI agree.\n"},{"id":"470684","messageId":"xmqqzgaf2zpt.fsf@gitster.g","threadId":"59048","inReplyTo":"230119.86cz7bwign.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3] grep: correctly identify utf-8 characters with \\{b,w} in -P","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-18T23:24:30Z","receivedAt":"2023-01-18T23:24:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> In short, it sounds to me that the earlier one that added PCRE2_UCP\n>> unconditionally would be the best alternative among those that have\n>> been discussed.\n>\n> I agree.\n\nSo, ... we'll mark cb/grep-pcre-ucp, ea8bc435 (grep: correctly\nidentify utf-8 characters with \\{b,w} in -P, 2023-01-08), to be\nmerged to 'next' and see what happens.  I'll add your Acked-by\nwhile at it, if you do not mind.\n\n---- >8 --------- >8 --------- >8 --------- >8 ------\nFrom: Carlo Marcelo Arenas Belón <carenas@gmail.com>\nDate: Sun, 8 Jan 2023 07:52:17 -0800\nSubject: [PATCH] grep: correctly identify utf-8 characters with \\{b,w} in -P\n\nWhen UTF is enabled for a PCRE match, the corresponding flags are\nadded to the pcre2_compile() call, but PCRE2_UCP wasn't included.\n\nThis prevents extending the meaning of the character classes to\ninclude those new valid characters and therefore result in failed\nmatches for expressions that rely on that extention, for ex:\n\n  $ git grep -P '\\bÆvar'\n\nAdd PCRE2_UCP so that \\w will include Æ and therefore \\b could\ncorrectly match the beginning of that word.\n\nThis has an impact on performance that has been estimated to be\nbetween 20% to 40% and that is shown through the added performance\ntest.\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\nAcked-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n grep.c                              |  2 +-\n t/perf/p7822-grep-perl-character.sh | 42 +++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 1 deletion(-)\n create mode 100755 t/perf/p7822-grep-perl-character.sh\n\ndiff --git a/grep.c b/grep.c\nindex 06eed69493..1687f65b64 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -293,7 +293,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\toptions |= PCRE2_CASELESS;\n \t}\n \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n-\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n+\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n \n #ifndef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in 10.36 */\ndiff --git a/t/perf/p7822-grep-perl-character.sh b/t/perf/p7822-grep-perl-character.sh\nnew file mode 100755\nindex 0000000000..87009c60df\n--- /dev/null\n+++ b/t/perf/p7822-grep-perl-character.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+\n+test_description=\"git-grep's perl regex\n+\n+If GIT_PERF_GREP_THREADS is set to a list of threads (e.g. '1 4 8'\n+etc.) we will test the patterns under those numbers of threads.\n+\"\n+\n+. ./perf-lib.sh\n+\n+test_perf_large_repo\n+test_checkout_worktree\n+\n+if test -n \"$GIT_PERF_GREP_THREADS\"\n+then\n+\ttest_set_prereq PERF_GREP_ENGINES_THREADS\n+fi\n+\n+for pattern in \\\n+\t'\\\\bhow' \\\n+\t'\\\\bÆvar' \\\n+\t'\\\\d+ \\\\bÆvar' \\\n+\t'\\\\bBelón\\\\b' \\\n+\t'\\\\w{12}\\\\b'\n+do\n+\techo '$pattern' >pat\n+\tif ! test_have_prereq PERF_GREP_ENGINES_THREADS\n+\tthen\n+\t\ttest_perf \"grep -P '$pattern'\" --prereq PCRE \"\n+\t\t\tgit -P grep -f pat || :\n+\t\t\"\n+\telse\n+\t\tfor threads in $GIT_PERF_GREP_THREADS\n+\t\tdo\n+\t\t\ttest_perf \"grep -P '$pattern' with $threads threads\" --prereq PTHREADS,PCRE \"\n+\t\t\t\tgit -c grep.threads=$threads -P grep -f pat || :\n+\t\t\t\"\n+\t\tdone\n+\tfi\n+done\n+\n+test_done\n-- \n2.39.1-231-ga7caae2729\n\n"}]}