{"thread":{"id":"56723","subject":"[PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","startedAt":"2021-10-15T16:14:03Z","lastAt":"2021-11-19T17:33:35Z","messageCount":47,"participants":["Hamza Mahfooz","Junio C Hamano","René Scharfe","Ævar Arnfjörð Bjarmason","Andreas Schwab","Carlo Arenas","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":13,"patchTotal":3},"messages":[{"id":"438860","messageId":"20211015161356.3372-1-someguy@effective-light.com","threadId":"56723","inReplyTo":null,"subject":"[PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-10-15T16:13:54Z","receivedAt":"2021-10-15T16:14:03Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"These changes are made in preparation of, the colorization support for the\n\"git log\" subcommands that, rely on regex functionality (i.e. \"--author\",\n\"--committer\" and \"--grep\"). These changes are necessary primarily because\nmatch_one_pattern() expects header lines to be prefixed, however, in\npretty, the prefixes are stripped from the lines because the name-email\npairs need to go through additional parsing, before they can be printed and\nbecause next_match() doesn't handle the case of\n\"ctx == GREP_CONTEXT_HEAD\" at all. So, teach next_match() how to handle the\nnew case and move match_one_pattern()'s core logic to\nheaderless_match_one_pattern() while preserving match_one_pattern()'s uses\nthat depend on the additional processing.\n\nSigned-off-by: Hamza Mahfooz \n---\nv5: separate grep changes from pretty changes.\n\nv6: rescope some variables.\n\nv7: export header_field[] and allow for subsequent matches on header lines\n in match_one_pattern().\n\nv8: add headerless_match_one_pattern() and move header_field[] back.\n\nv9: get rid of the new check in headerless_match_one_pattern(), move the\n header pattern filtering logic in grep_next_match() and document\n grep_next_match() in grep.h.\n\nv10: add a \"magic\" comment in grep_next_match() to signify a fall through\n in the switch statement.\n---\n grep.c | 79 ++++++++++++++++++++++++++++++++++++----------------------\n grep.h | 9 +++++++\n 2 files changed, 58 insertions(+), 30 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 14fe8a0fd2..fe847a0111 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -944,10 +944,10 @@ static struct {\n \t{ \"reflog \", 7 },\n };\n \n-static int match_one_pattern(struct grep_pat *p,\n-\t\t\t const char *bol, const char *eol,\n-\t\t\t enum grep_context ctx,\n-\t\t\t regmatch_t *pmatch, int eflags)\n+static int headerless_match_one_pattern(struct grep_pat *p,\n+\t\t\t\t\tconst char *bol, const char *eol,\n+\t\t\t\t\tenum grep_context ctx,\n+\t\t\t\t\tregmatch_t *pmatch, int eflags)\n {\n \tint hit = 0;\n \tconst char *start = bol;\n@@ -956,25 +956,6 @@ static int match_one_pattern(struct grep_pat *p,\n \t ((p->token == GREP_PATTERN_HEAD) != (ctx == GREP_CONTEXT_HEAD)))\n \t\treturn 0;\n \n-\tif (p->token == GREP_PATTERN_HEAD) {\n-\t\tconst char *field;\n-\t\tsize_t len;\n-\t\tassert(p->field < ARRAY_SIZE(header_field));\n-\t\tfield = header_field[p->field].field;\n-\t\tlen = header_field[p->field].len;\n-\t\tif (strncmp(bol, field, len))\n-\t\t\treturn 0;\n-\t\tbol += len;\n-\t\tswitch (p->field) {\n-\t\tcase GREP_HEADER_AUTHOR:\n-\t\tcase GREP_HEADER_COMMITTER:\n-\t\t\tstrip_timestamp(bol, &eol);\n-\t\t\tbreak;\n-\t\tdefault:\n-\t\t\tbreak;\n-\t\t}\n-\t}\n-\n again:\n \thit = patmatch(p, bol, eol, pmatch, eflags);\n \n@@ -1025,6 +1006,36 @@ static int match_one_pattern(struct grep_pat *p,\n \treturn hit;\n }\n \n+static int match_one_pattern(struct grep_pat *p,\n+\t\t\t const char *bol, const char *eol,\n+\t\t\t enum grep_context ctx, regmatch_t *pmatch,\n+\t\t\t int eflags)\n+{\n+\tconst char *field;\n+\tsize_t len;\n+\n+\tif (p->token == GREP_PATTERN_HEAD) {\n+\t\tassert(p->field < ARRAY_SIZE(header_field));\n+\t\tfield = header_field[p->field].field;\n+\t\tlen = header_field[p->field].len;\n+\t\tif (strncmp(bol, field, len))\n+\t\t\treturn 0;\n+\t\tbol += len;\n+\n+\t\tswitch (p->field) {\n+\t\tcase GREP_HEADER_AUTHOR:\n+\t\tcase GREP_HEADER_COMMITTER:\n+\t\t\tstrip_timestamp(bol, &eol);\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\n+\treturn headerless_match_one_pattern(p, bol, eol, ctx, pmatch, eflags);\n+}\n+\n+\n static int match_expr_eval(struct grep_opt *opt, struct grep_expr *x,\n \t\t\t const char *bol, const char *eol,\n \t\t\t enum grep_context ctx, ssize_t *col,\n@@ -1143,7 +1154,7 @@ static int match_next_pattern(struct grep_pat *p,\n {\n \tregmatch_t match;\n \n-\tif (!match_one_pattern(p, bol, eol, ctx, &match, eflags))\n+\tif (!headerless_match_one_pattern(p, bol, eol, ctx, &match, eflags))\n \t\treturn 0;\n \tif (match.rm_so < 0 || match.rm_eo < 0)\n \t\treturn 0;\n@@ -1158,19 +1169,26 @@ static int match_next_pattern(struct grep_pat *p,\n \treturn 1;\n }\n \n-static int next_match(struct grep_opt *opt,\n-\t\t const char *bol, const char *eol,\n-\t\t enum grep_context ctx, regmatch_t *pmatch, int eflags)\n+int grep_next_match(struct grep_opt *opt,\n+\t\t const char *bol, const char *eol,\n+\t\t enum grep_context ctx, regmatch_t *pmatch,\n+\t\t enum grep_header_field field, int eflags)\n {\n \tstruct grep_pat *p;\n \tint hit = 0;\n \n \tpmatch->rm_so = pmatch->rm_eo = -1;\n \tif (bol < eol) {\n-\t\tfor (p = opt->pattern_list; p; p = p->next) {\n+\t\tfor (p = ((ctx == GREP_CONTEXT_HEAD)\n+\t\t\t ? opt->header_list : opt->pattern_list);\n+\t\t\t p; p = p->next) {\n \t\t\tswitch (p->token) {\n-\t\t\tcase GREP_PATTERN: /* atom */\n \t\t\tcase GREP_PATTERN_HEAD:\n+\t\t\t\tif ((field != GREP_HEADER_FIELD_MAX) &&\n+\t\t\t\t (p->field != field))\n+\t\t\t\t\tcontinue;\n+\t\t\t\t/* fall thru */\n+\t\t\tcase GREP_PATTERN: /* atom */\n \t\t\tcase GREP_PATTERN_BODY:\n \t\t\t\thit |= match_next_pattern(p, bol, eol, ctx,\n \t\t\t\t\t\t\t pmatch, eflags);\n@@ -1261,7 +1279,8 @@ static void show_line(struct grep_opt *opt,\n \t\t\telse if (sign == '=')\n \t\t\t\tline_color = opt->colors[GREP_COLOR_FUNCTION];\n \t\t}\n-\t\twhile (next_match(opt, bol, eol, ctx, &match, eflags)) {\n+\t\twhile (grep_next_match(opt, bol, eol, ctx, &match,\n+\t\t\t\t GREP_HEADER_FIELD_MAX, eflags)) {\n \t\t\tif (match.rm_so == match.rm_eo)\n \t\t\t\tbreak;\n \ndiff --git a/grep.h b/grep.h\nindex 3cb8a83ae8..808ad76f0c 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -191,6 +191,15 @@ void compile_grep_patterns(struct grep_opt *opt);\n void free_grep_patterns(struct grep_opt *opt);\n int grep_buffer(struct grep_opt *opt, const char *buf, unsigned long size);\n \n+/* The field parameter is only used to filter header patterns\n+ * (where appropriate). If filtering isn't desirable\n+ * GREP_HEADER_FIELD_MAX should be supplied.\n+ */\n+int grep_next_match(struct grep_opt *opt,\n+\t\t const char *bol, const char *eol,\n+\t\t enum grep_context ctx, regmatch_t *pmatch,\n+\t\t enum grep_header_field field, int eflags);\n+\n struct grep_source {\n \tchar *name;\n \n-- \n2.33.0\n\n"},{"id":"438861","messageId":"20211015161356.3372-3-someguy@effective-light.com","threadId":"56723","inReplyTo":"20211015161356.3372-1-someguy@effective-light.com","subject":"[PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-10-15T16:13:56Z","receivedAt":"2021-10-15T16:14:09Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"If we attempt to grep non-ascii log message text with an ascii pattern, we\nrun into the following issue:\n\n $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author\n grep: (standard input): binary file matches\n\nSo, to fix this teach the grep code to use PCRE2_UTF, as long as the log\noutput is encoded in UTF-8.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: Hamza Mahfooz \n---\nv12: get rid of utf8_all_the_things and fix an issue with one of the unit\n tests.\n\nv13: make some clarifications in the commit message.\n---\n grep.c | 6 +++--\n t/t7812-grep-icase-non-ascii.sh | 48 +++++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex fe847a0111..f6e113e9f0 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -382,8 +382,10 @@ 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() && has_non_ascii(p->pattern) &&\n-\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n+\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n+\t (!opt->ignore_locale && is_utf8_locale() &&\n+\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n+\t\t\t\t\t (p->fixed || p->is_fixed))))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\ndiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\nindex e5d1e4ea68..22487d90fd 100755\n--- a/t/t7812-grep-icase-non-ascii.sh\n+++ b/t/t7812-grep-icase-non-ascii.sh\n@@ -53,6 +53,54 @@ test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii pattern on UTF-8 data' '\n+\tcat >expected <<-\\EOF &&\n+\tAuthor: À Ú Thor \n+\tEOF\n+\ttest_write_lines \"forth\" >file4 &&\n+\tgit add file4 &&\n+\tgit commit --author=\"À Ú Thor \" -m sécond &&\n+\tgit log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n+\tgrep Author log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success GETTEXT_LOCALE,PCRE 'log --committer with an ascii pattern on ISO-8859-1 data' '\n+\tcat >expected <<-\\EOF &&\n+\tCommit: Ç O Mîtter \n+\tEOF\n+\ttest_write_lines \"fifth\" >file5 &&\n+\tgit add file5 &&\n+\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n+\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n+\tgit -c i18n.commitEncoding=latin1 commit -m thïrd &&\n+\tgit -c i18n.logOutputEncoding=latin1 log -1 --pretty=fuller --color=always --perl-regexp --committer=\" O.*\" >log &&\n+\tgrep Commit: log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success GETTEXT_LOCALE,PCRE 'log --grep with an ascii pattern on UTF-8 data' '\n+\tcat >expected <<-\\EOF &&\n+\t sécond\n+\tEOF\n+\tgit log -1 --color=always --perl-regexp --grep=\"con\" >log &&\n+\tgrep con log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success GETTEXT_LOCALE,PCRE 'log --grep with an ascii pattern on ISO-8859-1 data' '\n+\tcat >expected <<-\\EOF &&\n+\t thïrd\n+\tEOF\n+\tgit -c i18n.logOutputEncoding=latin1 log -1 --color=always --perl-regexp --grep=\"th.*rd\" >log &&\n+\tgrep \"th.*rd\" log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: setup invalid UTF-8 data' '\n \tprintf \"\\\\200\\\\n\" >invalid-0x80 &&\n \techo \"ævar\" >expected &&\n-- \n2.33.0\n\n"},{"id":"438862","messageId":"20211015161356.3372-2-someguy@effective-light.com","threadId":"56723","inReplyTo":"20211015161356.3372-1-someguy@effective-light.com","subject":"[PATCH v13 2/3] pretty: colorize pattern matches in commit messages","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-10-15T16:13:55Z","receivedAt":"2021-10-15T16:14:10Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"The \"git log\" command limits its output to the commits that contain strings\nmatched by a pattern when the \"--grep=\" option is used, but unlike\noutput from \"git grep -e \", the matches are not highlighted,\nmaking them harder to spot.\n\nTeach the pretty-printer code to highlight matches from the\n\"--grep=\", \"--author=\" and \"--committer=\"\noptions (to view the last one, you may have to ask for --pretty=fuller).\n\nAlso, it must be noted that we are effectively greping the content twice\n(because it would be a hassle to rework the existing matching code to do\na /g match and then pass it all down to the coloring code), however it only\nslows down \"git log --author=^H\" on this repository by around 1-2%\n(compared to v2.33.0), so it should be a small enough slow down to justify\nthe addition of the feature.\n\nSigned-off-by: Hamza Mahfooz \n---\nv2: make the commit message whole (add the missing ingredients), rename\n append_matched_line() to append_line_with_color(), use\n colors[GREP_COLOR_MATCH_SELECTED] instead of\n colors[GREP_COLOR_MATCH_CONTEXT], allow the background color to be\n customized, don't copy strings to a buffer when not coloring in\n append_line_with_color(), rename next_match() to grep_next_match(),\n repurpose grep_next_match()/match_one_pattern() for use in\n append_line_with_color() (allowing us to remove duplicated matching\n code in append_line_with_color()), document how to customize the\n feature and modify some of the tests to fit the feature better.\n\nv3: fix a formatting issue with the added documentation.\n\nv4: add strbuf_add_with_color(), use the correct color code scheme in the\n unit tests and add more unit tests.\n\nv5: separate grep changes from pretty changes and add some performance\n analysis in the commit message.\n\nv6: put the documentation in the correct place, cleanup pretty.c and\n format the unit tests according to the current convention.\n\nv7: get rid of all manual strbuf management, constify where now appropriate\n and fix the header line prefix issue properly.\n\nv8: remove code that relies on grep_header_fields[].\n\nv11: clarify why we are grepping twice.\n---\n Documentation/config/color.txt | 7 ++-\n pretty.c | 101 +++++++++++++++++++++++++++++----\n t/t4202-log.sh | 51 +++++++++++++++++\n 3 files changed, 145 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/config/color.txt b/Documentation/config/color.txt\nindex e05d520a86..91d9a9da32 100644\n--- a/Documentation/config/color.txt\n+++ b/Documentation/config/color.txt\n@@ -104,9 +104,12 @@ color.grep.::\n `matchContext`;;\n \tmatching text in context lines\n `matchSelected`;;\n-\tmatching text in selected lines\n+\tmatching text in selected lines. Also, used to customize the following\n+\tlinkgit:git-log[1] subcommands: `--grep`, `--author` and `--committer`.\n `selected`;;\n-\tnon-matching text in selected lines\n+\tnon-matching text in selected lines. Also, used to customize the\n+\tfollowing linkgit:git-log[1] subcommands: `--grep`, `--author` and\n+\t`--committer`.\n `separator`;;\n \tseparators between fields on a line (`:`, `-`, and `=`)\n \tand between hunks (`--`)\ndiff --git a/pretty.c b/pretty.c\nindex 73b5ead509..2dd94af886 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -431,6 +431,52 @@ const char *show_ident_date(const struct ident_split *ident,\n \treturn show_date(date, tz, mode);\n }\n \n+static inline void strbuf_add_with_color(struct strbuf *sb, const char *color,\n+\t\t\t\t\t const char *buf, size_t buflen)\n+{\n+\tstrbuf_addstr(sb, color);\n+\tstrbuf_add(sb, buf, buflen);\n+\tif (*color)\n+\t\tstrbuf_addstr(sb, GIT_COLOR_RESET);\n+}\n+\n+static void append_line_with_color(struct strbuf *sb, struct grep_opt *opt,\n+\t\t\t\t const char *line, size_t linelen,\n+\t\t\t\t int color, enum grep_context ctx,\n+\t\t\t\t enum grep_header_field field)\n+{\n+\tconst char *buf, *eol, *line_color, *match_color;\n+\tregmatch_t match;\n+\tint eflags = 0;\n+\n+\tbuf = line;\n+\teol = buf + linelen;\n+\n+\tif (!opt || !want_color(color) || opt->invert)\n+\t\tgoto end;\n+\n+\tline_color = opt->colors[GREP_COLOR_SELECTED];\n+\tmatch_color = opt->colors[GREP_COLOR_MATCH_SELECTED];\n+\n+\twhile (grep_next_match(opt, buf, eol, ctx, &match, field, eflags)) {\n+\t\tif (match.rm_so == match.rm_eo)\n+\t\t\tbreak;\n+\n+\t\tstrbuf_add_with_color(sb, line_color, buf, match.rm_so);\n+\t\tstrbuf_add_with_color(sb, match_color, buf + match.rm_so,\n+\t\t\t\t match.rm_eo - match.rm_so);\n+\t\tbuf += match.rm_eo;\n+\t\teflags = REG_NOTBOL;\n+\t}\n+\n+\tif (eflags)\n+\t\tstrbuf_add_with_color(sb, line_color, buf, eol - buf);\n+\telse {\n+end:\n+\t\tstrbuf_add(sb, buf, eol - buf);\n+\t}\n+}\n+\n void pp_user_info(struct pretty_print_context *pp,\n \t\t const char *what, struct strbuf *sb,\n \t\t const char *line, const char *encoding)\n@@ -496,9 +542,26 @@ void pp_user_info(struct pretty_print_context *pp,\n \t\t\tstrbuf_addch(sb, '\\n');\n \t\tstrbuf_addf(sb, \" <%.*s>\\n\", (int)maillen, mailbuf);\n \t} else {\n-\t\tstrbuf_addf(sb, \"%s: %.*s%.*s <%.*s>\\n\", what,\n-\t\t\t (pp->fmt == CMIT_FMT_FULLER) ? 4 : 0, \" \",\n-\t\t\t (int)namelen, namebuf, (int)maillen, mailbuf);\n+\t\tstruct strbuf id = STRBUF_INIT;\n+\t\tenum grep_header_field field = GREP_HEADER_FIELD_MAX;\n+\t\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n+\n+\t\tif (!strcmp(what, \"Author\"))\n+\t\t\tfield = GREP_HEADER_AUTHOR;\n+\t\telse if (!strcmp(what, \"Commit\"))\n+\t\t\tfield = GREP_HEADER_COMMITTER;\n+\n+\t\tstrbuf_addf(sb, \"%s: \", what);\n+\t\tif (pp->fmt == CMIT_FMT_FULLER)\n+\t\t\tstrbuf_addchars(sb, ' ', 4);\n+\n+\t\tstrbuf_addf(&id, \"%.*s <%.*s>\", (int)namelen, namebuf,\n+\t\t\t (int)maillen, mailbuf);\n+\n+\t\tappend_line_with_color(sb, opt, id.buf, id.len, pp->color,\n+\t\t\t\t GREP_CONTEXT_HEAD, field);\n+\t\tstrbuf_addch(sb, '\\n');\n+\t\tstrbuf_release(&id);\n \t}\n \n \tswitch (pp->fmt) {\n@@ -1939,8 +2002,9 @@ static int pp_utf8_width(const char *start, const char *end)\n \treturn width;\n }\n \n-static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n-\t\t\t\t const char *line, int linelen)\n+static void strbuf_add_tabexpand(struct strbuf *sb, struct grep_opt *opt,\n+\t\t\t\t int color, int tabwidth, const char *line,\n+\t\t\t\t int linelen)\n {\n \tconst char *tab;\n \n@@ -1957,7 +2021,9 @@ static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n \t\t\tbreak;\n \n \t\t/* Output the data .. */\n-\t\tstrbuf_add(sb, line, tab - line);\n+\t\tappend_line_with_color(sb, opt, line, tab - line, color,\n+\t\t\t\t GREP_CONTEXT_BODY,\n+\t\t\t\t GREP_HEADER_FIELD_MAX);\n \n \t\t/* .. and the de-tabified tab */\n \t\tstrbuf_addchars(sb, ' ', tabwidth - (width % tabwidth));\n@@ -1972,7 +2038,8 @@ static void strbuf_add_tabexpand(struct strbuf *sb, int tabwidth,\n \t * worrying about width - there's nothing more to\n \t * align.\n \t */\n-\tstrbuf_add(sb, line, linelen);\n+\tappend_line_with_color(sb, opt, line, linelen, color, GREP_CONTEXT_BODY,\n+\t\t\t GREP_HEADER_FIELD_MAX);\n }\n \n /*\n@@ -1984,11 +2051,16 @@ static void pp_handle_indent(struct pretty_print_context *pp,\n \t\t\t struct strbuf *sb, int indent,\n \t\t\t const char *line, int linelen)\n {\n+\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n+\n \tstrbuf_addchars(sb, ' ', indent);\n \tif (pp->expand_tabs_in_log)\n-\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log, line, linelen);\n+\t\tstrbuf_add_tabexpand(sb, opt, pp->color, pp->expand_tabs_in_log,\n+\t\t\t\t line, linelen);\n \telse\n-\t\tstrbuf_add(sb, line, linelen);\n+\t\tappend_line_with_color(sb, opt, line, linelen, pp->color,\n+\t\t\t\t GREP_CONTEXT_BODY,\n+\t\t\t\t GREP_HEADER_FIELD_MAX);\n }\n \n static int is_mboxrd_from(const char *line, int len)\n@@ -2006,7 +2078,9 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\t struct strbuf *sb,\n \t\t int indent)\n {\n+\tstruct grep_opt *opt = pp->rev ? &pp->rev->grep_filter : NULL;\n \tint first = 1;\n+\n \tfor (;;) {\n \t\tconst char *line = *msg_p;\n \t\tint linelen = get_one_line(line);\n@@ -2027,14 +2101,17 @@ void pp_remainder(struct pretty_print_context *pp,\n \t\tif (indent)\n \t\t\tpp_handle_indent(pp, sb, indent, line, linelen);\n \t\telse if (pp->expand_tabs_in_log)\n-\t\t\tstrbuf_add_tabexpand(sb, pp->expand_tabs_in_log,\n-\t\t\t\t\t line, linelen);\n+\t\t\tstrbuf_add_tabexpand(sb, opt, pp->color,\n+\t\t\t\t\t pp->expand_tabs_in_log, line,\n+\t\t\t\t\t linelen);\n \t\telse {\n \t\t\tif (pp->fmt == CMIT_FMT_MBOXRD &&\n \t\t\t\t\tis_mboxrd_from(line, linelen))\n \t\t\t\tstrbuf_addch(sb, '>');\n \n-\t\t\tstrbuf_add(sb, line, linelen);\n+\t\t\tappend_line_with_color(sb, opt, line, linelen,\n+\t\t\t\t\t pp->color, GREP_CONTEXT_BODY,\n+\t\t\t\t\t GREP_HEADER_FIELD_MAX);\n \t\t}\n \t\tstrbuf_addch(sb, '\\n');\n \t}\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 9dfead936b..3d240bba57 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -449,6 +449,57 @@ test_expect_success !FAIL_PREREQS 'log with various grep.patternType configurati\n \t)\n '\n \n+test_expect_success 'log --author' '\n+\tcat >expect <<-\\EOF &&\n+\tAuthor: A U Thor \n+\tEOF\n+\tgit log -1 --color=always --author=\"A U\" >log &&\n+\tgrep Author log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log --committer' '\n+\tcat >expect <<-\\EOF &&\n+\tCommit: C O Mitter example.com>\n+\tEOF\n+\tgit log -1 --color=always --pretty=fuller --committer=\"example\" >log &&\n+\tgrep \"Commit:\" log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'log -i --grep with color' '\n+\tcat >expect <<-\\EOF &&\n+\t Second\n+\t second\n+\tEOF\n+\tgit log --color=always -i --grep=^sec >log &&\n+\tgrep -i sec log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '-c color.grep.selected log --grep' '\n+\tcat >expect <<-\\EOF &&\n+\t third\n+\tEOF\n+\tgit -c color.grep.selected=\"green\" log --color=always --grep=ir >log &&\n+\tgrep ir log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '-c color.grep.matchSelected log --grep' '\n+\tcat >expect <<-\\EOF &&\n+\t initial\n+\tEOF\n+\tgit -c color.grep.matchSelected=\"blue\" log --color=always --grep=i >log &&\n+\tgrep al log >actual.raw &&\n+\ttest_decode_color actual &&\n+\ttest_cmp expect actual\n+'\n+\n cat > expect < writes:\n\n> These changes are made in preparation of, the colorization support for the\n> \"git log\" subcommands that, rely on regex functionality (i.e. \"--author\",\n> \"--committer\" and \"--grep\"). These changes are necessary primarily because\n> match_one_pattern() expects header lines to be prefixed, however, in\n> pretty, the prefixes are stripped from the lines because the name-email\n> pairs need to go through additional parsing, before they can be printed and\n> because next_match() doesn't handle the case of\n> \"ctx == GREP_CONTEXT_HEAD\" at all. So, teach next_match() how to handle the\n> new case and move match_one_pattern()'s core logic to\n> headerless_match_one_pattern() while preserving match_one_pattern()'s uses\n> that depend on the additional processing.\n>\n> Signed-off-by: Hamza Mahfooz \n> ---\n> v5: separate grep changes from pretty changes.\n>\n> v6: rescope some variables.\n>\n> v7: export header_field[] and allow for subsequent matches on header lines\n> in match_one_pattern().\n>\n> v8: add headerless_match_one_pattern() and move header_field[] back.\n>\n> v9: get rid of the new check in headerless_match_one_pattern(), move the\n> header pattern filtering logic in grep_next_match() and document\n> grep_next_match() in grep.h.\n>\n> v10: add a \"magic\" comment in grep_next_match() to signify a fall through\n> in the switch statement.\n\nMakes readers curious what happend in v11 and later...\n"},{"id":"438891","messageId":"K4711R.RVTNDQG3R0UQ3@effective-light.com","threadId":"56723","inReplyTo":"xmqqwnme5fkr.fsf@gitster.g","subject":"Re: [PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-10-15T18:24:20Z","receivedAt":"2021-10-15T18:24:32Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"\nOn Fri, Oct 15 2021 at 11:05:24 AM -0700, Junio C Hamano \n wrote:\n> Makes readers curious what happend in v11 and later...\n\nIt was my understanding that no comment means nothing changed since \nthen, or is something to the effect of \"no changes\" preferable?\n\n\n"},{"id":"438897","messageId":"xmqqk0ie5bpx.fsf@gitster.g","threadId":"56723","inReplyTo":"K4711R.RVTNDQG3R0UQ3@effective-light.com","subject":"Re: [PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T19:28:42Z","receivedAt":"2021-10-15T19:28:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hamza Mahfooz writes:\n\n> On Fri, Oct 15 2021 at 11:05:24 AM -0700, Junio C Hamano\n> wrote:\n>> Makes readers curious what happend in v11 and later...\n>\n> It was my understanding that no comment means nothing changed since\n> then, or is something to the effect of \"no changes\" preferable?\n\nUsually people do not want to spam the list with a multi-patch\nseries when they have nothing new to show, unless there are other\nreasons to do so, like \"there is no change, but I am sending it\nagain because nobody seemed to have time reviewing the series the\nlast time\", in which case that would make a good explanation to put\nthere.\n"},{"id":"438898","messageId":"ANA11R.PYDS58RTKWNU3@effective-light.com","threadId":"56723","inReplyTo":"xmqqk0ie5bpx.fsf@gitster.g","subject":"Re: [PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-10-15T19:40:22Z","receivedAt":"2021-10-15T19:40:34Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"\nOn Fri, Oct 15 2021 at 12:28:42 PM -0700, Junio C Hamano \n wrote:\n> \n> Usually people do not want to spam the list with a multi-patch\n> series when they have nothing new to show, unless there are other\n> reasons to do so, like \"there is no change, but I am sending it\n> again because nobody seemed to have time reviewing the series the\n> last time\", in which case that would make a good explanation to put\n> there.\n\nI made changes to other patches in the series however, (I only document \nthe changes if that patch in particular has changed.)\n\n\n"},{"id":"438899","messageId":"xmqqczo65ar1.fsf@gitster.g","threadId":"56723","inReplyTo":"xmqqk0ie5bpx.fsf@gitster.g","subject":"Re: [PATCH v13 1/3] grep: refactor next_match() and match_one_pattern() for external use","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T19:49:38Z","receivedAt":"2021-10-15T19:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Hamza Mahfooz writes:\n>\n>> On Fri, Oct 15 2021 at 11:05:24 AM -0700, Junio C Hamano\n>> wrote:\n>>> Makes readers curious what happend in v11 and later...\n>>\n>> It was my understanding that no comment means nothing changed since\n>> then, or is something to the effect of \"no changes\" preferable?\n>\n> Usually people do not want to spam the list with a multi-patch\n> series when they have nothing new to show, unless there are other\n> reasons to do so, like \"there is no change, but I am sending it\n> again because nobody seemed to have time reviewing the series the\n> last time\", in which case that would make a good explanation to put\n> there.\n\nAh, I thought I was responding to a cover letter. OK, so you were\nsaying that there was no change in 1/3, but other steps may be\ndifferent.\n\nIn that case, that makes sense (even though it is more helpful to\njust say \"v13: same as v12\" or something).\n\nThanks.\n"},{"id":"438903","messageId":"xmqq4k9i5a4n.fsf@gitster.g","threadId":"56723","inReplyTo":"20211015161356.3372-3-someguy@effective-light.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-15T20:03:04Z","receivedAt":"2021-10-15T20:03:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hamza Mahfooz writes:\n\n> If we attempt to grep non-ascii log message text with an ascii pattern, we\n\n\"with an ascii pattern, when Git is built with and told to use pcre2, we\"\n\n> run into the following issue:\n>\n> $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author\n> grep: (standard input): binary file matches\n>\n> So, to fix this teach the grep code to use PCRE2_UTF, as long as the log\n> output is encoded in UTF-8.\n\n> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> +\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n> +\t (!opt->ignore_locale && is_utf8_locale() &&\n> +\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n> +\t\t\t\t\t (p->fixed || p->is_fixed))))\n\nThat's a mouthful. It is not obvious what new condition is being\nadded. I had to flip the order to see the only difference is, that\n\n> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> +\tif ((!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed))) ||\n> +\t (!opt->ignore_locale && !has_non_ascii(p->pattern)))\n\n... in addition to the case where the original condition holds, if\nwe do not say \"ignore locale\" and the pattern is ascii-only, we\napply these two option flags. And that matches what the proposed\nlog message explained as the condition the problem appears.\n\nSo,... looks good, I guess.\n\nThanks, will queue.\n\n\nAddendum.\n\nIf we were reordering pieces in the condition, I wonder if there is\na better way to reorganize it, though. The original is already\nbarely explainable with words, and with this new condition added, I\nam not sure if anybody can phrase the condition in simple words to\nothers after staring it for a few minutes. I can't.\n\nBut straightening it out is best left as a future clean-up patch,\nseparate from this series.\n\n"},{"id":"438947","messageId":"eddcbe66-b172-90b7-309e-e9ce5b5b44a4@web.de","threadId":"56723","inReplyTo":"xmqq4k9i5a4n.fsf@gitster.g","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-16T16:25:20Z","receivedAt":"2021-10-16T16:25:43Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 15.10.21 um 22:03 schrieb Junio C Hamano:\n> Hamza Mahfooz writes:\n>\n>> If we attempt to grep non-ascii log message text with an ascii pattern, we\n>\n> \"with an ascii pattern, when Git is built with and told to use pcre2, we\"\n>\n>> run into the following issue:\n>>\n>> $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author\n>> grep: (standard input): binary file matches\n\nI get no error message on macOS 11.6, but this result, with the underlined\npart in red:\n\n Author: ??var Arnfjörð Bjarmason \n ^^^^^^^^^^^^^^^^^^\n\nSo the pattern matches the second byte of a two-byte character, inserts a\ncolor code in the middle and thus produces invalid output in this case.\n\n>>\n>> So, to fix this teach the grep code to use PCRE2_UTF, as long as the log\n>> output is encoded in UTF-8.\n>\n>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>> +\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n>> +\t (!opt->ignore_locale && is_utf8_locale() &&\n>> +\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n>> +\t\t\t\t\t (p->fixed || p->is_fixed))))\n>\n> That's a mouthful. It is not obvious what new condition is being\n> added. I had to flip the order to see the only difference is, that\n>\n>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>> +\tif ((!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed))) ||\n>> +\t (!opt->ignore_locale && !has_non_ascii(p->pattern)))\n>\n> ... in addition to the case where the original condition holds, if\n> we do not say \"ignore locale\" and the pattern is ascii-only, we\n> apply these two option flags. And that matches what the proposed\n> log message explained as the condition the problem appears.\n>\n> So,... looks good, I guess.\n>\n> Thanks, will queue.\n>\n>\n> Addendum.\n>\n> If we were reordering pieces in the condition, I wonder if there is\n> a better way to reorganize it, though. The original is already\n> barely explainable with words, and with this new condition added, I\n> am not sure if anybody can phrase the condition in simple words to\n> others after staring it for a few minutes. I can't.\n>\n> But straightening it out is best left as a future clean-up patch,\n> separate from this series.\n>\n\nIt can be written as:\n\n\tliteral = !opt->ignore_case && (p->fixed || p->is_fixed);\n\tif (!opt->ignore_locale) {\n\t\tif (!has_non_ascii(p->pattern) ||\n\t\t (is_utf8_locale() && !literal))\n\t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\t}\n\nLiteral patterns are those that don't use any wildcards or case-folding.\nIf the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\npattern only consists of ASCII characters, or if the pattern is encoded\nin UTF-8 and is not just a literal pattern.\n\nHmm. Why enable PCRE2_UTF for literal patterns that consist of only\nASCII chars?\n\nThe old condition was (reformatted to better match the new one):\n\n\tif (!opt->ignore_locale) {\n\t\tif (is_utf8_locale() && has_non_ascii(p->pattern) && !literal)\n\t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\t}\n\nIntuitively I'd say the condition should be:\n\n\tif (!opt->ignore_locale && is_utf8_locale()) {\n\t\tif (has_non_ascii(p->pattern) || !literal)\n\t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\t}\n\nIf both input and pattern are encoded in UTF-8, enable PCRE2_UTF if we\nhave to match non-ASCII characters or do more than just literal\nmatching.\n\nFor literal patterns that consist only of ASCII characters we don't need\nthe cost and complication of PCRE2_UTF.\n\nMakes sense?\n\nRené\n"},{"id":"438948","messageId":"87zgr8dg8j.fsf@evledraar.gmail.com","threadId":"56723","inReplyTo":"eddcbe66-b172-90b7-309e-e9ce5b5b44a4@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-16T17:12:47Z","receivedAt":"2021-10-16T17:36:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sat, Oct 16 2021, René Scharfe wrote:\n\n> Am 15.10.21 um 22:03 schrieb Junio C Hamano:\n>> Hamza Mahfooz writes:\n>>\n>>> If we attempt to grep non-ascii log message text with an ascii pattern, we\n>>\n>> \"with an ascii pattern, when Git is built with and told to use pcre2, we\"\n>>\n>>> run into the following issue:\n>>>\n>>> $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author\n>>> grep: (standard input): binary file matches\n>\n> I get no error message on macOS 11.6, but this result, with the underlined\n> part in red:\n>\n> Author: ??var Arnfjörð Bjarmason \n> ^^^^^^^^^^^^^^^^^^\n>\n> So the pattern matches the second byte of a two-byte character, inserts a\n> color code in the middle and thus produces invalid output in this case.\n\nThanks for digging into these edge cases...\n\n>>>\n>>> So, to fix this teach the grep code to use PCRE2_UTF, as long as the log\n>>> output is encoded in UTF-8.\n>>\n>>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>>> +\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n>>> +\t (!opt->ignore_locale && is_utf8_locale() &&\n>>> +\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n>>> +\t\t\t\t\t (p->fixed || p->is_fixed))))\n>>\n>> That's a mouthful. It is not obvious what new condition is being\n>> added. I had to flip the order to see the only difference is, that\n>>\n>>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>>> +\tif ((!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed))) ||\n>>> +\t (!opt->ignore_locale && !has_non_ascii(p->pattern)))\n>>\n>> ... in addition to the case where the original condition holds, if\n>> we do not say \"ignore locale\" and the pattern is ascii-only, we\n>> apply these two option flags. And that matches what the proposed\n>> log message explained as the condition the problem appears.\n>>\n>> So,... looks good, I guess.\n>>\n>> Thanks, will queue.\n>>\n>>\n>> Addendum.\n>>\n>> If we were reordering pieces in the condition, I wonder if there is\n>> a better way to reorganize it, though. The original is already\n>> barely explainable with words, and with this new condition added, I\n>> am not sure if anybody can phrase the condition in simple words to\n>> others after staring it for a few minutes. I can't.\n>>\n>> But straightening it out is best left as a future clean-up patch,\n>> separate from this series.\n>>\n>\n> It can be written as:\n>\n> \tliteral = !opt->ignore_case && (p->fixed || p->is_fixed);\n> \tif (!opt->ignore_locale) {\n> \t\tif (!has_non_ascii(p->pattern) ||\n> \t\t (is_utf8_locale() && !literal))\n> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> \t}\n\nWhatever we go from here I'm very much for untangling that condition,\nbut I guess it can be done as a follow-up too, I'll defer to Hamza\nthere...\n\n> Literal patterns are those that don't use any wildcards or case-folding.\n> If the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\n> pattern only consists of ASCII characters, or if the pattern is encoded\n> in UTF-8 and is not just a literal pattern.\n>\n> Hmm. Why enable PCRE2_UTF for literal patterns that consist of only\n> ASCII chars?\n>\n> The old condition was (reformatted to better match the new one):\n>\n> \tif (!opt->ignore_locale) {\n> \t\tif (is_utf8_locale() && has_non_ascii(p->pattern) && !literal)\n> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> \t}\n>\n> Intuitively I'd say the condition should be:\n>\n> \tif (!opt->ignore_locale && is_utf8_locale()) {\n> \t\tif (has_non_ascii(p->pattern) || !literal)\n> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> \t}\n>\n> If both input and pattern are encoded in UTF-8, enable PCRE2_UTF if we\n> have to match non-ASCII characters or do more than just literal\n> matching.\n>\n> For literal patterns that consist only of ASCII characters we don't need\n> the cost and complication of PCRE2_UTF.\n>\n> Makes sense?\n\n echo 'René Scharfe' >f &&\n $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?Ren. S' f; echo $?\n 1\n $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?R[eé]n. S' f; echo $?\n f:René Scharfe\n 0\n\nSo it's a choose-your-own adventure where you can pick if you're\nright. I.e. do you want the \".\" metacharacter to match your \"é\" or not?\n\nThese sorts of patterns demonstrate nicely that the relationship between your\npattern being ASCII and wanting or not wanting UTF-8 matching semantics\nisn't what you might imagine it to be.\n\nIf you look at:\n\n git log --reverse -p -Gis_utf8_locale -- grep.c\n\nAnd my earlier replies in this thread-at-large (not digging up a\nreference now, sorry), you'll see that the current behavior in grep.c is\nreally a compromise based on some intersection of user patterns, us\npotentially grepping arbitrary binary data and/or \"valid\" encodings, and\nwhat PCRE does and doesn't puke on.\n\nIt's not \"right\" by any sense, but we sort of limp along with it, mostly\nbecause unlike say Perl's regex engine which really is used for serious\nproduction work where Unicode-correctness matters I don't think anyone\nis using \"git grep\" for anything like that, it's mostly for people's\nad-hoc greps.\n\nRight now I can't remember if the only reason we can't \"fix this\" is\nbecause we'd be too aggressive with the PCRE version dependency, see\n95ca1f987ed (grep/pcre2: better support invalid UTF-8 haystacks,\n2021-01-24).\n\n\n\n\n\n\n"},{"id":"438954","messageId":"fc7eb9fc-9521-5484-b05f-9c20086fd485@web.de","threadId":"56723","inReplyTo":"87zgr8dg8j.fsf@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-16T19:44:13Z","receivedAt":"2021-10-16T19:44:31Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.10.21 um 19:12 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Sat, Oct 16 2021, René Scharfe wrote:\n>\n>> Am 15.10.21 um 22:03 schrieb Junio C Hamano:\n>>> Hamza Mahfooz writes:\n>>>\n>>>> If we attempt to grep non-ascii log message text with an ascii pattern, we\n>>>\n>>> \"with an ascii pattern, when Git is built with and told to use pcre2, we\"\n>>>\n>>>> run into the following issue:\n>>>>\n>>>> $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author\n>>>> grep: (standard input): binary file matches\n>>\n>> I get no error message on macOS 11.6, but this result, with the underlined\n>> part in red:\n>>\n>> Author: ??var Arnfjörð Bjarmason \n>> ^^^^^^^^^^^^^^^^^^\n>>\n>> So the pattern matches the second byte of a two-byte character, inserts a\n>> color code in the middle and thus produces invalid output in this case.\n>\n> Thanks for digging into these edge cases...\n>\n>>>>\n>>>> So, to fix this teach the grep code to use PCRE2_UTF, as long as the log\n>>>> output is encoded in UTF-8.\n>>>\n>>>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>>>> +\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n>>>> +\t (!opt->ignore_locale && is_utf8_locale() &&\n>>>> +\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n>>>> +\t\t\t\t\t (p->fixed || p->is_fixed))))\n>>>\n>>> That's a mouthful. It is not obvious what new condition is being\n>>> added. I had to flip the order to see the only difference is, that\n>>>\n>>>> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>>> -\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>>>> +\tif ((!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n>>>> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed))) ||\n>>>> +\t (!opt->ignore_locale && !has_non_ascii(p->pattern)))\n>>>\n>>> ... in addition to the case where the original condition holds, if\n>>> we do not say \"ignore locale\" and the pattern is ascii-only, we\n>>> apply these two option flags. And that matches what the proposed\n>>> log message explained as the condition the problem appears.\n>>>\n>>> So,... looks good, I guess.\n>>>\n>>> Thanks, will queue.\n>>>\n>>>\n>>> Addendum.\n>>>\n>>> If we were reordering pieces in the condition, I wonder if there is\n>>> a better way to reorganize it, though. The original is already\n>>> barely explainable with words, and with this new condition added, I\n>>> am not sure if anybody can phrase the condition in simple words to\n>>> others after staring it for a few minutes. I can't.\n>>>\n>>> But straightening it out is best left as a future clean-up patch,\n>>> separate from this series.\n>>>\n>>\n>> It can be written as:\n>>\n>> \tliteral = !opt->ignore_case && (p->fixed || p->is_fixed);\n>> \tif (!opt->ignore_locale) {\n>> \t\tif (!has_non_ascii(p->pattern) ||\n>> \t\t (is_utf8_locale() && !literal))\n>> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>> \t}\n>\n> Whatever we go from here I'm very much for untangling that condition,\n> but I guess it can be done as a follow-up too, I'll defer to Hamza\n> there...\n>\n>> Literal patterns are those that don't use any wildcards or case-folding.\n>> If the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\n>> pattern only consists of ASCII characters, or if the pattern is encoded\n>> in UTF-8 and is not just a literal pattern.\n>>\n>> Hmm. Why enable PCRE2_UTF for literal patterns that consist of only\n>> ASCII chars?\n>>\n>> The old condition was (reformatted to better match the new one):\n>>\n>> \tif (!opt->ignore_locale) {\n>> \t\tif (is_utf8_locale() && has_non_ascii(p->pattern) && !literal)\n>> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>> \t}\n>>\n>> Intuitively I'd say the condition should be:\n>>\n>> \tif (!opt->ignore_locale && is_utf8_locale()) {\n>> \t\tif (has_non_ascii(p->pattern) || !literal)\n>> \t\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>> \t}\n>>\n>> If both input and pattern are encoded in UTF-8, enable PCRE2_UTF if we\n>> have to match non-ASCII characters or do more than just literal\n>> matching.\n>>\n>> For literal patterns that consist only of ASCII characters we don't need\n>> the cost and complication of PCRE2_UTF.\n>>\n>> Makes sense?\n>\n> echo 'René Scharfe' >f &&\n> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?Ren. S' f; echo $?\n> 1\n> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?R[eé]n. S' f; echo $?\n> f:René Scharfe\n> 0\n>\n> So it's a choose-your-own adventure where you can pick if you're\n> right. I.e. do you want the \".\" metacharacter to match your \"é\" or not?\n\nYes, I do, and it's what Hamza's patch is fixing.\n\n> These sorts of patterns demonstrate nicely that the relationship between your\n> pattern being ASCII and wanting or not wanting UTF-8 matching semantics\n> isn't what you might imagine it to be.\n\nDifferences are:\n\no: opt->ignore_locale\nh: has_non_ascii(p->pattern)\ni: is_utf8_locale()\nl: literal\n\no h i l master hamza rene\n0 0 0 0 0 1 0\n0 0 0 1 0 1 0\n0 0 1 0 0 1 1 <== your first example\n0 0 1 1 0 1 0\n0 1 1 1 0 0 1\n\nTurning on PCRE2_UTF when is_utf8_locale() == 0 seems wrong (first two\nlines).\n\nTurning on PCRE2_UTF for literal matching (fourth line) goes against\n870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,\n2019-07-26).\n\nTurning on PCRE2_UTF for literal matching of non-ASCII characters (fifth\nline) also goes against that, so my intuition betrayed me. When I\nadjust it, I get:\n\n\tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n\t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\nThat looks deceptively simple -- just drop has_non_ascii(p->pattern)\nfrom the original condition.\n\nYour second example is handle the same by all versions btw.:\n\no h i l master hamza rene\n0 1 1 0 1 1 1\n\nRené\n"},{"id":"438965","messageId":"xmqq1r4k197p.fsf@gitster.g","threadId":"56723","inReplyTo":"fc7eb9fc-9521-5484-b05f-9c20086fd485@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-17T06:00:58Z","receivedAt":"2021-10-17T06:01:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>>> Literal patterns are those that don't use any wildcards or case-folding.\n>>> If the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\n>>> pattern only consists of ASCII characters, or if the pattern is encoded\n>>> in UTF-8 and is not just a literal pattern.\n>>>\n>>> Hmm. Why enable PCRE2_UTF for literal patterns that consist of only\n>>> ASCII chars?\n>>> ...\n>> echo 'René Scharfe' >f &&\n>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?Ren. S' f; echo $?\n>> 1\n>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?R[eé]n. S' f; echo $?\n>> f:René Scharfe\n>> 0\n>>\n>> So it's a choose-your-own adventure where you can pick if you're\n>> right. I.e. do you want the \".\" metacharacter to match your \"é\" or not?\n>\n> Yes, I do, and it's what Hamza's patch is fixing.\n\nThat may be correct but is this discussion still about \"Why enable\n... for literal patterns that consist of only ASCII\"? Calling \".\" a\n\"metacharacter\" and wanting it to match anything other than a single\ndot would mean the pattern we are discussing is no longer \"literal\",\nisn't it? I am puzzled.\n\n>> These sorts of patterns demonstrate nicely that the relationship between your\n>> pattern being ASCII and wanting or not wanting UTF-8 matching semantics\n>> isn't what you might imagine it to be.\n>\n> Differences are:\n>\n> o: opt->ignore_locale\n> h: has_non_ascii(p->pattern)\n> i: is_utf8_locale()\n> l: literal\n>\n> o h i l master hamza rene\n> 0 0 0 0 0 1 0\n> 0 0 0 1 0 1 0\n> 0 0 1 0 0 1 1 <== your first example\n> 0 0 1 1 0 1 0\n> 0 1 1 1 0 0 1\n>\n> Turning on PCRE2_UTF when is_utf8_locale() == 0 seems wrong (first two\n> lines).\n>\n> Turning on PCRE2_UTF for literal matching (fourth line) goes against\n> 870eea8166 (grep: do not enter PCRE2_UTF mode on fixed matching,\n> 2019-07-26).\n>\n> Turning on PCRE2_UTF for literal matching of non-ASCII characters (fifth\n> line) also goes against that, so my intuition betrayed me. When I\n> adjust it, I get:\n>\n> \tif (!opt->ignore_locale && is_utf8_locale() && !literal)\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>\n> That looks deceptively simple -- just drop has_non_ascii(p->pattern)\n> from the original condition.\n>\n> Your second example is handle the same by all versions btw.:\n>\n> o h i l master hamza rene\n> 0 1 1 0 1 1 1\n>\n> René\n"},{"id":"438967","messageId":"0ea73e7a-6d43-e223-ab2e-24c684102856@web.de","threadId":"56723","inReplyTo":"xmqq1r4k197p.fsf@gitster.g","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-10-17T06:55:41Z","receivedAt":"2021-10-17T06:56:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 17.10.21 um 08:00 schrieb Junio C Hamano:\n> René Scharfe writes:\n>\n>>>> Literal patterns are those that don't use any wildcards or case-folding.\n>>>> If the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\n>>>> pattern only consists of ASCII characters, or if the pattern is encoded\n>>>> in UTF-8 and is not just a literal pattern.\n>>>>\n>>>> Hmm. Why enable PCRE2_UTF for literal patterns that consist of only\n>>>> ASCII chars?\n>>>> ...\n>>> echo 'René Scharfe' >f &&\n>>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?Ren. S' f; echo $?\n>>> 1\n>>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?R[eé]n. S' f; echo $?\n>>> f:René Scharfe\n>>> 0\n>>>\n>>> So it's a choose-your-own adventure where you can pick if you're\n>>> right. I.e. do you want the \".\" metacharacter to match your \"é\" or not?\n>>\n>> Yes, I do, and it's what Hamza's patch is fixing.\n>\n> That may be correct but is this discussion still about \"Why enable\n> ... for literal patterns that consist of only ASCII\"? Calling \".\" a\n> \"metacharacter\" and wanting it to match anything other than a single\n> dot would mean the pattern we are discussing is no longer \"literal\",\n> isn't it? I am puzzled.\n\nRight, Ævar's comment is not about my question, but highlights an\ninconsistency in master that is fixed by Hamza's patch.\n\nI'll repeat and extend my question: Hamza's patch enables PCRE2_UTF for\nnon-ASCII patterns even if they are literal or our locale is not UTF-8.\nThe following change would fix the edge case mentioned in its commit\nmessage without these side-effects. Am I correct?\n\ndiff --git a/grep.c b/grep.c\nindex fe847a0111..5badb6d851 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -382,7 +382,7 @@ 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() && has_non_ascii(p->pattern) &&\n+\tif (!opt->ignore_locale && is_utf8_locale() &&\n \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\n"},{"id":"438968","messageId":"87r1ckc6gf.fsf@evledraar.gmail.com","threadId":"56723","inReplyTo":"0ea73e7a-6d43-e223-ab2e-24c684102856@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-10-17T09:44:31Z","receivedAt":"2021-10-17T10:05:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Oct 17 2021, René Scharfe wrote:\n\n> Am 17.10.21 um 08:00 schrieb Junio C Hamano:\n>> René Scharfe writes:\n>>\n>>>>> Literal patterns are those that don't use any wildcards or case-folding.\n>>>>> If the text is encoded in UTF-8 then we enable PCRE2_UTF either if the\n>>>>> pattern only consists of ASCII characters, or if the pattern is encoded\n>>>>> in UTF-8 and is not just a literal pattern.\n>>>>>\n>>>>> Hmm. Why enable PCRE2_UTF for literal patterns that consist of only\n>>>>> ASCII chars?\n>>>>> ...\n>>>> echo 'René Scharfe' >f &&\n>>>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?Ren. S' f; echo $?\n>>>> 1\n>>>> $ git -P grep --no-index -P '^(?:You are (?:wrong|correct), )?R[eé]n. S' f; echo $?\n>>>> f:René Scharfe\n>>>> 0\n>>>>\n>>>> So it's a choose-your-own adventure where you can pick if you're\n>>>> right. I.e. do you want the \".\" metacharacter to match your \"é\" or not?\n>>>\n>>> Yes, I do, and it's what Hamza's patch is fixing.\n>>\n>> That may be correct but is this discussion still about \"Why enable\n>> ... for literal patterns that consist of only ASCII\"? Calling \".\" a\n>> \"metacharacter\" and wanting it to match anything other than a single\n>> dot would mean the pattern we are discussing is no longer \"literal\",\n>> isn't it? I am puzzled.\n>\n> Right, Ævar's comment is not about my question, but highlights an\n> inconsistency in master that is fixed by Hamza's patch.\n\nYes, sorry about that. Just generally about the messy semantics.\n\n> I'll repeat and extend my question: Hamza's patch enables PCRE2_UTF for\n> non-ASCII patterns even if they are literal or our locale is not UTF-8.\n> The following change would fix the edge case mentioned in its commit\n> message without these side-effects. Am I correct?\n>\n> diff --git a/grep.c b/grep.c\n> index fe847a0111..5badb6d851 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -382,7 +382,7 @@ 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() && has_non_ascii(p->pattern) &&\n> +\tif (!opt->ignore_locale && is_utf8_locale() &&\n> \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\nI haven't had time to carefully look into this, but one caveat to check\nout is if it works with older PCRE and whether you need e.g. the\nGIT_PCRE2_VERSION_10_36_OR_HIGHER.\n\nI tried your suggestion on top of Hamza's series, compiled PCRE v2\n10.23, tested, and also tried manually removing the\nPCRE2_MATCH_INVALID_UTF flag and tested again.\n\nWe pass all tests with both, so maybe this is safe to do (or maybe we're\nmissing some test we haven't thought of yet...).\n\nOne thing that makes me nervous is that we had breakages in the past\nonce the patches escaped into the wild, particularly because the code\nbeing modified here has is_utf8_locale(), and our tests run under\nLANG=c LC_ALL=C.\n\nI tried running all the tests with a non-C locale, there's a lot of\nfailures, but none new with this change. As an aside the below patch\nmakes all but one shortlog test pass for me. I wonder if we shouldn't do\nthis for real to smoke out any $LANG or $LC_ALL dependencies.\n\nI.e. almost all of the failures were due to relying on the sort order of\nsort(1), and in one case comm(1), the first hunk here is also redundant\nto defining our own ls(1) wrapper....\n\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex bd17f308b38..738ca6ef587 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -693,12 +693,12 @@ test_expect_success 'expire removes unreferenced packs' '\n \t\t^refs/heads/C\n \t\tEOF\n \t\tgit multi-pack-index write &&\n-\t\tls .git/objects/pack | grep -v -e pack-[AB] >expect &&\n+\t\tls .git/objects/pack | sort | grep -v -e pack-[AB] >expect &&\n \t\tgit multi-pack-index expire &&\n-\t\tls .git/objects/pack >actual &&\n+\t\tls .git/objects/pack | sort >actual &&\n \t\ttest_cmp expect actual &&\n-\t\tls .git/objects/pack/ | grep idx >expect-idx &&\n-\t\ttest-tool read-midx .git/objects | grep idx >actual-midx &&\n+\t\tls .git/objects/pack/ | sort | grep idx >expect-idx &&\n+\t\ttest-tool read-midx .git/objects | sort | grep idx >actual-midx &&\n \t\ttest_cmp expect-idx actual-midx &&\n \t\tgit multi-pack-index verify &&\n \t\tgit fsck\n@@ -802,7 +802,7 @@ test_expect_success 'expire works when adding new packs' '\n \t\trefs/heads/E\n \t\tEOF\n \t\tgit multi-pack-index expire &&\n-\t\tls .git/objects/pack/ | grep idx >expect &&\n+\t\tls .git/objects/pack/ | sort | grep idx >expect &&\n \t\ttest-tool read-midx .git/objects | grep idx >actual &&\n \t\ttest_cmp expect actual &&\n \t\tgit multi-pack-index verify\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 8361b5c1c57..f4f9d231f28 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -417,14 +417,22 @@ test -n \"$BASH_VERSION\" && shopt -u checkwinsize 2>/dev/null\n \n # For repeatability, reset the environment to known value.\n # TERM is sanitized below, after saving color control sequences.\n-LANG=C\n-LC_ALL=C\n+LANG=en_US.UTF-8\n+LC_ALL=en_US.UTF-8\n PAGER=cat\n TZ=UTC\n COLUMNS=80\n export LANG LC_ALL PAGER TZ COLUMNS\n EDITOR=:\n \n+sort () {\n+\tLC_ALL=C LANG=C command sort \"$@\"\n+}\n+\n+comm () {\n+\tLC_ALL=C LANG=C command comm \"$@\"\n+}\n+\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out. So keep the unsets\n # deriving from the command substitution clustered with the other\n"},{"id":"441205","messageId":"211115.86fsrxqbvp.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"877dd9i1zj.fsf@igel.home","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-15T22:41:27Z","receivedAt":"2021-11-15T23:44:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Nov 15 2021, Andreas Schwab wrote:\n\n> This breaks 'PCRE v2: grep ASCII from invalid UTF-8 data'.\n\nWhat PCREv2 version are you using? I wonder if it's got to do with the\n10.36 boundary, as noted in 95ca1f987ed (grep/pcre2: better support\ninvalid UTF-8 haystacks, 2021-01-24). One of the two tests matching your\ndescription tests for that bug.\n\nI can't remember much of the details of the bug, and it seems PCREv2's\nbug tracker went private, or at least I can't view\nhttps://bugs.exim.org/show_bug.cgi?id=2642 anymore.\n"},{"id":"441207","messageId":"877dd9i1zj.fsf@igel.home","threadId":"56723","inReplyTo":"20211015161356.3372-3-someguy@effective-light.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-11-15T20:43:28Z","receivedAt":"2021-11-16T00:18:07Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"This breaks 'PCRE v2: grep ASCII from invalid UTF-8 data'.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"441245","messageId":"CAPUEspiobsM2HYhi47zcAR+eRveF24mGA+ozUsYGpNCc-D9F+w@mail.gmail.com","threadId":"56723","inReplyTo":"211115.86fsrxqbvp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-16T02:12:27Z","receivedAt":"2021-11-16T05:12:29Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"> I can't remember much of the details of the bug, and it seems PCREv2's\n> bug tracker went private, or at least I can't view\n> https://bugs.exim.org/show_bug.cgi?id=2642 anymore.\n\nIt was shutdown; all new development is now done in github[1], old\nclosed bugs were not migrated over though but the fix is pretty\nstraight forward if it needs backporting[2]\n\nI would have expected the less risky change from René[3] to be\nreleased though, but might be a good opportunity to take a deeper look\nat the underlying problem and also have a likely solution to both the\nregression and the original bug.\n\nCarlo\n\n[1] https://github.com/PhilipHazel/pcre2\n[2] https://github.com/PhilipHazel/pcre2/commit/f8cbb1f58df05f175a6898f35dc18e26be6362d0\n[3] https://lore.kernel.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n"},{"id":"441265","messageId":"87o86kv6fh.fsf@igel.home","threadId":"56723","inReplyTo":"211115.86fsrxqbvp.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-11-16T08:41:22Z","receivedAt":"2021-11-16T08:41:30Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 15 2021, Ævar Arnfjörð Bjarmason wrote:\n\n> What PCREv2 version are you using?\n\n10.31\n\nhttps://build.opensuse.org/package/show/openSUSE:Leap:15.3:Update/pcre2\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"441267","messageId":"CAPUEspi=r9EsG8KPvdiD-HM7Drq8ho1yjkN_c_T1e+ZeR4eejg@mail.gmail.com","threadId":"56723","inReplyTo":"87o86kv6fh.fsf@igel.home","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-16T09:06:51Z","receivedAt":"2021-11-16T09:07:11Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 12:42 AM Andreas Schwab wrote:\n>\n> On Nov 15 2021, Ævar Arnfjörð Bjarmason wrote:\n>\n> > What PCREv2 version are you using?\n>\n> 10.31\n>\n> https://build.opensuse.org/package/show/openSUSE:Leap:15.3:Update/pcre2\n\ninteresting; that version doesn't need the workaround because it\ndoesn't have MATCH_INVALID_UTF that was released with 10.34, but it\nhas a recently backported fix for a similar issue that might be\nconfusing git and it is somehow related to it.\n\na full log (run it with -x -i, and get the last lines) would be useful.\n\ncurious also if you see other problems using `git log --grep` and\nwhich locale do you have configured?\n\nCarlo\n"},{"id":"441268","messageId":"87k0h8v4pu.fsf@igel.home","threadId":"56723","inReplyTo":"CAPUEspi=r9EsG8KPvdiD-HM7Drq8ho1yjkN_c_T1e+ZeR4eejg@mail.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-11-16T09:18:21Z","receivedAt":"2021-11-16T09:18:28Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 16 2021, Carlo Arenas wrote:\n\n> curious also if you see other problems using `git log --grep` and\n\nNot sure what you are asking for.\n\n> which locale do you have configured?\n\nThere is no locale configured during build.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"441269","messageId":"87fsrwv46h.fsf@igel.home","threadId":"56723","inReplyTo":"CAPUEspi=r9EsG8KPvdiD-HM7Drq8ho1yjkN_c_T1e+ZeR4eejg@mail.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-11-16T09:29:58Z","receivedAt":"2021-11-16T09:30:04Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"expecting success of 7812.13 'PCRE v2: grep ASCII from invalid UTF-8 data': \n git grep -h \"var\" invalid-0x80 >actual &&\n test_cmp expected actual &&\n git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n test_cmp expected actual\n\n++ git grep -h var invalid-0x80\n++ test_cmp expected actual\n++ test 2 -ne 2\n++ eval 'diff -u' '\"$@\"'\n+++ diff -u expected actual\n++ git grep -h '(*NO_JIT)var' invalid-0x80\nfatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\nerror: last command exited with $?=128\nnot ok 13 - PCRE v2: grep ASCII from invalid UTF-8 data\n#\n# git grep -h \"var\" invalid-0x80 >actual &&\n# test_cmp expected actual &&\n# git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n# test_cmp expected actual\n#\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"441270","messageId":"CAPUEspg8ZUdn+KFz35yG1k9bbfVTe1b+7=+WdMknRS1zu8VcDQ@mail.gmail.com","threadId":"56723","inReplyTo":"87fsrwv46h.fsf@igel.home","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-16T09:38:13Z","receivedAt":"2021-11-16T09:38:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 1:30 AM Andreas Schwab wrote:\n>\n> expecting success of 7812.13 'PCRE v2: grep ASCII from invalid UTF-8 data':\n> git grep -h \"var\" invalid-0x80 >actual &&\n> test_cmp expected actual &&\n> git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n> test_cmp expected actual\n>\n> ++ git grep -h var invalid-0x80\n> ++ test_cmp expected actual\n> ++ test 2 -ne 2\n> ++ eval 'diff -u' '\"$@\"'\n> +++ diff -u expected actual\n> ++ git grep -h '(*NO_JIT)var' invalid-0x80\n> fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\n\nThat is exactly what I was worried about, this is not failing one\ntest, but making `git grep` unusable in any repository that has any\nbinary files that might be reachable by it, and it is likely affecting\nanyone using PCRE older than 10.34\n\nReverting this specific commit might fix it and is unlikely to\nintroduce other issues, did you try it?\n\nCarlo\n"},{"id":"441287","messageId":"87bl2kv309.fsf@igel.home","threadId":"56723","inReplyTo":"CAPUEspg8ZUdn+KFz35yG1k9bbfVTe1b+7=+WdMknRS1zu8VcDQ@mail.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2021-11-16T09:55:18Z","receivedAt":"2021-11-16T09:55:28Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"On Nov 16 2021, Carlo Arenas wrote:\n\n> Reverting this specific commit might fix it and is unlikely to\n> introduce other issues, did you try it?\n\nYes, of course.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1\n\"And now for something completely different.\"\n"},{"id":"441293","messageId":"20211116110035.22140-1-carenas@gmail.com","threadId":"56723","inReplyTo":"87bl2kv309.fsf@igel.home","subject":"[PATCH] grep: avoid setting UTF mode when not needed","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-11-16T11:00:35Z","receivedAt":"2021-11-16T11:00:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns\nand UTF-8 data, 2021-10-15), PCRE_UTF mode is enabled in cases where it\nwill fail because of UTF-8 validation, which is needed for versions of\nPCRE2 older than 10.34.\n\nRevert the change on logic to avoid failures that were reported from the\ntest cases, but that should also reflect in normal use when JIT is enabled\nand could result in crashes (or worse), as UTF-8 validation is skipped.\n\nKeeping the tests, as they pass even without the fix as replicated locally\nin Debian 10 and the CI.\n\nReported-by: Andreas Schwab \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n grep.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f6e113e9f0..fe847a0111 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -382,10 +382,8 @@ 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 && !has_non_ascii(p->pattern)) ||\n-\t (!opt->ignore_locale && is_utf8_locale() &&\n-\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n-\t\t\t\t\t (p->fixed || p->is_fixed))))\n+\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n+\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n-- \n2.34.0.352.g07dee3c5e1\n\n"},{"id":"441323","messageId":"211116.86tugcp8mg.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"20211116110035.22140-1-carenas@gmail.com","subject":"Re: [PATCH] grep: avoid setting UTF mode when not needed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-16T12:32:17Z","receivedAt":"2021-11-16T12:50:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 16 2021, Carlo Marcelo Arenas Belón wrote:\n\n> Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns\n> and UTF-8 data, 2021-10-15), PCRE_UTF mode is enabled in cases where it\n> will fail because of UTF-8 validation, which is needed for versions of\n> PCRE2 older than 10.34.\n>\n> Revert the change on logic to avoid failures that were reported from the\n> test cases, but that should also reflect in normal use when JIT is enabled\n> and could result in crashes (or worse), as UTF-8 validation is skipped.\n>\n> Keeping the tests, as they pass even without the fix as replicated locally\n> in Debian 10 and the CI.\n>\n> Reported-by: Andreas Schwab \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> grep.c | 6 ++----\n> 1 file changed, 2 insertions(+), 4 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index f6e113e9f0..fe847a0111 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -382,10 +382,8 @@ 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 && !has_non_ascii(p->pattern)) ||\n> -\t (!opt->ignore_locale && is_utf8_locale() &&\n> -\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n> -\t\t\t\t\t (p->fixed || p->is_fixed))))\n> +\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> \n> #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n\nHrm.\n\nA few things:\n\nFirst, if we've got a post-PCREv2 version whatever fix let's guard that\nwith an ifdef, see thep GIT_PCRE2_VERSION_*_HIGHER at the top of grep.h.\n\nIt really helps to have those, both to know to test on those older\nversions, and also so that we can at some point in the future bump the\nrequired version and drop these workarounds entirely (as we do with\ngit-curl-compat.h).\n\nSecondly, whatever do here let's first fix the test added in ae39ba431a,\nso we're not groping around in the dark even more.\n\nI didn't spot this at the time but the test that Hamza added in that\nbased on my initial report[1] is broken & doesn't test anything\nmeaningful. It needs to have this applied:\n\t\n\tdiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\n\tindex 22487d90fdc..1da6b07a579 100755\n\t--- a/t/t7812-grep-icase-non-ascii.sh\n\t+++ b/t/t7812-grep-icase-non-ascii.sh\n\t@@ -60,7 +60,7 @@ test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii pattern on U\n\t test_write_lines \"forth\" >file4 &&\n\t git add file4 &&\n\t git commit --author=\"À Ú Thor \" -m sécond &&\n\t- git log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n\t+ git log -1 --color=always --perl-regexp --author=\". . Thor\" >log &&\n\t grep Author log >actual.raw &&\n\t test_decode_color actual &&\n\t test_cmp expected actual\n\nI.e. the whole point of using the color output to test this is to\ndiscover where PCRE2 is going to consider a character boundary to be,\nusing .* means that it won't be tested at add, since .* will happily eat\nup whatever arbitrary data it finds with or without UTF-8 mode.\n\nOther tests added in that & adjacent (if any?) commits may have the same\nissue, I haven't dug into it.\n\nIf we lead with that patch we'll get the test passing on master as\nbefore, but with your patch above it'll break. I.e. the \"when not\nneeded' in the $subject isn't true, it's just that the test is\ncompletely broken.\n\nIn the context of this being a pretty urgent post-release fix (but I\ndon't know if Junio would consider a point-release, so perhaphs it's\nnot) I'd be OK with either of:\n\n A. Let's back out this new log grep color thing entirely while we\n reconsider this. The gitster/hm/paint-hits-in-log-grep topic\n currently reverts cleanly.\n\n B. Don't break the new log grep color thing, and also fix the 'grepping\n binary' regression (which is much more important than having A)\n\nBut let's not go for some in-between where we break the new feature to\nthe point of it being worse than the state of not having it at all in\nv2.33.0.\n\nI.e. without the that log grep color feature we wouldn't screw up the\ndisplay of non-ASCII characters in log output (yay), in v2.34.0 we\ndon't, but also color the match (yay), but we broke grepping binary\n*files* (boo!).\n\nI think the approach I suggested in [2] is a much more viable way\nforward, i.e. let's stop fiddling with this giant nested if statement\nthat's mainly meant for the grep-a-file-case, revert to the\npre-log-grep-color state, and have the log-grep-color mode pass in a\n\"yes, I'd like the UTF-8 mode, please\".\n\nHaving said that that's probably also broken, just in ways we're less\nlikely to spot (or maybe some of the log encoding settings/reencoding\nsaves us there?), we may need to have that ifdef'd to 10.34 and higher,\nor have some opt-in setting for this.\n\n1. https://lore.kernel.org/git/87v92bju64.fsf@evledraar.gmail.com/\n2. https://lore.kernel.org/git/87v92bju64.fsf@evledraar.gmail.com/\n"},{"id":"441325","messageId":"R33O2R.WYXS4AQP7W05@effective-light.com","threadId":"56723","inReplyTo":"211116.86tugcp8mg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: avoid setting UTF mode when not needed","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-11-16T13:35:51Z","receivedAt":"2021-11-16T13:36:02Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"Does anyone know if René's patch causes older PCRE versions to break? \nIf it doesn't I think René's patch plus Ævar's fix is the way to go.\n\nOn Tue, Nov 16 2021 at 01:32:17 PM +0100, Ævar Arnfjörð Bjarmason \n wrote:\n> \n> On Tue, Nov 16 2021, Carlo Marcelo Arenas Belón wrote:\n> \n>> Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii \n>> patterns\n>> and UTF-8 data, 2021-10-15), PCRE_UTF mode is enabled in cases \n>> where it\n>> will fail because of UTF-8 validation, which is needed for versions \n>> of\n>> PCRE2 older than 10.34.\n>> \n>> Revert the change on logic to avoid failures that were reported \n>> from the\n>> test cases, but that should also reflect in normal use when JIT is \n>> enabled\n>> and could result in crashes (or worse), as UTF-8 validation is \n>> skipped.\n>> \n>> Keeping the tests, as they pass even without the fix as replicated \n>> locally\n>> in Debian 10 and the CI.\n>> \n>> Reported-by: Andreas Schwab \n>> Signed-off-by: Carlo Marcelo Arenas Belón \n>> ---\n>> grep.c | 6 ++----\n>> 1 file changed, 2 insertions(+), 4 deletions(-)\n>> \n>> diff --git a/grep.c b/grep.c\n>> index f6e113e9f0..fe847a0111 100644\n>> --- a/grep.c\n>> +++ b/grep.c\n>> @@ -382,10 +382,8 @@ static void compile_pcre2_pattern(struct \n>> grep_pat *p, const struct grep_opt *opt\n>> \t\t}\n>> \t\toptions |= PCRE2_CASELESS;\n>> \t}\n>> -\tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n>> -\t (!opt->ignore_locale && is_utf8_locale() &&\n>> -\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n>> -\t\t\t\t\t (p->fixed || p->is_fixed))))\n>> +\tif (!opt->ignore_locale && is_utf8_locale() && \n>> has_non_ascii(p->pattern) &&\n>> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n>> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>> \n>> #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n> \n> Hrm.\n> \n> A few things:\n> \n> First, if we've got a post-PCREv2 version whatever fix let's guard \n> that\n> with an ifdef, see thep GIT_PCRE2_VERSION_*_HIGHER at the top of \n> grep.h.\n> \n> It really helps to have those, both to know to test on those older\n> versions, and also so that we can at some point in the future bump the\n> required version and drop these workarounds entirely (as we do with\n> git-curl-compat.h).\n> \n> Secondly, whatever do here let's first fix the test added in \n> ae39ba431a,\n> so we're not groping around in the dark even more.\n> \n> I didn't spot this at the time but the test that Hamza added in that\n> based on my initial report[1] is broken & doesn't test anything\n> meaningful. It needs to have this applied:\n> \n> \tdiff --git a/t/t7812-grep-icase-non-ascii.sh \n> b/t/t7812-grep-icase-non-ascii.sh\n> \tindex 22487d90fdc..1da6b07a579 100755\n> \t--- a/t/t7812-grep-icase-non-ascii.sh\n> \t+++ b/t/t7812-grep-icase-non-ascii.sh\n> \t@@ -60,7 +60,7 @@ test_expect_success GETTEXT_LOCALE,PCRE 'log \n> --author with an ascii pattern on U\n> \t test_write_lines \"forth\" >file4 &&\n> \t git add file4 &&\n> \t git commit --author=\"À Ú Thor \" -m \n> sécond &&\n> \t- git log -1 --color=always --perl-regexp --author=\".*Thor\" \n> >log &&\n> \t+ git log -1 --color=always --perl-regexp --author=\". . Thor\" \n> >log &&\n> \t grep Author log >actual.raw &&\n> \t test_decode_color actual &&\n> \t test_cmp expected actual\n> \n> I.e. the whole point of using the color output to test this is to\n> discover where PCRE2 is going to consider a character boundary to be,\n> using .* means that it won't be tested at add, since .* will happily \n> eat\n> up whatever arbitrary data it finds with or without UTF-8 mode.\n> \n> Other tests added in that & adjacent (if any?) commits may have the \n> same\n> issue, I haven't dug into it.\n> \n> If we lead with that patch we'll get the test passing on master as\n> before, but with your patch above it'll break. I.e. the \"when not\n> needed' in the $subject isn't true, it's just that the test is\n> completely broken.\n> \n> In the context of this being a pretty urgent post-release fix (but I\n> don't know if Junio would consider a point-release, so perhaphs it's\n> not) I'd be OK with either of:\n> \n> A. Let's back out this new log grep color thing entirely while we\n> reconsider this. The gitster/hm/paint-hits-in-log-grep topic\n> currently reverts cleanly.\n> \n> B. Don't break the new log grep color thing, and also fix the \n> 'grepping\n> binary' regression (which is much more important than having A)\n> \n> But let's not go for some in-between where we break the new feature to\n> the point of it being worse than the state of not having it at all in\n> v2.33.0.\n> \n> I.e. without the that log grep color feature we wouldn't screw up the\n> display of non-ASCII characters in log output (yay), in v2.34.0 we\n> don't, but also color the match (yay), but we broke grepping binary\n> *files* (boo!).\n> \n> I think the approach I suggested in [2] is a much more viable way\n> forward, i.e. let's stop fiddling with this giant nested if statement\n> that's mainly meant for the grep-a-file-case, revert to the\n> pre-log-grep-color state, and have the log-grep-color mode pass in a\n> \"yes, I'd like the UTF-8 mode, please\".\n> \n> Having said that that's probably also broken, just in ways we're less\n> likely to spot (or maybe some of the log encoding settings/reencoding\n> saves us there?), we may need to have that ifdef'd to 10.34 and \n> higher,\n> or have some opt-in setting for this.\n> \n> 1. https://lore.kernel.org/git/87v92bju64.fsf@evledraar.gmail.com/\n> 2. https://lore.kernel.org/git/87v92bju64.fsf@evledraar.gmail.com/\n\n\n"},{"id":"441339","messageId":"CAPUEspgUugXNqs8nWHTupsOhtD-g1ETY=VRyo0+9bVpCH1A_mw@mail.gmail.com","threadId":"56723","inReplyTo":"211116.86tugcp8mg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: avoid setting UTF mode when not needed","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-16T18:22:06Z","receivedAt":"2021-11-16T18:22:20Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Tue, Nov 16, 2021 at 4:50 AM Ævar Arnfjörð Bjarmason\n wrote:\n> First, if we've got a post-PCREv2 version whatever fix let's guard that\n> with an ifdef, see thep GIT_PCRE2_VERSION_*_HIGHER at the top of grep.h.\n\nThe availability of PCRE2_MATCH_INVALID_UTF does that implicitly, but\nadding one makes sense to make it explicit; specially if it leads to\nbetter testing.\n\n> In the context of this being a pretty urgent post-release fix (but I\n> don't know if Junio would consider a point-release, so perhaphs it's\n> not) I'd be OK with either of:\n>\n> A. Let's back out this new log grep color thing entirely while we\n> reconsider this. The gitster/hm/paint-hits-in-log-grep topic\n> currently reverts cleanly.\n\nAgree that reverting the whole feature makes more sense, but was\naiming to the minimum change required to allow for a \"brown bag\"\nrelease with this.\n\nThe way the PCRE2 integration works, uses the fast path that presumes\nvalid UTF-8 and is documented[1] to have \"Undefined Behaviour\" when\nthe subject that is being searched on is not.\n\nThat has been shown before to lead to crashes or infinite loops\n\nCarlo\n\n[1] https://www.pcre.org/current/doc/html/pcre2jit.html#SEC4\n"},{"id":"441346","messageId":"xmqq35nwaqd7.fsf@gitster.g","threadId":"56723","inReplyTo":"211116.86tugcp8mg.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] grep: avoid setting UTF mode when not needed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-16T18:48:36Z","receivedAt":"2021-11-16T18:48:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> In the context of this being a pretty urgent post-release fix (but I\n> don't know if Junio would consider a point-release, so perhaphs it's\n> not) I'd be OK with either of:\n>\n> A. Let's back out this new log grep color thing entirely while we\n> reconsider this. The gitster/hm/paint-hits-in-log-grep topic\n> currently reverts cleanly.\n>\n> B. Don't break the new log grep color thing, and also fix the 'grepping\n> binary' regression (which is much more important than having A)\n>\n> But let's not go for some in-between where we break the new feature to\n> the point of it being worse than the state of not having it at all in\n> v2.33.0.\n>\n> I.e. without the that log grep color feature we wouldn't screw up the\n> display of non-ASCII characters in log output (yay), in v2.34.0 we\n> don't, but also color the match (yay), but we broke grepping binary\n> *files* (boo!).\n\nSorry, but with too many new patches on the list that are not\nurgent, while we wanted to see us work solely on post-release\nregression fixes, I do not seem to be able to locate the reports of\nthis \"breakage\" and the other binary-file breakage.\n\nBut in any case, yes, since the 2.33.0 cycle was run deliberately\nloosely to take undercooked topics to 'next' without much reviews\n(no, \"I looked at some part and they looked OK\" is not a review),\nand because any topic in 'next', by default, graduates to 'master'\nsolely on time basis, I am fully expecting that we'd have to issue\n2.33.1 (and possibly .2) during the first two or three weeks of this\ncycle. So let's make sure we fix any iffy ones.\n\nDownthread Carlo seem to agree with you that it would be better to\nrevert the paint-hits-in-log-grep topic wholesale; I have not yet\nformed an opinion, as I haven't seen the breakage reports as I said.\n\nAs I will be offline most of the day and perhaps tomorrow, nothing\nmay happen in the meantime on my end, but hopefully we'll see a\nreviewed and ready-to-be-applied patchset that people are happy with\nby the time I look at the list again ;-)?\n\nThanks.\n"},{"id":"441468","messageId":"20211117102329.95456-1-carenas@gmail.com","threadId":"56723","inReplyTo":"20211116110035.22140-1-carenas@gmail.com","subject":"[PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T10:23:29Z","receivedAt":"2021-11-17T10:23:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns\nand UTF-8 data, 2021-10-15), PCRE2_UTF mode is enabled for cases where it\nwill trigger UTF-8 validation errors (as reported) or can result in\nundefined behaviour.\n\nOur use of PCRE2 only allows searching through non UTF-8 validated data\nsafely through the use of the PCRE2_MATCH_INVALID_UTF flag, that is only\navailable after 10.34, so restrict the change to newer versions of PCRE\nand revert to the old logic for older releases, which will still allow\nfor matching not using UTF-8 for likely most usecases (as shown in the\ntests).\n\nFix one test that was using an expression that wouldn't fail without the\nnew code so it can be forced to fail if it is missing and restrict it to\nrun only for newer PCRE releases; while at it do some minor refactoring\nto cleanup the fallout for when that test might be skipped or might\nsucceed under the new conditions.\n\nKeeping the overly complex and unnecessary logic for now, to reduce risk\nbut with the hope that it will be cleaned up later.\n\nHelped-by: René Scharfe \nReported-by: Andreas Schwab \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\nv2:\n* restrict the code at compile time instead of reverting\n* \"fix\" test to document the behaviour under both PCRE code versions\n* update commit message to better explain the issue\n\n grep.c | 7 ++++++-\n t/t7812-grep-icase-non-ascii.sh | 32 ++++++++++++++++++--------------\n 2 files changed, 24 insertions(+), 15 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f6e113e9f0..0126aa3db4 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -382,12 +382,17 @@ 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+#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n \tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n \t (!opt->ignore_locale && is_utf8_locale() &&\n \t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n \t\t\t\t\t (p->fixed || p->is_fixed))))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n-\n+#else\n+\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n+\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n+\t\toptions |= PCRE2_UTF;\n+#endif\n #ifdef 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 \tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS))\ndiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\nindex 22487d90fd..3bfe1ee728 100755\n--- a/t/t7812-grep-icase-non-ascii.sh\n+++ b/t/t7812-grep-icase-non-ascii.sh\n@@ -53,14 +53,27 @@ test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii pattern on UTF-8 data' '\n-\tcat >expected <<-\\EOF &&\n-\tAuthor: À Ú Thor \n-\tEOF\n+test_expect_success GETTEXT_LOCALE,PCRE 'setup ascii pattern on UTF-8 data' '\n \ttest_write_lines \"forth\" >file4 &&\n \tgit add file4 &&\n \tgit commit --author=\"À Ú Thor \" -m sécond &&\n-\tgit log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n+\ttest_write_lines \"fifth\" >file5 &&\n+\tgit add file5 &&\n+\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n+\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n+\tgit -c i18n.commitEncoding=latin1 commit -m thïrd\n+'\n+\n+test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n+\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n+'\n+\n+test_expect_success GETTEXT_LOCALE,PCRE,PCRE2_MATCH_INVALID_UTF 'log --author with an ascii pattern on UTF-8 data' '\n+\tcat >expected <<-\\EOF &&\n+\tAuthor: A U Thor \n+\tAuthor: À Ú Thor \n+\tEOF\n+\tgit log --color=always --perl-regexp --author=\". . Thor\" >log &&\n \tgrep Author log >actual.raw &&\n \ttest_decode_color actual &&\n \ttest_cmp expected actual\n@@ -70,11 +83,6 @@ test_expect_success GETTEXT_LOCALE,PCRE 'log --committer with an ascii pattern o\n \tcat >expected <<-\\EOF &&\n \tCommit: Ç O Mîtter \n \tEOF\n-\ttest_write_lines \"fifth\" >file5 &&\n-\tgit add file5 &&\n-\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n-\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n-\tgit -c i18n.commitEncoding=latin1 commit -m thïrd &&\n \tgit -c i18n.logOutputEncoding=latin1 log -1 --pretty=fuller --color=always --perl-regexp --committer=\" O.*\" >log &&\n \tgrep Commit: log >actual.raw &&\n \ttest_decode_color actual &&\n@@ -141,10 +149,6 @@ test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: grep non-ASCII from invali\n \ttest_cmp invalid-0xe5 actual\n '\n \n-test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n-\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n-'\n-\n test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' '\n \ttest_might_fail git grep -hi \"Æ\" invalid-0x80 >actual &&\n \ttest_might_fail git grep -hi \"(*NO_JIT)Æ\" invalid-0x80 >actual\n-- \n2.34.0.352.g07dee3c5e1\n\n"},{"id":"441498","messageId":"211117.867dd67spq.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"R33O2R.WYXS4AQP7W05@effective-light.com","subject":"Re: [PATCH] grep: avoid setting UTF mode when not needed","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-17T14:31:19Z","receivedAt":"2021-11-17T14:38:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Nov 16 2021, Hamza Mahfooz wrote:\n\n> Does anyone know if René's patch causes older PCRE versions to break?\n> If it doesn't I think René's patch plus Ævar's fix is the way to go.\n\nI haven't tested, but I think a very good thing to do/test (hint hint!)\nis to add a CI job where we build/test git in combination with various\nPCRE versions.\n\nPCRE2 used to be in SVN, but nowadays it's in a git repository on\ngithub: https://github.com/PhilipHazel/pcre2\n\nIt should be a small matter of copying the existing template to set up a\n\"matrix\" where we test various known older pcre versions, they're\navailable as tags in the git repository.\n\n\"--enable-jit\" and \"--disable-jit\" should be part of that matrix too,\nand \"--enable-utf\" and \"--disable-utf\".\n\nSee the \"microsoft/vcpkg\" and \"regular:\" parts of\n.github/workflows/main.git for relevant examples, i.e. this would\ne.g. clone pcre2.git into compat/pcre2-repo, check out a given version\nfrom the matrix. Then:\n\n cd compat/pcre2-repo &&\n ./autogen.sh &&\n ./configure --prefix=\"$PWD/inst\" &&\n make install\n\nThen at the top-level:\n\n make USE_LIBPCRE=Y CFLAGS=-O3 LIBPCREDIR=\"$PWD/compat/pcre2-repo/inst\"\n\nThen run \"make test\", for saving some CPU & speeding up the runs it\nshould be more than sufficient to only run those tests that match\n*{grep,log,pickaxe}*\n \n"},{"id":"441518","messageId":"634c4237-325a-13e8-0a92-09d23bdfb111@web.de","threadId":"56723","inReplyTo":"CAPUEspg8ZUdn+KFz35yG1k9bbfVTe1b+7=+WdMknRS1zu8VcDQ@mail.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-11-17T18:46:19Z","receivedAt":"2021-11-17T18:46:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 16.11.21 um 10:38 schrieb Carlo Arenas:\n> On Tue, Nov 16, 2021 at 1:30 AM Andreas Schwab wrote:\n>>\n>> expecting success of 7812.13 'PCRE v2: grep ASCII from invalid UTF-8 data':\n>> git grep -h \"var\" invalid-0x80 >actual &&\n>> test_cmp expected actual &&\n>> git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n>> test_cmp expected actual\n>>\n>> ++ git grep -h var invalid-0x80\n>> ++ test_cmp expected actual\n>> ++ test 2 -ne 2\n>> ++ eval 'diff -u' '\"$@\"'\n>> +++ diff -u expected actual\n>> ++ git grep -h '(*NO_JIT)var' invalid-0x80\n>> fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\n>\n> That is exactly what I was worried about, this is not failing one\n> test, but making `git grep` unusable in any repository that has any\n> binary files that might be reachable by it, and it is likely affecting\n> anyone using PCRE older than 10.34\n\nLet's have a look at the map. Here are the differences between the\nversions regarding use of PCRE2_UTF:\n\no: opt->ignore_locale\nh: has_non_ascii(p->pattern)\ni: is_utf8_locale()\nl: !opt->ignore_case && (p->fixed || p->is_fixed)\n\no h i l master hamza rene2\n0 0 0 0 0 1 0\n0 0 0 1 0 1 0\n0 0 1 0 0 1 1\n0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging\n\nSo http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\nshould not have this breakage, because it doesn't enable PCRE2_UTF for\nliteral patterns.\n\nRené\n"},{"id":"441522","messageId":"CAPUEspg3Ox41H-dCKzqV5oS4UP5Pkdoq0wUaXtxfNWWRTO0k1w@mail.gmail.com","threadId":"56723","inReplyTo":"634c4237-325a-13e8-0a92-09d23bdfb111@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T19:56:25Z","receivedAt":"2021-11-17T19:56:41Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 17, 2021 at 10:46 AM René Scharfe wrote:\n>\n> Am 16.11.21 um 10:38 schrieb Carlo Arenas:\n> > On Tue, Nov 16, 2021 at 1:30 AM Andreas Schwab wrote:\n> >>\n> >> expecting success of 7812.13 'PCRE v2: grep ASCII from invalid UTF-8 data':\n> >> git grep -h \"var\" invalid-0x80 >actual &&\n> >> test_cmp expected actual &&\n> >> git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n> >> test_cmp expected actual\n> >>\n> >> ++ git grep -h var invalid-0x80\n> >> ++ test_cmp expected actual\n> >> ++ test 2 -ne 2\n> >> ++ eval 'diff -u' '\"$@\"'\n> >> +++ diff -u expected actual\n> >> ++ git grep -h '(*NO_JIT)var' invalid-0x80\n> >> fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\n> >\n> > That is exactly what I was worried about, this is not failing one\n> > test, but making `git grep` unusable in any repository that has any\n> > binary files that might be reachable by it, and it is likely affecting\n> > anyone using PCRE older than 10.34\n>\n> Let's have a look at the map. Here are the differences between the\n> versions regarding use of PCRE2_UTF:\n>\n> o: opt->ignore_locale\n> h: has_non_ascii(p->pattern)\n> i: is_utf8_locale()\n> l: !opt->ignore_case && (p->fixed || p->is_fixed)\n>\n> o h i l master hamza rene2\n> 0 0 0 0 0 1 0\n> 0 0 0 1 0 1 0\n> 0 0 1 0 0 1 1\n> 0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging\n>\n> So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n> should not have this breakage, because it doesn't enable PCRE2_UTF for\n> literal patterns.\n\nCorrect, and indeed I had your expression instead of the ugly one in\nmy draft for v2[1] but then I found the tests were even more broken\nthan reported originally and got worried it might introduce yet\nanother issue because of the brittleness of the rest of the code\naround it. I am hoping it will be introduced in a follow up series\nthough, and unless this code gets thrown away in the promised\nrefactoring by Ævar which I haven't seen yet and once this is in\nmaint.\n\nIMHO and in line with the previous warnings you raised[2] during the\ndevelopment of this feature, it might be worth looking at it more\ndeeply, but at least the proposed patch keeps the feature working in\npractice (the only case that won't work as expected will be when the\nexpression includes \".\" with the intention of matching a UTF-8\ncharacter and while using PCRE2 older than 10.34) and more importantly\nfixes the regression that it introduced.\n\nCarlo\n\n[1] https://lore.kernel.org/git/20211117102329.95456-1-carenas@gmail.com/\n[2] https://lore.kernel.org/git/fc7eb9fc-9521-5484-b05f-9c20086fd485@web.de/\n"},{"id":"441525","messageId":"211117.86y25m5wez.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"634c4237-325a-13e8-0a92-09d23bdfb111@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-17T20:59:45Z","receivedAt":"2021-11-17T21:01:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 17 2021, René Scharfe wrote:\n\n> Am 16.11.21 um 10:38 schrieb Carlo Arenas:\n>> On Tue, Nov 16, 2021 at 1:30 AM Andreas Schwab wrote:\n>>>\n>>> expecting success of 7812.13 'PCRE v2: grep ASCII from invalid UTF-8 data':\n>>> git grep -h \"var\" invalid-0x80 >actual &&\n>>> test_cmp expected actual &&\n>>> git grep -h \"(*NO_JIT)var\" invalid-0x80 >actual &&\n>>> test_cmp expected actual\n>>>\n>>> ++ git grep -h var invalid-0x80\n>>> ++ test_cmp expected actual\n>>> ++ test 2 -ne 2\n>>> ++ eval 'diff -u' '\"$@\"'\n>>> +++ diff -u expected actual\n>>> ++ git grep -h '(*NO_JIT)var' invalid-0x80\n>>> fatal: pcre2_match failed with error code -22: UTF-8 error: isolated byte with 0x80 bit set\n>>\n>> That is exactly what I was worried about, this is not failing one\n>> test, but making `git grep` unusable in any repository that has any\n>> binary files that might be reachable by it, and it is likely affecting\n>> anyone using PCRE older than 10.34\n>\n> Let's have a look at the map. Here are the differences between the\n> versions regarding use of PCRE2_UTF:\n>\n> o: opt->ignore_locale\n> h: has_non_ascii(p->pattern)\n> i: is_utf8_locale()\n> l: !opt->ignore_case && (p->fixed || p->is_fixed)\n>\n> o h i l master hamza rene2\n> 0 0 0 0 0 1 0\n> 0 0 0 1 0 1 0\n> 0 0 1 0 0 1 1\n> 0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging\n>\n> So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n> should not have this breakage, because it doesn't enable PCRE2_UTF for\n> literal patterns.\n\nPCRE2_UTF will also matter for literal patterns. Try to peel apart the\ntwo bytes in \"é\" and match them under -i with/without PCRE_UTF.\n\nI don't know what the right behavior should be here (haven't had time to\ndig), but it matters for matching.\n"},{"id":"441527","messageId":"CAPUEsph8NMJtK2r_fuT6UPsgmoPAeGVfTLOj8uz6NaOj4UZ1dg@mail.gmail.com","threadId":"56723","inReplyTo":"211117.86y25m5wez.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-17T21:53:05Z","receivedAt":"2021-11-17T21:53:21Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 17, 2021 at 1:01 PM Ævar Arnfjörð Bjarmason\n wrote:\n>\n> PCRE2_UTF will also matter for literal patterns. Try to peel apart the\n> two bytes in \"é\" and match them under -i with/without PCRE_UTF.\n\nIs there a real use case for why someone would do that? and how is\nthat \"literal\" valid UTF to warrant setting PCRE2_UTF?\nI would expect that someone including random bytes in the expression\nis really more interested in binary matching anyway and the use of -i\nwith it probably should be an error.\n\nIndeed I suspect the fact that pcre2_compile lets it through might be\na bug in PCRE2\n\n$ pcre2test\nPCRE2 version 10.39 2021-10-29\n re> /\\303/utf,caseless\ndata> \\303\n 0: \\x{c3}\ndata> é\nNo match\n\nCarlo\n"},{"id":"441542","messageId":"211118.86pmqy5naz.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"CAPUEsph8NMJtK2r_fuT6UPsgmoPAeGVfTLOj8uz6NaOj4UZ1dg@mail.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-18T00:00:26Z","receivedAt":"2021-11-18T00:18:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Nov 17 2021, Carlo Arenas wrote:\n\n> On Wed, Nov 17, 2021 at 1:01 PM Ævar Arnfjörð Bjarmason\n> wrote:\n>>\n>> PCRE2_UTF will also matter for literal patterns. Try to peel apart the\n>> two bytes in \"é\" and match them under -i with/without PCRE_UTF.\n>\n> Is there a real use case for why someone would do that? and how is\n> that \"literal\" valid UTF to warrant setting PCRE2_UTF?\n\nWe don't check the validity of the input pattern as correct UTF-8 before\nfeeding it to PCRE. We don't even check if you're under a UTF-8 locale,\na call to is_utf8_locale() is one of the conditions we'll try (but check\nhow loose we are under NO_GETTEXT=Y even then), but it's not a necessary\none.\n\nSo in practice we'll likely have users pasting say Big5, UTF-32, JIS\nencoding or whatever into \"git grep\" and then grepping arbitrary binary\ndata.\n\nI don't really have a specific use-case in mind. I'm just trying to\ncounter the apparent recurring misconception that's popped up more than\nonce in these recent threads that the UTF-8 *mode* is only something we\nneed to worry about if the pattern contains non-ASCII.\n\nThe implications of UTF-8 as a matching mode go much deeper than that in\nPerl's and PCRE's regex engines, e.g. in this case what's considered a\ncharacter boundary.\n\n> I would expect that someone including random bytes in the expression\n> is really more interested in binary matching anyway and the use of -i\n> with it probably should be an error.\n>\n> Indeed I suspect the fact that pcre2_compile lets it through might be\n> a bug in PCRE2\n>\n> $ pcre2test\n> PCRE2 version 10.39 2021-10-29\n> re> /\\303/utf,caseless\n> data> \\303\n> 0: \\x{c3}\n> data> é\n> No match\n\nIt's really not \"random bytes\".\n\nWe're not talking about someone doing a git grep where the argument is\nfed from a \"cat\" of /dev/urandom. Just plain old boring natural language\nencodings in common use can and will conflict with what's considered a\ncharacter boundary in UTF-8,\n\nAnyway, I haven't had time to re-page this topic at large into my\nwetware and really don't know what we should be doing exactly at this\npoint to move forward, except my previous suggestion to either revert\nthe recent changes, or at least to narrow them down so that we get the\nold behavior for everything except the revision.c \"git log\" caller.\n\nI think a really good next step aside from dealing with that immediate\nproblem would be to harden some of our test data in a way similar to\nwhat we've got in t7816-grep-binary-pattern.sh, but for partial matches,\nmixtures of valid/invalid/partial character patterns and subjects etc.\n\nWe might be able to lift some tests from existing test suites,\nperl.git's t/re/re_tests and pcre2.git's RunGrepTest (And testdata/\ndirectory) are probably good places to start looking.\n\nWe don't need to test e.g. PCRE itself independently, but it is worth\ntesting how whatever special-sauce we add on top (if and when to turn on\nits flags based on heuristics) impacts things.\n\nWe've also got the problem that whatever code we write will target\nmultiple PCREv2 versions, which isn't something PCREv2 itself needs to\ndeal with.\n\n"},{"id":"441579","messageId":"xmqqpmqxyla3.fsf@gitster.g","threadId":"56723","inReplyTo":"20211117102329.95456-1-carenas@gmail.com","subject":"Re: [PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T07:29:08Z","receivedAt":"2021-11-18T07:29:18Z","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> Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii patterns\n> and UTF-8 data, 2021-10-15), PCRE2_UTF mode is enabled for cases where it\n> will trigger UTF-8 validation errors (as reported) or can result in\n> undefined behaviour.\n>\n> Our use of PCRE2 only allows searching through non UTF-8 validated data\n> safely through the use of the PCRE2_MATCH_INVALID_UTF flag, that is only\n> available after 10.34, so restrict the change to newer versions of PCRE\n> and revert to the old logic for older releases, which will still allow\n> for matching not using UTF-8 for likely most usecases (as shown in the\n> tests).\n>\n> Fix one test that was using an expression that wouldn't fail without the\n> new code so it can be forced to fail if it is missing and restrict it to\n> run only for newer PCRE releases; while at it do some minor refactoring\n> to cleanup the fallout for when that test might be skipped or might\n> succeed under the new conditions.\n>\n> Keeping the overly complex and unnecessary logic for now, to reduce risk\n> but with the hope that it will be cleaned up later.\n>\n> Helped-by: René Scharfe \n> Reported-by: Andreas Schwab \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v2:\n> * restrict the code at compile time instead of reverting\n> * \"fix\" test to document the behaviour under both PCRE code versions\n> * update commit message to better explain the issue\n\nSo, is this the final verrsion everybody is happy with, instead of\njust reverting the whole \"paint hits in log --grep\" topic, or did I\nmiss more discussion in the original thread?\n\nWith too many patches on the list, it is a bit hard to keep track of\nthe more urgent regresison fixes X-<.\n\nThanks.\n\n> grep.c | 7 ++++++-\n> t/t7812-grep-icase-non-ascii.sh | 32 ++++++++++++++++++--------------\n> 2 files changed, 24 insertions(+), 15 deletions(-)\n>\n> diff --git a/grep.c b/grep.c\n> index f6e113e9f0..0126aa3db4 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -382,12 +382,17 @@ 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> +#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> \tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n> \t (!opt->ignore_locale && is_utf8_locale() &&\n> \t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n> \t\t\t\t\t (p->fixed || p->is_fixed))))\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> -\n> +#else\n> +\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> +\t\toptions |= PCRE2_UTF;\n> +#endif\n> #ifdef 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> \tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | PCRE2_CASELESS))\n> diff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\n> index 22487d90fd..3bfe1ee728 100755\n> --- a/t/t7812-grep-icase-non-ascii.sh\n> +++ b/t/t7812-grep-icase-non-ascii.sh\n> @@ -53,14 +53,27 @@ test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '\n> \ttest_cmp expected actual\n> '\n> \n> -test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii pattern on UTF-8 data' '\n> -\tcat >expected <<-\\EOF &&\n> -\tAuthor: À Ú Thor \n> -\tEOF\n> +test_expect_success GETTEXT_LOCALE,PCRE 'setup ascii pattern on UTF-8 data' '\n> \ttest_write_lines \"forth\" >file4 &&\n> \tgit add file4 &&\n> \tgit commit --author=\"À Ú Thor \" -m sécond &&\n> -\tgit log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n> +\ttest_write_lines \"fifth\" >file5 &&\n> +\tgit add file5 &&\n> +\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n> +\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n> +\tgit -c i18n.commitEncoding=latin1 commit -m thïrd\n> +'\n> +\n> +test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n> +\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n> +'\n> +\n> +test_expect_success GETTEXT_LOCALE,PCRE,PCRE2_MATCH_INVALID_UTF 'log --author with an ascii pattern on UTF-8 data' '\n> +\tcat >expected <<-\\EOF &&\n> +\tAuthor: A U Thor \n> +\tAuthor: À Ú Thor \n> +\tEOF\n> +\tgit log --color=always --perl-regexp --author=\". . Thor\" >log &&\n> \tgrep Author log >actual.raw &&\n> \ttest_decode_color actual &&\n> \ttest_cmp expected actual\n> @@ -70,11 +83,6 @@ test_expect_success GETTEXT_LOCALE,PCRE 'log --committer with an ascii pattern o\n> \tcat >expected <<-\\EOF &&\n> \tCommit: Ç O Mîtter \n> \tEOF\n> -\ttest_write_lines \"fifth\" >file5 &&\n> -\tgit add file5 &&\n> -\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n> -\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n> -\tgit -c i18n.commitEncoding=latin1 commit -m thïrd &&\n> \tgit -c i18n.logOutputEncoding=latin1 log -1 --pretty=fuller --color=always --perl-regexp --committer=\" O.*\" >log &&\n> \tgrep Commit: log >actual.raw &&\n> \ttest_decode_color actual &&\n> @@ -141,10 +149,6 @@ test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: grep non-ASCII from invali\n> \ttest_cmp invalid-0xe5 actual\n> '\n> \n> -test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n> -\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n> -'\n> -\n> test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: grep non-ASCII from invalid UTF-8 data with -i' '\n> \ttest_might_fail git grep -hi \"Æ\" invalid-0x80 >actual &&\n> \ttest_might_fail git grep -hi \"(*NO_JIT)Æ\" invalid-0x80 >actual\n"},{"id":"441590","messageId":"CAPUEsphbA0Qrnt-MdxyYT2OMrnXnbi0tN5_YNqLypCqDqZyeWg@mail.gmail.com","threadId":"56723","inReplyTo":"xmqqpmqxyla3.fsf@gitster.g","subject":"Re: [PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-18T10:15:49Z","receivedAt":"2021-11-18T10:18:41Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Wed, Nov 17, 2021 at 11:29 PM Junio C Hamano wrote:\n>\n> So, is this the final verrsion everybody is happy with, instead of\n> just reverting the whole \"paint hits in log --grep\" topic, or did I\n> miss more discussion in the original thread?\n\nThis will likely need at least some ACKs.\n\nApologies for the noise.\n\nCarlo\n"},{"id":"441602","messageId":"HAQR2R.KZGF0ERBQ2Z72@effective-light.com","threadId":"56723","inReplyTo":"CAPUEsphbA0Qrnt-MdxyYT2OMrnXnbi0tN5_YNqLypCqDqZyeWg@mail.gmail.com","subject":"Re: [PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-11-18T12:49:29Z","receivedAt":"2021-11-18T12:51:54Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"If none of the patches that are trying to fix our usage of PCRE2 are \nACKed.\nIt would be preferable (from my perspective) to revert only ae39ba431a\n(grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data,\n2021-10-15) and disable the feature (for the time being) if PCRE2 is \nselected, like so:\n\ndiff --git a/pretty.c b/pretty.c\nindex 1af5b093ae..a53d008d1e 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -452,7 +452,7 @@ static void append_line_with_color(struct strbuf \n*sb, struct grep_opt *opt,\n buf = line;\n eol = buf + linelen;\n\n- if (!opt || !want_color(color) || opt->invert)\n+ if (!opt || !want_color(color) || opt->invert || opt->pcre2)\n goto end;\n\n line_color = opt->colors[GREP_COLOR_SELECTED];\n\n\nOn Thu, Nov 18 2021 at 02:15:49 AM -0800, Carlo Arenas \n wrote:\n> \n> This will likely need at least some ACKs.\n> \n> Apologies for the noise.\n> \n> Carlo\n\n\n"},{"id":"441650","messageId":"xmqqczmxxr8o.fsf@gitster.g","threadId":"56723","inReplyTo":"211117.86y25m5wez.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-18T18:17:59Z","receivedAt":"2021-11-18T18:18:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Let's have a look at the map. Here are the differences between the\n>> versions regarding use of PCRE2_UTF:\n>>\n>> o: opt->ignore_locale\n>> h: has_non_ascii(p->pattern)\n>> i: is_utf8_locale()\n>> l: !opt->ignore_case && (p->fixed || p->is_fixed)\n>>\n>> o h i l master hamza rene2\n>> 0 0 0 0 0 1 0\n>> 0 0 0 1 0 1 0\n>> 0 0 1 0 0 1 1\n>> 0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging\n>>\n>> So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n>> should not have this breakage, because it doesn't enable PCRE2_UTF for\n>> literal patterns.\n>\n> PCRE2_UTF will also matter for literal patterns. Try to peel apart the\n> two bytes in \"é\" and match them under -i with/without PCRE_UTF.\n\nSorry for being late to the party, but doesn't \"literal\" in the\ncontext of this thread mean the column \"l\" above, i.e. we are not\nignoring case and fixed or is_fixed member is set? So \"under -i\"\ndisqualifies as an example for \"will also matter for literal\",\ndoesn't it?\n\nIn hindsight, I guess we could have pushed a bit harder when René's\n\n-\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n+\tif (!opt->ignore_locale && is_utf8_locale() &&\n \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n\nin https://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n(is that what is called 'rene2' above?) was raised on Oct 17th to\namend/fix Hamza's [v13 3/3]; that would have prevented 'master' from\nhaving this breakage?\n\nCarlo, in your [PATCH v2] in <20211117102329.95456-1-carenas@gmail.com>,\nI see that the #else side for older PCREv2 users essentially reverts\nwhat Hamza's [PATCH v13 3/3] did to this area.\n\n+#else\n+\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n+\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n+\t\toptions |= PCRE2_UTF;\n+#endif\n\nI guess this is a lot of change in the amount of text involved but\nthe least amount of actual change in the behaviour. For those with\nnewer PCREv2, the behaviour would be the same as v2.34.0, and for\nothers, the behaviour would be the same as v2.33.0.\n\nHaving said all that, because the consensus seems to be that the\nwhole \"when we should match in UTF mode\" may need to be rethought, I\nthink reverting Hamza's [v13 3/3] would be the simplest way to clean\nup the mess for v2.34.1 that will give us a cleaner slate to later\nbuild on, than applying this patch.\n\nSo, I dunno. Comments from those involved in the discussion?\n\nThanks.\n\n"},{"id":"441658","messageId":"c5223004-6db6-e808-f607-d11bd45e7d99@web.de","threadId":"56723","inReplyTo":"xmqqczmxxr8o.fsf@gitster.g","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-11-18T20:57:35Z","receivedAt":"2021-11-18T20:57:51Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 18.11.21 um 19:17 schrieb Junio C Hamano:\n> Ævar Arnfjörð Bjarmason writes:\n>\n>>> Let's have a look at the map. Here are the differences between the\n>>> versions regarding use of PCRE2_UTF:\n>>>\n>>> o: opt->ignore_locale\n>>> h: has_non_ascii(p->pattern)\n>>> i: is_utf8_locale()\n>>> l: !opt->ignore_case && (p->fixed || p->is_fixed)\n>>>\n>>> o h i l master hamza rene2\n>>> 0 0 0 0 0 1 0\n>>> 0 0 0 1 0 1 0\n>>> 0 0 1 0 0 1 1\n>>> 0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging\n>>>\n>>> So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n>>> should not have this breakage, because it doesn't enable PCRE2_UTF for\n>>> literal patterns.\n>>\n>> PCRE2_UTF will also matter for literal patterns. Try to peel apart the\n>> two bytes in \"é\" and match them under -i with/without PCRE_UTF.\n>\n> Sorry for being late to the party, but doesn't \"literal\" in the\n> context of this thread mean the column \"l\" above, i.e. we are not\n> ignoring case and fixed or is_fixed member is set? So \"under -i\"\n> disqualifies as an example for \"will also matter for literal\",\n> doesn't it?\n\nCorrect.\n\n> In hindsight, I guess we could have pushed a bit harder when René's\n>\n> -\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> +\tif (!opt->ignore_locale && is_utf8_locale() &&\n> \t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n>\n> in https://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/\n> (is that what is called 'rene2' above?) was raised on Oct 17th to\n> amend/fix Hamza's [v13 3/3]; that would have prevented 'master' from\n> having this breakage?\n\nYes, that the change I meant with \"rene2\".\n\n> Carlo, in your [PATCH v2] in <20211117102329.95456-1-carenas@gmail.com>,\n> I see that the #else side for older PCREv2 users essentially reverts\n> what Hamza's [PATCH v13 3/3] did to this area.\n>\n> +#else\n> +\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> +\t\toptions |= PCRE2_UTF;\n> +#endif\n>\n> I guess this is a lot of change in the amount of text involved but\n> the least amount of actual change in the behaviour. For those with\n> newer PCREv2, the behaviour would be the same as v2.34.0, and for\n> others, the behaviour would be the same as v2.33.0.\n>\n> Having said all that, because the consensus seems to be that the\n> whole \"when we should match in UTF mode\" may need to be rethought, I\n> think reverting Hamza's [v13 3/3] would be the simplest way to clean\n> up the mess for v2.34.1 that will give us a cleaner slate to later\n> build on, than applying this patch.\n\nMakes sense to me. It gives a better starting point to solve the issue\nafresh without getting entangled in mind-melting boolean expressions.\n\nRené\n"},{"id":"441659","messageId":"JZDS2R.2JB8M8ZCH6NX1@effective-light.com","threadId":"56723","inReplyTo":"20211117102329.95456-1-carenas@gmail.com","subject":"Re: [PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Hamza Mahfooz","fromEmail":"someguy@effective-light.com","sentAt":"2021-11-18T21:21:19Z","receivedAt":"2021-11-18T21:21:38Z","isPatch":true,"sender":{"key":"someguy@effective-light.com","avatar":"https://avatars.githubusercontent.com/u/3885502?v=4"},"body":"Building on to your patch we can disable the feature if an older \nversion of PCRE2 is used, like so:\n\ndiff --git a/pretty.c b/pretty.c\nindex 1af5b093ae..9bd3f61b30 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -451,8 +451,11 @@ static void append_line_with_color(struct strbuf \n*sb, struct grep_opt *opt,\n\n buf = line;\n eol = buf + linelen;\n-\n+#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n if (!opt || !want_color(color) || opt->invert)\n+#else\n+ if (!opt || !want_color(color) || opt->invert || opt->pcre2)\n+#endif\n goto end;\n\n line_color = opt->colors[GREP_COLOR_SELECTED];\n\n\n\nOn Wed, Nov 17 2021 at 02:23:29 AM -0800, Carlo Marcelo Arenas Belón \n wrote:\n> Since ae39ba431a (grep/pcre2: fix an edge case concerning ascii \n> patterns\n> and UTF-8 data, 2021-10-15), PCRE2_UTF mode is enabled for cases \n> where it\n> will trigger UTF-8 validation errors (as reported) or can result in\n> undefined behaviour.\n> \n> Our use of PCRE2 only allows searching through non UTF-8 validated \n> data\n> safely through the use of the PCRE2_MATCH_INVALID_UTF flag, that is \n> only\n> available after 10.34, so restrict the change to newer versions of \n> PCRE\n> and revert to the old logic for older releases, which will still allow\n> for matching not using UTF-8 for likely most usecases (as shown in the\n> tests).\n> \n> Fix one test that was using an expression that wouldn't fail without \n> the\n> new code so it can be forced to fail if it is missing and restrict it \n> to\n> run only for newer PCRE releases; while at it do some minor \n> refactoring\n> to cleanup the fallout for when that test might be skipped or might\n> succeed under the new conditions.\n> \n> Keeping the overly complex and unnecessary logic for now, to reduce \n> risk\n> but with the hope that it will be cleaned up later.\n> \n> Helped-by: René Scharfe \n> Reported-by: Andreas Schwab \n> Signed-off-by: Carlo Marcelo Arenas Belón \n> ---\n> v2:\n> * restrict the code at compile time instead of reverting\n> * \"fix\" test to document the behaviour under both PCRE code versions\n> * update commit message to better explain the issue\n> \n> grep.c | 7 ++++++-\n> t/t7812-grep-icase-non-ascii.sh | 32 ++++++++++++++++++--------------\n> 2 files changed, 24 insertions(+), 15 deletions(-)\n> \n> diff --git a/grep.c b/grep.c\n> index f6e113e9f0..0126aa3db4 100644\n> --- a/grep.c\n> +++ b/grep.c\n> @@ -382,12 +382,17 @@ static void compile_pcre2_pattern(struct \n> grep_pat *p, const struct grep_opt *opt\n> \t\t}\n> \t\toptions |= PCRE2_CASELESS;\n> \t}\n> +#ifdef GIT_PCRE2_VERSION_10_34_OR_HIGHER\n> \tif ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||\n> \t (!opt->ignore_locale && is_utf8_locale() &&\n> \t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n> \t\t\t\t\t (p->fixed || p->is_fixed))))\n> \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n> -\n> +#else\n> +\tif (!opt->ignore_locale && is_utf8_locale() && \n> has_non_ascii(p->pattern) &&\n> +\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n> +\t\toptions |= PCRE2_UTF;\n> +#endif\n> #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\n> \t/* Work around https://bugs.exim.org/show_bug.cgi?id=2642 fixed in \n> 10.36 */\n> \tif (PCRE2_MATCH_INVALID_UTF && options & (PCRE2_UTF | \n> PCRE2_CASELESS))\n> diff --git a/t/t7812-grep-icase-non-ascii.sh \n> b/t/t7812-grep-icase-non-ascii.sh\n> index 22487d90fd..3bfe1ee728 100755\n> --- a/t/t7812-grep-icase-non-ascii.sh\n> +++ b/t/t7812-grep-icase-non-ascii.sh\n> @@ -53,14 +53,27 @@ test_expect_success REGEX_LOCALE 'pickaxe -i on \n> non-ascii' '\n> \ttest_cmp expected actual\n> '\n> \n> -test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii \n> pattern on UTF-8 data' '\n> -\tcat >expected <<-\\EOF &&\n> -\tAuthor: À Ú Thor \n> -\tEOF\n> +test_expect_success GETTEXT_LOCALE,PCRE 'setup ascii pattern on \n> UTF-8 data' '\n> \ttest_write_lines \"forth\" >file4 &&\n> \tgit add file4 &&\n> \tgit commit --author=\"À Ú Thor \" -m sécond &&\n> -\tgit log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n> +\ttest_write_lines \"fifth\" >file5 &&\n> +\tgit add file5 &&\n> +\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n> +\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n> +\tgit -c i18n.commitEncoding=latin1 commit -m thïrd\n> +'\n> +\n> +test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n> +\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n> +'\n> +\n> +test_expect_success GETTEXT_LOCALE,PCRE,PCRE2_MATCH_INVALID_UTF 'log \n> --author with an ascii pattern on UTF-8 data' '\n> +\tcat >expected <<-\\EOF &&\n> +\tAuthor: A U Thor \n> +\tAuthor: À Ú Thor \n> +\tEOF\n> +\tgit log --color=always --perl-regexp --author=\". . Thor\" >log &&\n> \tgrep Author log >actual.raw &&\n> \ttest_decode_color actual &&\n> \ttest_cmp expected actual\n> @@ -70,11 +83,6 @@ test_expect_success GETTEXT_LOCALE,PCRE 'log \n> --committer with an ascii pattern o\n> \tcat >expected <<-\\EOF &&\n> \tCommit: Ç O Mîtter \n> \tEOF\n> -\ttest_write_lines \"fifth\" >file5 &&\n> -\tgit add file5 &&\n> -\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n> -\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n> -\tgit -c i18n.commitEncoding=latin1 commit -m thïrd &&\n> \tgit -c i18n.logOutputEncoding=latin1 log -1 --pretty=fuller \n> --color=always --perl-regexp --committer=\" O.*\" >log &&\n> \tgrep Commit: log >actual.raw &&\n> \ttest_decode_color actual &&\n> @@ -141,10 +149,6 @@ test_expect_success GETTEXT_LOCALE,LIBPCRE2 \n> 'PCRE v2: grep non-ASCII from invali\n> \ttest_cmp invalid-0xe5 actual\n> '\n> \n> -test_lazy_prereq PCRE2_MATCH_INVALID_UTF '\n> -\ttest-tool pcre2-config has-PCRE2_MATCH_INVALID_UTF\n> -'\n> -\n> test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: grep non-ASCII \n> from invalid UTF-8 data with -i' '\n> \ttest_might_fail git grep -hi \"Æ\" invalid-0x80 >actual &&\n> \ttest_might_fail git grep -hi \"(*NO_JIT)Æ\" invalid-0x80 >actual\n> --\n> 2.34.0.352.g07dee3c5e1\n> \n\n\n"},{"id":"441698","messageId":"211119.86r1bc4om5.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"HAQR2R.KZGF0ERBQ2Z72@effective-light.com","subject":"Re: [PATCH v2] grep: avoid setting UTF mode when dangerous with PCRE","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-19T06:58:57Z","receivedAt":"2021-11-19T06:59:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 18 2021, Hamza Mahfooz wrote:\n\n> If none of the patches that are trying to fix our usage of PCRE2 are\n> ACKed.\n> It would be preferable (from my perspective) to revert only ae39ba431a\n> (grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data,\n> 2021-10-15) and disable the feature (for the time being) if PCRE2 is\n> selected, like so:\n>\n> diff --git a/pretty.c b/pretty.c\n> index 1af5b093ae..a53d008d1e 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -452,7 +452,7 @@ static void append_line_with_color(struct strbuf\n> *sb, struct grep_opt *opt,\n> buf = line;\n> eol = buf + linelen;\n>\n> - if (!opt || !want_color(color) || opt->invert)\n> + if (!opt || !want_color(color) || opt->invert || opt->pcre2)\n> goto end;\n\nI see that depending on LC_ALL the behavior of grep.patternType=extended\ncan exhibit some of the same issues with character splitting, so perhaps\nwhat we'd do with the C library serves as a useful guide?\n"},{"id":"441699","messageId":"211119.86mtm04og8.gmgdl@evledraar.gmail.com","threadId":"56723","inReplyTo":"c5223004-6db6-e808-f607-d11bd45e7d99@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-19T07:00:30Z","receivedAt":"2021-11-19T07:03:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Nov 18 2021, René Scharfe wrote:\n\n> Am 18.11.21 um 19:17 schrieb Junio C Hamano:\n>> Ævar Arnfjörð Bjarmason writes:\n> [...]\n>> I guess this is a lot of change in the amount of text involved but\n>> the least amount of actual change in the behaviour. For those with\n>> newer PCREv2, the behaviour would be the same as v2.34.0, and for\n>> others, the behaviour would be the same as v2.33.0.\n>>\n>> Having said all that, because the consensus seems to be that the\n>> whole \"when we should match in UTF mode\" may need to be rethought, I\n>> think reverting Hamza's [v13 3/3] would be the simplest way to clean\n>> up the mess for v2.34.1 that will give us a cleaner slate to later\n>> build on, than applying this patch.\n>\n> Makes sense to me. It gives a better starting point to solve the issue\n> afresh without getting entangled in mind-melting boolean expressions.\n\nYes, agreed. As noted I haven't had time to dig deeply into this, but\nfrom what I've seen so far there doesn't seem to be any obvious way\nforward in terms of a quick fix.\n\nI thought perhaps your patch would be that (but I haven't looked into it\ncarefully enough), but since you're on-board with reverting & retrying.\n"},{"id":"441753","messageId":"d1509690-0341-b6ef-5fd4-878ebbdfaf39@web.de","threadId":"56723","inReplyTo":"211119.86mtm04og8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2021-11-19T16:08:06Z","receivedAt":"2021-11-19T16:08:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.11.21 um 08:00 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Thu, Nov 18 2021, René Scharfe wrote:\n>\n>> Am 18.11.21 um 19:17 schrieb Junio C Hamano:\n>>> Ævar Arnfjörð Bjarmason writes:\n>> [...]\n>>> I guess this is a lot of change in the amount of text involved but\n>>> the least amount of actual change in the behaviour. For those with\n>>> newer PCREv2, the behaviour would be the same as v2.34.0, and for\n>>> others, the behaviour would be the same as v2.33.0.\n>>>\n>>> Having said all that, because the consensus seems to be that the\n>>> whole \"when we should match in UTF mode\" may need to be rethought, I\n>>> think reverting Hamza's [v13 3/3] would be the simplest way to clean\n>>> up the mess for v2.34.1 that will give us a cleaner slate to later\n>>> build on, than applying this patch.\n>>\n>> Makes sense to me. It gives a better starting point to solve the issue\n>> afresh without getting entangled in mind-melting boolean expressions.\n>\n> Yes, agreed. As noted I haven't had time to dig deeply into this, but\n> from what I've seen so far there doesn't seem to be any obvious way\n> forward in terms of a quick fix.\n>\n> I thought perhaps your patch would be that (but I haven't looked into it\n> carefully enough), but since you're on-board with reverting & retrying.\n\nThat patch should fix the edge case without any side-effects -- at least\nI haven't seen any reports of ill effects that would apply to it. It's\neasier to understand and reason about when applied after reverting, I\nthink. But it's only for grep.c and I don't know the situation in t/.\n\nRené\n"},{"id":"441764","messageId":"xmqqczmwqdda.fsf@gitster.g","threadId":"56723","inReplyTo":"211119.86mtm04og8.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-19T17:11:45Z","receivedAt":"2021-11-19T17:11:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, Nov 18 2021, René Scharfe wrote:\n>>\n>> Makes sense to me. It gives a better starting point to solve the issue\n>> afresh without getting entangled in mind-melting boolean expressions.\n>\n> Yes, agreed. As noted I haven't had time to dig deeply into this, but\n> from what I've seen so far there doesn't seem to be any obvious way\n> forward in terms of a quick fix.\n>\n> I thought perhaps your patch would be that (but I haven't looked into it\n> carefully enough), but since you're on-board with reverting & retrying.\n\nOK. Here is what I'll queue 'master', in the hope of later merging\ndown and be part of v2.34.1.\n\nThanks, all.\n\n----- >8 --------- >8 --------- >8 --------- >8 -----\nSubject: [PATCH] Revert \"grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data\"\n\nThis reverts commit ae39ba431ab861548eb60b4bd2e1d8b8813db76f, as it\nbreaks \"grep\" when looking for a string in non UTF-8 haystack, when\nlinked with certain versions of PCREv2 library.\n\nSigned-off-by: Junio C Hamano \n---\n grep.c | 6 ++---\n t/t7812-grep-icase-non-ascii.sh | 48 ---------------------------------\n 2 files changed, 2 insertions(+), 52 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex f6e113e9f0..fe847a0111 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -382,10 +382,8 @@ 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 && !has_non_ascii(p->pattern)) ||\n-\t (!opt->ignore_locale && is_utf8_locale() &&\n-\t has_non_ascii(p->pattern) && !(!opt->ignore_case &&\n-\t\t\t\t\t (p->fixed || p->is_fixed))))\n+\tif (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&\n+\t !(!opt->ignore_case && (p->fixed || p->is_fixed)))\n \t\toptions |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);\n \n #ifdef GIT_PCRE2_VERSION_10_36_OR_HIGHER\ndiff --git a/t/t7812-grep-icase-non-ascii.sh b/t/t7812-grep-icase-non-ascii.sh\nindex 22487d90fd..e5d1e4ea68 100755\n--- a/t/t7812-grep-icase-non-ascii.sh\n+++ b/t/t7812-grep-icase-non-ascii.sh\n@@ -53,54 +53,6 @@ test_expect_success REGEX_LOCALE 'pickaxe -i on non-ascii' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success GETTEXT_LOCALE,PCRE 'log --author with an ascii pattern on UTF-8 data' '\n-\tcat >expected <<-\\EOF &&\n-\tAuthor: À Ú Thor \n-\tEOF\n-\ttest_write_lines \"forth\" >file4 &&\n-\tgit add file4 &&\n-\tgit commit --author=\"À Ú Thor \" -m sécond &&\n-\tgit log -1 --color=always --perl-regexp --author=\".*Thor\" >log &&\n-\tgrep Author log >actual.raw &&\n-\ttest_decode_color actual &&\n-\ttest_cmp expected actual\n-'\n-\n-test_expect_success GETTEXT_LOCALE,PCRE 'log --committer with an ascii pattern on ISO-8859-1 data' '\n-\tcat >expected <<-\\EOF &&\n-\tCommit: Ç O Mîtter \n-\tEOF\n-\ttest_write_lines \"fifth\" >file5 &&\n-\tgit add file5 &&\n-\tGIT_COMMITTER_NAME=\"Ç O Mîtter\" &&\n-\tGIT_COMMITTER_EMAIL=\"committer@example.com\" &&\n-\tgit -c i18n.commitEncoding=latin1 commit -m thïrd &&\n-\tgit -c i18n.logOutputEncoding=latin1 log -1 --pretty=fuller --color=always --perl-regexp --committer=\" O.*\" >log &&\n-\tgrep Commit: log >actual.raw &&\n-\ttest_decode_color actual &&\n-\ttest_cmp expected actual\n-'\n-\n-test_expect_success GETTEXT_LOCALE,PCRE 'log --grep with an ascii pattern on UTF-8 data' '\n-\tcat >expected <<-\\EOF &&\n-\t sécond\n-\tEOF\n-\tgit log -1 --color=always --perl-regexp --grep=\"con\" >log &&\n-\tgrep con log >actual.raw &&\n-\ttest_decode_color actual &&\n-\ttest_cmp expected actual\n-'\n-\n-test_expect_success GETTEXT_LOCALE,PCRE 'log --grep with an ascii pattern on ISO-8859-1 data' '\n-\tcat >expected <<-\\EOF &&\n-\t thïrd\n-\tEOF\n-\tgit -c i18n.logOutputEncoding=latin1 log -1 --color=always --perl-regexp --grep=\"th.*rd\" >log &&\n-\tgrep \"th.*rd\" log >actual.raw &&\n-\ttest_decode_color actual &&\n-\ttest_cmp expected actual\n-'\n-\n test_expect_success GETTEXT_LOCALE,LIBPCRE2 'PCRE v2: setup invalid UTF-8 data' '\n \tprintf \"\\\\200\\\\n\" >invalid-0x80 &&\n \techo \"ævar\" >expected &&\n-- \n2.34.0-202-gd9146917d7\n\n"},{"id":"441770","messageId":"CAPUEspiq2tdBoCic0mzEXaiut7bwra+pUzj2ArMVU2-ee45=SQ@mail.gmail.com","threadId":"56723","inReplyTo":"d1509690-0341-b6ef-5fd4-878ebbdfaf39@web.de","subject":"Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-11-19T17:33:22Z","receivedAt":"2021-11-19T17:33:35Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Nov 19, 2021 at 8:08 AM René Scharfe wrote:\n>\n> Am 19.11.21 um 08:00 schrieb Ævar Arnfjörð Bjarmason:\n> >\n> > On Thu, Nov 18 2021, René Scharfe wrote:\n> >\n> >> Am 18.11.21 um 19:17 schrieb Junio C Hamano:\n> >>> Ævar Arnfjörð Bjarmason writes:\n> >> [...]\n> >>> I guess this is a lot of change in the amount of text involved but\n> >>> the least amount of actual change in the behaviour. For those with\n> >>> newer PCREv2, the behaviour would be the same as v2.34.0, and for\n> >>> others, the behaviour would be the same as v2.33.0.\n> >>>\n> >>> Having said all that, because the consensus seems to be that the\n> >>> whole \"when we should match in UTF mode\" may need to be rethought, I\n> >>> think reverting Hamza's [v13 3/3] would be the simplest way to clean\n> >>> up the mess for v2.34.1 that will give us a cleaner slate to later\n> >>> build on, than applying this patch.\n> >>\n> >> Makes sense to me. It gives a better starting point to solve the issue\n> >> afresh without getting entangled in mind-melting boolean expressions.\n> >\n> > Yes, agreed. As noted I haven't had time to dig deeply into this, but\n> > from what I've seen so far there doesn't seem to be any obvious way\n> > forward in terms of a quick fix.\n> >\n> > I thought perhaps your patch would be that (but I haven't looked into it\n> > carefully enough), but since you're on-board with reverting & retrying.\n>\n> That patch should fix the edge case without any side-effects -- at least\n> I haven't seen any reports of ill effects that would apply to it.\n\nSince it isn't restricted to log, it will still cause a regression to\nthe `git grep` case with binary data for versions of PCRE2 older than\n10.34 and unlike the previous one it might not trigger an error in the\ntestsuite just because we are missing a test for it.\n\n> It's\n> easier to understand and reason about when applied after reverting, I\n> think. But it's only for grep.c and I don't know the situation in t/.\n\nWe had been focusing in PCRE in this discussion, but I see the strict\nbehaviour of older PCRE2 as just a \"coal mine canary\" to point to the\nbigger problem that we are expecting git's regex to handle safely and\ncorrectly from the point of UTF, what is technically a binary match\nwith --color making the mismatch obvious.\n\nThe issue is not unique to PCRE, and seems Ævar also acknowledges[1]\nthat by seeing the same bug this was attempting to fix with probably\nsome version of glibc's ERE. I suspect FreeBSD's (and derivatives) is\nalso broken and might be throwing REGILLSEQ errors as well, so I think\nthat it is better to revert the whole thing.\n\nCarlo\n\n[1] https://lore.kernel.org/git/211119.86r1bc4om5.gmgdl@evledraar.gmail.com/\n"}]}