{"thread":{"id":"60675","subject":"[RFC PATCH] grep: default to posix digits with -P","startedAt":"2024-01-01T15:04:15Z","lastAt":"2024-07-10T13:49:31Z","messageCount":6,"participants":["Carlo Marcelo Arenas Belón","René Scharfe","Eric Sunshine","Carlo Arenas","Mathias Krause"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"486188","messageId":"20240101150336.89098-1-carenas@gmail.com","threadId":"60675","inReplyTo":null,"subject":"[RFC PATCH] grep: default to posix digits with -P","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2024-01-01T15:03:36Z","receivedAt":"2024-01-01T15:04:15Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Since acabd2048e (grep: correctly identify utf-8 characters with\n\\{b,w} in -P, 2023-01-08), PCRE2's UCP mode has been enabled when\nUTF content was expected and therefore muktibyte characters are\nincluded as part of all character classes (when defined).\n\nnote that if the locale used is not UTF enabled (specially: C, POSIX)\nor uses an extended charmap binary that is not unicode compatible, binary\nmatch will be used instead.\n\nIt was argued that doing so, at least for \\d, was not a good idea,\nas that might not be what the user expected based on its historical\nmeaning and was also slower, and indeed a similar change that was done\nto GNU grep was reverted and required further tweaks.\n\nAt that time, PCRE2 didn't have a way to disable UCP's character\nexpansion selectively, but flags to do so, and that will be\navailable in the next release (planned soon) were added, and\none of them has been in use by GNU grep since their last release\nin May (only if built and linked against the prereleased PCRE2\nlibrary though).\n\nAdd flags to make sure that both \\d and [:digit:] only include\nASCII digits so that `git grep` will be closer to GNU grep and\nimprove performance as a side effect, but add a configuration flag\nto allow keeping the current behaviour (which is closer to perl\nand ripgrep).\n\nSigned-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n---\n Documentation/config/grep.txt       |  4 ++++\n grep.c                              | 37 ++++++++++++++++++++++-------\n grep.h                              |  5 ++++\n t/perf/p7822-grep-perl-character.sh | 11 +++++++--\n t/t7818-grep-digit.sh               | 32 +++++++++++++++++++++++++\n 5 files changed, 78 insertions(+), 11 deletions(-)\n create mode 100755 t/t7818-grep-digit.sh\n\ndiff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\nindex e521f20390..4e405c8ad1 100644\n--- a/Documentation/config/grep.txt\n+++ b/Documentation/config/grep.txt\n@@ -26,3 +26,7 @@ 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+grep.perl.digit::\n+\tIf set to true, use the perl definitions for \\d and [:digit:].\n+\tDefaults to false.\ndiff --git a/grep.c b/grep.c\nindex fc2d0c837a..fec36ccb30 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -88,6 +88,11 @@ int grep_config(const char *var, const char *value,\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"grep.perl.digit\")) {\n+\t\topt->perl_digit = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"color.grep\"))\n \t\topt->color = git_config_colorbool(var, value);\n \tif (!strcmp(var, \"color.grep.match\")) {\n@@ -301,6 +306,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \tint patinforet;\n \tsize_t jitsizearg;\n \tint literal = !opt->ignore_case && (p->fixed || p->is_fixed);\n+\tuint32_t xoptions = 0;\n \n \t/*\n \t * Call pcre2_general_context_create() before calling any\n@@ -321,17 +327,22 @@ 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-\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n+\tif (!opt->ignore_locale && is_utf8_locale() && !literal) {\n+\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n-#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n-\t/*\n-\t * Work around a JIT bug related to invalid Unicode character handling\n-\t * fixed in 10.35:\n-\t * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n-\t */\n-\toptions &= ~PCRE2_UCP;\n+#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n+\t\t/*\n+\t\t * Work around a JIT bug related to invalid Unicode character handling\n+\t\t * fixed in 10.35:\n+\t\t * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n+\t\t */\n+\t\toptions |= PCRE2_UCP;\n+#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER\n+\t\tif (!opt->perl_digit)\n+\t\t\txoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);\n #endif\n+#endif\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 */\n@@ -339,6 +350,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n \t\toptions |= PCRE2_NO_START_OPTIMIZE;\n #endif\n \n+\tif (xoptions) {\n+\t\tif (!p->pcre2_compile_context)\n+\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n+\n+\t\tpcre2_set_compile_extra_options(p->pcre2_compile_context,\n+\t\t\t\t\t\txoptions);\n+\t}\n+\n \tp->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n \t\t\t\t\t p->patternlen, options, &error, &erroffset,\n \t\t\t\t\t p->pcre2_compile_context);\ndiff --git a/grep.h b/grep.h\nindex 926c0875c4..cd5c416a0a 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -4,6 +4,9 @@\n #ifdef USE_LIBPCRE2\n #define PCRE2_CODE_UNIT_WIDTH 8\n #include <pcre2.h>\n+#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 43) || PCRE2_MAJOR >= 11\n+#define GIT_PCRE2_VERSION_10_43_OR_HIGHER\n+#endif\n #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n #endif\n@@ -178,6 +181,8 @@ struct grep_opt {\n \n \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n \tvoid *output_priv;\n+\n+\tunsigned perl_digit:1;\n };\n \n #define GREP_OPT_INIT { \\\ndiff --git a/t/perf/p7822-grep-perl-character.sh b/t/perf/p7822-grep-perl-character.sh\nindex 87009c60df..cc88d5a695 100755\n--- a/t/perf/p7822-grep-perl-character.sh\n+++ b/t/perf/p7822-grep-perl-character.sh\n@@ -8,6 +8,13 @@ etc.) we will test the patterns under those numbers of threads.\n \n . ./perf-lib.sh\n \n+# setting a LOCALE is needed, but not yet supported by :\n+#. \"$TEST_DIRECTORY\"/lib-gettext.sh\n+\n+# Invoke like:\n+#\n+# LC_ALL=is_IS.utf8 ./p7822-grep-perl-character.sh\n+\n test_perf_large_repo\n test_checkout_worktree\n \n@@ -27,13 +34,13 @@ do\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\tgit grep -P -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\tgit -c grep.threads=$threads grep -P -f pat || :\n \t\t\t\"\n \t\tdone\n \tfi\ndiff --git a/t/t7818-grep-digit.sh b/t/t7818-grep-digit.sh\nnew file mode 100755\nindex 0000000000..44007e6be6\n--- /dev/null\n+++ b/t/t7818-grep-digit.sh\n@@ -0,0 +1,32 @@\n+#!/bin/sh\n+\n+test_description='git grep -P with digits'\n+\n+TEST_PASSES_SANITIZE_LEAK=true\n+. ./lib-gettext.sh\n+\n+test_expect_success 'setup' '\n+\techo 2023 >ascii &&\n+\tprintf \"\\357\\274\\222\\357\\274\\220\\357\\274\\222\\357\\274\\223\\n\" >fullwidth &&\n+\tprintf \"\\331\\241\\331\\244\\331\\244\\331\\245\\n\" >multibyte &&\n+\tgit add . &&\n+\tgit commit -m. &&\n+\tLC_ALL=\"$is_IS_locale\" &&\n+\texport LC_ALL\n+'\n+\n+test_expect_success PCRE 'grep -P \"\\d\"' '\n+\techo \"ascii:2023\" >expected &&\n+\tgit grep -P \"\\d{2}[[:digit:]]{2}\" >actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success PCRE 'git -c grep.perl.digit' '\n+\ttest_config grep.perl.digit true &&\n+\tgit grep -P \"\\d{2}[[:digit:]]{2}\" >actual &&\n+\tgrep fullwidth actual &&\n+\tgrep multibyte actual &&\n+\ttest_line_count = 3 actual\n+'\n+\n+test_done\n-- \n2.39.3 (Apple Git-145)\n\n"},{"id":"486191","messageId":"55309061-5996-4f70-8e6b-b341fc632ddf@web.de","threadId":"60675","inReplyTo":"20240101150336.89098-1-carenas@gmail.com","subject":"Re: [RFC PATCH] grep: default to posix digits with -P","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-01T17:18:22Z","receivedAt":"2024-01-01T17:18:24Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:\n> Since acabd2048e (grep: correctly identify utf-8 characters with\n> \\{b,w} in -P, 2023-01-08), PCRE2's UCP mode has been enabled when\n> UTF content was expected and therefore muktibyte characters are\n> included as part of all character classes (when defined).\n>\n> note that if the locale used is not UTF enabled (specially: C, POSIX)\n> or uses an extended charmap binary that is not unicode compatible, binary\n> match will be used instead.\n>\n> It was argued that doing so, at least for \\d, was not a good idea,\n> as that might not be what the user expected based on its historical\n> meaning and was also slower, and indeed a similar change that was done\n> to GNU grep was reverted and required further tweaks.\n>\n> At that time, PCRE2 didn't have a way to disable UCP's character\n> expansion selectively, but flags to do so, and that will be\n> available in the next release (planned soon) were added, and\n> one of them has been in use by GNU grep since their last release\n> in May (only if built and linked against the prereleased PCRE2\n> library though).\n>\n> Add flags to make sure that both \\d and [:digit:] only include\n> ASCII digits so that `git grep` will be closer to GNU grep and\n> improve performance as a side effect, but add a configuration flag\n> to allow keeping the current behaviour (which is closer to perl\n> and ripgrep).\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>\n> ---\n>  Documentation/config/grep.txt       |  4 ++++\n>  grep.c                              | 37 ++++++++++++++++++++++-------\n>  grep.h                              |  5 ++++\n>  t/perf/p7822-grep-perl-character.sh | 11 +++++++--\n>  t/t7818-grep-digit.sh               | 32 +++++++++++++++++++++++++\n>  5 files changed, 78 insertions(+), 11 deletions(-)\n>  create mode 100755 t/t7818-grep-digit.sh\n>\n> diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt\n> index e521f20390..4e405c8ad1 100644\n> --- a/Documentation/config/grep.txt\n> +++ b/Documentation/config/grep.txt\n> @@ -26,3 +26,7 @@ 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> +grep.perl.digit::\n> +\tIf set to true, use the perl definitions for \\d and [:digit:].\n> +\tDefaults to false.\n> diff --git a/grep.c b/grep.c\n> index fc2d0c837a..fec36ccb30 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -88,6 +88,11 @@ int grep_config(const char *var, const char *value,\n>  \t\treturn 0;\n>  \t}\n>\n> +\tif (!strcmp(var, \"grep.perl.digit\")) {\n> +\t\topt->perl_digit = git_config_bool(var, value);\n> +\t\treturn 0;\n> +\t}\n> +\n>  \tif (!strcmp(var, \"color.grep\"))\n>  \t\topt->color = git_config_colorbool(var, value);\n>  \tif (!strcmp(var, \"color.grep.match\")) {\n> @@ -301,6 +306,7 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \tint patinforet;\n>  \tsize_t jitsizearg;\n>  \tint literal = !opt->ignore_case && (p->fixed || p->is_fixed);\n> +\tuint32_t xoptions = 0;\n>\n>  \t/*\n>  \t * Call pcre2_general_context_create() before calling any\n> @@ -321,17 +327,22 @@ 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> -\t\toptions |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n> +\tif (!opt->ignore_locale && is_utf8_locale() && !literal) {\n> +\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>\n> -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n> -\t/*\n> -\t * Work around a JIT bug related to invalid Unicode character handling\n> -\t * fixed in 10.35:\n> -\t * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n> -\t */\n> -\toptions &= ~PCRE2_UCP;\n> +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n> +\t\t/*\n> +\t\t * Work around a JIT bug related to invalid Unicode character handling\n> +\t\t * fixed in 10.35:\n> +\t\t * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n> +\t\t */\n> +\t\toptions |= PCRE2_UCP;\n> +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER\n> +\t\tif (!opt->perl_digit)\n> +\t\t\txoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);\n>  #endif\n> +#endif\n\nWhy do we need that extra level of indentation?\n\nThe old code turned PCRE2_UCP on by default and turned it off for older\nversions.  The new code enables PCRE2_UCP only for newer versions.  The\nresult should be the same, no?  So why change that part at all?\n\nBut the comment is now off, isn't it?  The workaround was turning\nPCRE2_UCP off for older versions (because those were broken), not\nturning it on for newer versions (because it would be required by some\nunfixed regression).\n\nDo we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and\nGIT_PCRE2_VERSION_10_43_OR_HIGHER?  Keeping them separate would help\nkeep the #ifdef parts as short as possible and maintainable\nindependently.\n\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 */\n> @@ -339,6 +350,14 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>  \t\toptions |= PCRE2_NO_START_OPTIMIZE;\n>  #endif\n>\n> +\tif (xoptions) {\n> +\t\tif (!p->pcre2_compile_context)\n> +\t\t\tp->pcre2_compile_context = pcre2_compile_context_create(p->pcre2_general_context);\n> +\n> +\t\tpcre2_set_compile_extra_options(p->pcre2_compile_context,\n> +\t\t\t\t\t\txoptions);\n> +\t}\n> +\n>  \tp->pcre2_pattern = pcre2_compile((PCRE2_SPTR)p->pattern,\n>  \t\t\t\t\t p->patternlen, options, &error, &erroffset,\n>  \t\t\t\t\t p->pcre2_compile_context);\n> diff --git a/grep.h b/grep.h\n> index 926c0875c4..cd5c416a0a 100644\n> --- a/grep.h\n> +++ b/grep.h\n> @@ -4,6 +4,9 @@\n>  #ifdef USE_LIBPCRE2\n>  #define PCRE2_CODE_UNIT_WIDTH 8\n>  #include <pcre2.h>\n> +#if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 43) || PCRE2_MAJOR >= 11\n> +#define GIT_PCRE2_VERSION_10_43_OR_HIGHER\n> +#endif\n>  #if (PCRE2_MAJOR >= 10 && PCRE2_MINOR >= 36) || PCRE2_MAJOR >= 11\n>  #define GIT_PCRE2_VERSION_10_36_OR_HIGHER\n>  #endif\n> @@ -178,6 +181,8 @@ struct grep_opt {\n>\n>  \tvoid (*output)(struct grep_opt *opt, const void *data, size_t size);\n>  \tvoid *output_priv;\n> +\n> +\tunsigned perl_digit:1;\n>  };\n>\n>  #define GREP_OPT_INIT { \\\n> diff --git a/t/perf/p7822-grep-perl-character.sh b/t/perf/p7822-grep-perl-character.sh\n> index 87009c60df..cc88d5a695 100755\n> --- a/t/perf/p7822-grep-perl-character.sh\n> +++ b/t/perf/p7822-grep-perl-character.sh\n> @@ -8,6 +8,13 @@ etc.) we will test the patterns under those numbers of threads.\n>\n>  . ./perf-lib.sh\n>\n> +# setting a LOCALE is needed, but not yet supported by :\n> +#. \"$TEST_DIRECTORY\"/lib-gettext.sh\n> +\n> +# Invoke like:\n> +#\n> +# LC_ALL=is_IS.utf8 ./p7822-grep-perl-character.sh\n> +\n>  test_perf_large_repo\n>  test_checkout_worktree\n>\n> @@ -27,13 +34,13 @@ do\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\tgit grep -P -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\tgit -c grep.threads=$threads grep -P -f pat || :\n\nWhat's going on here?  You remove the -P option of git (--no-pager) and\nadd the -P option of git grep (--perl-regexp).  So this perf test never\ntested PCRE despite its name?  Seems worth a separate patch.\n\nDo the performance numbers in acabd2048e (grep: correctly identify utf-8\ncharacters with \\{b,w} in -P, 2023-01-08) still hold up in that light?\n\n>  \t\t\t\"\n>  \t\tdone\n>  \tfi\n\nRené\n\n"},{"id":"486192","messageId":"CAPig+cQBn8CefD2O_DVQTj2QUnU4mwxL4YLVH48utPd2CToUoQ@mail.gmail.com","threadId":"60675","inReplyTo":"20240101150336.89098-1-carenas@gmail.com","subject":"Re: [RFC PATCH] grep: default to posix digits with -P","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-01-01T18:07:42Z","receivedAt":"2024-01-01T18:07:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Jan 1, 2024 at 10:04 AM Carlo Marcelo Arenas Belón\n<carenas@gmail.com> wrote:\n> Since acabd2048e (grep: correctly identify utf-8 characters with\n> \\{b,w} in -P, 2023-01-08), PCRE2's UCP mode has been enabled when\n> UTF content was expected and therefore muktibyte characters are\n> included as part of all character classes (when defined).\n\ns/muktibyte/multibyte/\n"},{"id":"486208","messageId":"CAPUEspgbVx6wbp4UNjjxFO8iNJw3i2FnJxNwn4pk5EbaAP-7gQ@mail.gmail.com","threadId":"60675","inReplyTo":"55309061-5996-4f70-8e6b-b341fc632ddf@web.de","subject":"Re: [RFC PATCH] grep: default to posix digits with -P","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2024-01-02T19:02:10Z","receivedAt":"2024-01-02T19:02:24Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:\n>\n> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:\n> > @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n> >               }\n> >               options |= PCRE2_CASELESS;\n> >       }\n> > -     if (!opt->ignore_locale && is_utf8_locale() && !literal)\n> > -             options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n> > +     if (!opt->ignore_locale && is_utf8_locale() && !literal) {\n> > +             options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> >\n> > -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n> > -     /*\n> > -      * Work around a JIT bug related to invalid Unicode character handling\n> > -      * fixed in 10.35:\n> > -      * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n> > -      */\n> > -     options &= ~PCRE2_UCP;\n> > +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n> > +             /*\n> > +              * Work around a JIT bug related to invalid Unicode character handling\n> > +              * fixed in 10.35:\n> > +              * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n> > +              */\n> > +             options |= PCRE2_UCP;\n> > +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER\n> > +             if (!opt->perl_digit)\n> > +                     xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);\n> >  #endif\n> > +#endif\n>\n> Why do we need that extra level of indentation?\n\nI was just trying to simplify the code by including all the logic in\none single set.\n\nThe original lack of indentation that was introduced by later fixes to\nthe code was IMHO also misguided since the obvious \"objective\" as set\nin the original code that added PCRE2_UCP was that it should be used\nwhenever UTF was also in use as shown by\nacabd2048ee0ee53728100408970ab45a6dab65e.\n\nOf course, we soon found out that the original implementation of\nPCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an\nexception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.\n\nNote that the problematic code is only relevant when JIT is also\nenabled, but JIT is almost always enabled.\n\n> The old code turned PCRE2_UCP on by default and turned it off for older\n> versions. The new code enables PCRE2_UCP only for newer versions.  The\n> result should be the same, no?  So why change that part at all?\n\nBecause it gets us a little closer to the real reason why we need to\ndisable UCP for anything older than 10.35, and that is that there is a\nbug there that is ONLY relevant if we are using JIT.\n\nMy hope though is that with the release of 10.43 (currently in RC1),\n10.34 will become irrelevant soon enough and this whole code could be\ncleaned up further.\n\n> But the comment is now off, isn't it?  The workaround was turning\n> PCRE2_UCP off for older versions (because those were broken), not\n> turning it on for newer versions (because it would be required by some\n> unfixed regression).\n\nThe comment was never correct, because it was turning it off, because\nthe combination of JIT + MATCH_INVALID_UTF (which was released in\n10.34) + UCP is broken.\n\n> Do we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and\n> GIT_PCRE2_VERSION_10_43_OR_HIGHER?  Keeping them separate would help\n> keep the #ifdef parts as short as possible and maintainable\n> independently.\n\nI thought that nesting them would make it simpler to maintain, since\nthere are somehow connected anyway.\n\nThe additional flags that are added in PCRE 10.43 are ONLY needed and\nuseful on top of PCRE2_UCP, which itself only makes sense on top of\nUTF.\n\n> > @@ -27,13 +34,13 @@ do\n> >       if ! test_have_prereq PERF_GREP_ENGINES_THREADS\n> >       then\n> >               test_perf \"grep -P '$pattern'\" --prereq PCRE \"\n> > -                     git -P grep -f pat || :\n> > +                     git grep -P -f pat || :\n> >               \"\n> >       else\n> >               for threads in $GIT_PERF_GREP_THREADS\n> >               do\n> >                       test_perf \"grep -P '$pattern' with $threads threads\" --prereq PTHREADS,PCRE \"\n> > -                             git -c grep.threads=$threads -P grep -f pat || :\n> > +                             git -c grep.threads=$threads grep -P -f pat || :\n>\n> What's going on here?  You remove the -P option of git (--no-pager) and\n> add the -P option of git grep (--perl-regexp).  So this perf test never\n> tested PCRE despite its name?  Seems worth a separate patch.\n\nMy original code was a dud.  This would make that at least more obvious,\n\n> Do the performance numbers in acabd2048e (grep: correctly identify utf-8\n> characters with \\{b,w} in -P, 2023-01-08) still hold up in that light?\n\nFWIW the performance numbers still hold up, but just because I did the\ntests independently using hyperfine, and indeed when I did the first\nversion of this patch, I had a nice reproducible description that\nshowed how to get 20% better performance while searching the git\nrepository itself for something like 4 digits (as used in Copyright\ndates).\n\nMy idea was to reply to my own post with instructions on how to test\nthis, which basically require to also compile the recently released\n10.43RC1 and build against that.\n\nIndeed, AFAIK the test would fail if built with an older version of\nPCRE, but wasn't sure if making a prerequisite that hardcoded the\nversion in test-tool might be the best approach to prevent that,\nespecially with all the libification work.\n\nCarlo\n\nPS. your reply got lost somehow in my SPAM folder, so I apologize for\nthe late reply\n"},{"id":"486317","messageId":"ff21e91a-f2db-4d37-ab9b-da9a492db8d3@web.de","threadId":"60675","inReplyTo":"CAPUEspgbVx6wbp4UNjjxFO8iNJw3i2FnJxNwn4pk5EbaAP-7gQ@mail.gmail.com","subject":"Re: [RFC PATCH] grep: default to posix digits with -P","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-01-04T21:14:23Z","receivedAt":"2024-01-04T21:14:26Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 02.01.24 um 20:02 schrieb Carlo Arenas:\n> On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:\n>>\n>> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:\n>>> @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>>>               }\n>>>               options |= PCRE2_CASELESS;\n>>>       }\n>>> -     if (!opt->ignore_locale && is_utf8_locale() && !literal)\n>>> -             options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n>>> +     if (!opt->ignore_locale && is_utf8_locale() && !literal) {\n>>> +             options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>>>\n>>> -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n>>> -     /*\n>>> -      * Work around a JIT bug related to invalid Unicode character handling\n>>> -      * fixed in 10.35:\n>>> -      * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n>>> -      */\n>>> -     options &= ~PCRE2_UCP;\n>>> +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n>>> +             /*\n>>> +              * Work around a JIT bug related to invalid Unicode character handling\n>>> +              * fixed in 10.35:\n>>> +              * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n>>> +              */\n>>> +             options |= PCRE2_UCP;\n>>> +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER\n>>> +             if (!opt->perl_digit)\n>>> +                     xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);\n>>>  #endif\n>>> +#endif\n>>\n>> Why do we need that extra level of indentation?\n>\n> I was just trying to simplify the code by including all the logic in\n> one single set.\n>\n> The original lack of indentation that was introduced by later fixes to\n> the code was IMHO also misguided since the obvious \"objective\" as set\n> in the original code that added PCRE2_UCP was that it should be used\n> whenever UTF was also in use as shown by\n> acabd2048ee0ee53728100408970ab45a6dab65e.\n\nOne level of indentation is correct because the code in question is part\nof a function and it's not nested in a loop or conditional.  And\npreprocessor directives don't affect the indentation of C code.  Am I\nmissing something?\n\n> Of course, we soon found out that the original implementation of\n> PCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an\n> exception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.\n>\n> Note that the problematic code is only relevant when JIT is also\n> enabled, but JIT is almost always enabled.\n>\n>> The old code turned PCRE2_UCP on by default and turned it off for older\n>> versions. The new code enables PCRE2_UCP only for newer versions.  The\n>> result should be the same, no?  So why change that part at all?\n>\n> Because it gets us a little closer to the real reason why we need to\n> disable UCP for anything older than 10.35, and that is that there is a\n> bug there that is ONLY relevant if we are using JIT.\n\nHow so?  If the same flags are set in the end then it seems like a\nlateral movement to me.\n\nDo you want to disable JIT compilation for older versions?\n\n> My hope though is that with the release of 10.43 (currently in RC1),\n> 10.34 will become irrelevant soon enough and this whole code could be\n> cleaned up further.\n\nCleanup is good, but if we package the workarounds nicely we can keep\nthem around indefinitely without them bothering us.  Debian buster\nstill has a few months of support left and comes with PCRE2 10.32..\n\n>> But the comment is now off, isn't it?  The workaround was turning\n>> PCRE2_UCP off for older versions (because those were broken), not\n>> turning it on for newer versions (because it would be required by some\n>> unfixed regression).\n>\n> The comment was never correct, because it was turning it off, because\n> the combination of JIT + MATCH_INVALID_UTF (which was released in\n> 10.34) + UCP is broken.\n\nThe code disabled PCRE2_UCP for PCRE2 10.34 and older.  The comment\nclaimed that this was done as a workaround for a bug fixed in 10.35.\nYou seem to say the same.  What was incorrect about the comment?\n\n>> Do we need to nest the checks for GIT_PCRE2_VERSION_10_35_OR_HIGHER and\n>> GIT_PCRE2_VERSION_10_43_OR_HIGHER?  Keeping them separate would help\n>> keep the #ifdef parts as short as possible and maintainable\n>> independently.\n>\n> I thought that nesting them would make it simpler to maintain, since\n> there are somehow connected anyway.\n>\n> The additional flags that are added in PCRE 10.43 are ONLY needed and\n> useful on top of PCRE2_UCP, which itself only makes sense on top of\n> UTF.\n\nThis conditional stacking is complicated.  I find the old model where\nwe say which features we want up front and then disable buggy ones\nfor specific versions easier to grasp.\n\n>>> @@ -27,13 +34,13 @@ do\n>>>       if ! test_have_prereq PERF_GREP_ENGINES_THREADS\n>>>       then\n>>>               test_perf \"grep -P '$pattern'\" --prereq PCRE \"\n>>> -                     git -P grep -f pat || :\n>>> +                     git grep -P -f pat || :\n>>>               \"\n>>>       else\n>>>               for threads in $GIT_PERF_GREP_THREADS\n>>>               do\n>>>                       test_perf \"grep -P '$pattern' with $threads threads\" --prereq PTHREADS,PCRE \"\n>>> -                             git -c grep.threads=$threads -P grep -f pat || :\n>>> +                             git -c grep.threads=$threads grep -P -f pat || :\n>>\n>> What's going on here?  You remove the -P option of git (--no-pager) and\n>> add the -P option of git grep (--perl-regexp).  So this perf test never\n>> tested PCRE despite its name?  Seems worth a separate patch.\n>\n> My original code was a dud.  This would make that at least more obvious,\n\nThe change is good, but I don't see any connection to the\ngrep.perl.digit feature that this patch is primarily about,\nso I (still) think it deserves its own patch.\n\n>> Do the performance numbers in acabd2048e (grep: correctly identify utf-8\n>> characters with \\{b,w} in -P, 2023-01-08) still hold up in that light?\n>\n> FWIW the performance numbers still hold up, but just because I did the\n> tests independently using hyperfine, and indeed when I did the first\n> version of this patch, I had a nice reproducible description that\n> showed how to get 20% better performance while searching the git\n> repository itself for something like 4 digits (as used in Copyright\n> dates).\n\nGreat!\n\n> My idea was to reply to my own post with instructions on how to test\n> this, which basically require to also compile the recently released\n> 10.43RC1 and build against that.\n\nOK, so this is about the grep.perl.digit feature, right?\n\nWhat I meant above was: Is the statement in acabd2048e (grep: correctly\nidentify utf-8 characters with \\{b,w} in -P, 2023-01-08) about 20-40%\nperformance impact (in which direction, by the way) still measurable\nwith the fixed perf test script?\n\nWith the fix and PCRE2 10.42 and PCRE2_UCP I get:\n\nTest                           this tree\n----------------------------------------------\n7822.1: grep -P '\\bhow'        0.05(0.02+0.30)\n7822.2: grep -P '\\bÆvar'       0.05(0.02+0.29)\n7822.3: grep -P '\\d+ \\bÆvar'   0.05(0.02+0.27)\n7822.4: grep -P '\\bBelón\\b'    0.04(0.02+0.23)\n7822.5: grep -P '\\w{12}\\b'     0.03(0.02+0.13)\n\n... and without PCRE2_UCP:\n\nTest                           this tree\n----------------------------------------------\n7822.1: grep -P '\\bhow'        0.05(0.02+0.26)\n7822.2: grep -P '\\bÆvar'       0.04(0.02+0.18)\n7822.3: grep -P '\\d+ \\bÆvar'   0.05(0.02+0.26)\n7822.4: grep -P '\\bBelón\\b'    0.05(0.02+0.27)\n7822.5: grep -P '\\w{12}\\b'     0.04(0.02+0.18)\n\nHmm, inconclusive.  Perhaps the test data is too small?\n\n> Indeed, AFAIK the test would fail if built with an older version of\n> PCRE, but wasn't sure if making a prerequisite that hardcoded the\n> version in test-tool might be the best approach to prevent that,\n> especially with all the libification work.\n\nYou could extend test-pcre2-config to report whether that feature is\nactive.  Performance tests could set prerequisites based on its output.\n\n> PS. your reply got lost somehow in my SPAM folder, so I apologize for\n> the late reply\n\nNo worries, I need days to reply without any detour through the\nspam folder..\n\nRené\n"},{"id":"498448","messageId":"4e2de5cc-e438-4975-9630-a1b40b66ae1a@grsecurity.net","threadId":"60675","inReplyTo":"CAPUEspgbVx6wbp4UNjjxFO8iNJw3i2FnJxNwn4pk5EbaAP-7gQ@mail.gmail.com","subject":"Re: [RFC PATCH] grep: default to posix digits with -P","fromName":"Mathias Krause","fromEmail":"minipli@grsecurity.net","sentAt":"2024-07-10T13:49:28Z","receivedAt":"2024-07-10T13:49:31Z","isPatch":true,"sender":{"key":"minipli@grsecurity.net","avatar":"https://avatars.githubusercontent.com/u/65352095?v=4"},"body":"On 02.01.24 20:02, Carlo Arenas wrote:\n> On Mon, Jan 1, 2024 at 9:18 AM René Scharfe <l.s.r@web.de> wrote:\n>> Am 01.01.24 um 16:03 schrieb Carlo Marcelo Arenas Belón:\n>>> @@ -321,17 +327,22 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt\n>>>               }\n>>>               options |= PCRE2_CASELESS;\n>>>       }\n>>> -     if (!opt->ignore_locale && is_utf8_locale() && !literal)\n>>> -             options |= (PCRE2_UTF | PCRE2_UCP | PCRE2_MATCH_INVALID_UTF);\n>>> +     if (!opt->ignore_locale && is_utf8_locale() && !literal) {\n>>> +             options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>>>\n>>> -#ifndef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n>>> -     /*\n>>> -      * Work around a JIT bug related to invalid Unicode character handling\n>>> -      * fixed in 10.35:\n>>> -      * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n>>> -      */\n>>> -     options &= ~PCRE2_UCP;\n>>> +#ifdef GIT_PCRE2_VERSION_10_35_OR_HIGHER\n>>> +             /*\n>>> +              * Work around a JIT bug related to invalid Unicode character handling\n>>> +              * fixed in 10.35:\n>>> +              * https://github.com/PCRE2Project/pcre2/commit/c21bd977547d\n>>> +              */\n>>> +             options |= PCRE2_UCP;\n>>> +#ifdef GIT_PCRE2_VERSION_10_43_OR_HIGHER\n>>> +             if (!opt->perl_digit)\n>>> +                     xoptions |= (PCRE2_EXTRA_ASCII_BSD | PCRE2_EXTRA_ASCII_DIGIT);\n>>>  #endif\n>>> +#endif\n>>\n>> Why do we need that extra level of indentation?\n> \n> I was just trying to simplify the code by including all the logic in\n> one single set.\n> \n> The original lack of indentation that was introduced by later fixes to\n> the code was IMHO also misguided since the obvious \"objective\" as set\n> in the original code that added PCRE2_UCP was that it should be used\n> whenever UTF was also in use as shown by\n> acabd2048ee0ee53728100408970ab45a6dab65e.\n> \n> Of course, we soon found out that the original implementation of\n> PCRE2_MATCH_INVALID_UTF that came with PCRE2 10.34 was buggy and so an\n> exception was introduced in 14b9a044798ebb3858a1f1a1377309a3d6054ac8.\n\nMy early fix attempt[1] also had it indented but Junio argued against\nit[2]. I see no reason why we should change that now?\n\n[1]\nhttps://lore.kernel.org/git/20230323144000.21146-1-minipli@grsecurity.net/\n[2] https://lore.kernel.org/git/xmqq355va1a2.fsf@gitster.g/\n\n> \n> Note that the problematic code is only relevant when JIT is also\n> enabled, but JIT is almost always enabled.\n\nRight. But it doesn't hurt to mask a bit that isn't set, the compiler\nwill figure, I guess.\n\n> \n>> The old code turned PCRE2_UCP on by default and turned it off for older\n>> versions. The new code enables PCRE2_UCP only for newer versions.  The\n>> result should be the same, no?  So why change that part at all?\n> \n> Because it gets us a little closer to the real reason why we need to\n> disable UCP for anything older than 10.35, and that is that there is a\n> bug there that is ONLY relevant if we are using JIT.\n> \n> My hope though is that with the release of 10.43 (currently in RC1),\n> 10.34 will become irrelevant soon enough and this whole code could be\n> cleaned up further.\n> \n>> But the comment is now off, isn't it?  The workaround was turning\n>> PCRE2_UCP off for older versions (because those were broken), not\n>> turning it on for newer versions (because it would be required by some\n>> unfixed regression).\n> \n> The comment was never correct, because it was turning it off, because\n> the combination of JIT + MATCH_INVALID_UTF (which was released in\n> 10.34) + UCP is broken.\n\nAnd what makes the comment wrong? It's mentioning \"JIT\", \"invalid\nUnicode character handling\", \"bug\" and even the URL to the PCRE2 commit\nfixing the bug.\n\nMoreover is your proposed change making the comment look wrong as it's\nnegating the preprocessor test and sets the PCRE2_UCP bit instead of\nmasking it, suggesting *this* makes it work around the bug, while it's\nactually the opposite.\n\nSo, yes, IMHO we should leave that part as-is.\n\n> [snip]\n\nCheers,\nMathias\n"}]}