On Fri, Oct 08 2021, Hamza Mahfooz wrote:
> If we attempt to grep non-ascii log message text with an ascii pattern, we
> run into the following issue:
>
> $ git log --color --author='.var.*Bjar' -1 origin/master | grep ^Author
> grep: (standard input): binary file matches
>
> So, to fix this teach the grep code to mark the pattern as UTF-8 (even if
> the pattern is composed of only ascii characters), so long as the log
> output is encoded using UTF-8.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> Signed-off-by: Hamza Mahfooz <someguy@xxxxxxxxxxxxxxxxxxx>
> ---
> v12: get rid of utf8_all_the_things and fix an issue with one of the unit
> tests.
> ---
> grep.c | 6 +++--
> t/t7812-grep-icase-non-ascii.sh | 48 +++++++++++++++++++++++++++++++++
> 2 files changed, 52 insertions(+), 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index fe847a0111..f6e113e9f0 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -382,8 +382,10 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
> }
> options |= PCRE2_CASELESS;
> }
> - if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
> - !(!opt->ignore_case && (p->fixed || p->is_fixed)))
> + if ((!opt->ignore_locale && !has_non_ascii(p->pattern)) ||
> + (!opt->ignore_locale && is_utf8_locale() &&
> + has_non_ascii(p->pattern) && !(!opt->ignore_case &&
> + (p->fixed || p->is_fixed))))
> options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
I think at least some of that existing "if" is my fault, and I *think*
your patch works here, but FWIW I'd find something like this way more
readable:
@@ -382,8 +383,16 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt
}
options |= PCRE2_CASELESS;
}
+ if (opt->utf8_all_the_things) {
+ options |= PCRE2_UCP;
+ do_utf8 = 1;
+ }
+
if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
!(!opt->ignore_case && (p->fixed || p->is_fixed)))
+ do_utf8 = 1;
+
+ if (do_utf8)
options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
Well, without the "utf8_all_the_things" probably. That's a reference to
a popular meme, probably better to name it differently, and the
PCRE2_UCP is just something I was experimenting with.
It's late here, but I've got to admit that I'm still a bit confused by
this. Let's see if I can try to sum up why:
Ultimately whether we use PCRE2_UTF *should* have nothing do to with
whether the pattern is UTF-8 or not, because even an expression like:
/.*/
Will behave differently under UTF-8, i.e. character classes change, byte
boundaries change to "character" boundaries etc.
That the existing code has has_non_ascii() and the like is a trade-off
that had to be made for the grep-a-file case, because you might run into
arbitrary binary data, but logs are cleaner/encoded/re-encoded etc.
If you run PCRE in UTF-8 mode it will die on some of that data (as
you'll see from our test suite if you turn it on unconditionally).
Are there cases where my "utf8_all_the_things" POC patch would have
turned on PCRE2_UTF, but yours doesn't? IOW is there a 1=1 mapping still
between the encoding mode log/revision.c thinks it's in & PCRE2_UTF?
Anyway, I've tried to break things with this patch and haven't
succeeded, so maybe it's all OK now, thanks a lot for working on this
again, it's a really neat feature. Just wondering if there's any
remaining edge cases we know about...
