Re: [PATCH v13 3/3] grep/pcre2: fix an edge case concerning ascii patterns and UTF-8 data

From: Carlo Arenas <carenas@gmail.com>
Date: Nov 19, 2021, 17:33 UTC
Message-ID: <CAPUEspiq2tdBoCic0mzEXaiut7bwra+pUzj2ArMVU2-ee45=SQ@mail.gmail.com>
In-Reply-To: <d1509690-0341-b6ef-5fd4-878ebbdfaf39@web.de>

On Fri, Nov 19, 2021 at 8:08 AM René Scharfe <l.s.r@web.de> wrote:

Show 31 quoted lines

>
> Am 19.11.21 um 08:00 schrieb Ævar Arnfjörð Bjarmason:
> >
> > On Thu, Nov 18 2021, René Scharfe wrote:
> >
> >> Am 18.11.21 um 19:17 schrieb Junio C Hamano:
> >>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> >> [...]
> >>> I guess this is a lot of change in the amount of text involved but
> >>> the least amount of actual change in the behaviour.  For those with
> >>> newer PCREv2, the behaviour would be the same as v2.34.0, and for
> >>> others, the behaviour would be the same as v2.33.0.
> >>>
> >>> Having said all that, because the consensus seems to be that the
> >>> whole "when we should match in UTF mode" may need to be rethought, I
> >>> think reverting Hamza's [v13 3/3] would be the simplest way to clean
> >>> up the mess for v2.34.1 that will give us a cleaner slate to later
> >>> build on, than applying this patch.
> >>
> >> Makes sense to me.  It gives a better starting point to solve the issue
> >> afresh without getting entangled in mind-melting boolean expressions.
> >
> > Yes, agreed. As noted I haven't had time to dig deeply into this, but
> > from what I've seen so far there doesn't seem to be any obvious way
> > forward in terms of a quick fix.
> >
> > I thought perhaps your patch would be that (but I haven't looked into it
> > carefully enough), but since you're on-board with reverting & retrying.
>
> That patch should fix the edge case without any side-effects -- at least
> I haven't seen any reports of ill effects that would apply to it.

Since it isn't restricted to log, it will still cause a regression to the `git grep` case with binary data for versions of PCRE2 older than 10.34 and unlike the previous one it might not trigger an error in the testsuite just because we are missing a test for it.

> It's
> easier to understand and reason about when applied after reverting, I
> think.  But it's only for grep.c and I don't know the situation in t/.

We had been focusing in PCRE in this discussion, but I see the strict behaviour of older PCRE2 as just a "coal mine canary" to point to the bigger problem that we are expecting git's regex to handle safely and correctly from the point of UTF, what is technically a binary match with --color making the mismatch obvious.

The issue is not unique to PCRE, and seems Ævar also acknowledges[1] that by seeing the same bug this was attempting to fix with probably some version of glibc's ERE. I suspect FreeBSD's (and derivatives) is also broken and might be throwing REGILLSEQ errors as well, so I think that it is better to revert the whole thing.

Carlo

[1] https://lore.kernel.org/git/211119.86r1bc4om5.gmgdl@evledraar.gmail.com/