From: Junio C Hamano <hidden> Date: 2021-11-18 18:18:06
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
Let's have a look at the map. Here are the differences between the
versions regarding use of PCRE2_UTF:
o: opt->ignore_locale
h: has_non_ascii(p->pattern)
i: is_utf8_locale()
l: !opt->ignore_case && (p->fixed || p->is_fixed)
o h i l master hamza rene2
0 0 0 0 0 1 0
0 0 0 1 0 1 0
0 0 1 0 0 1 1
0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging
So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/
should not have this breakage, because it doesn't enable PCRE2_UTF for
literal patterns.
PCRE2_UTF will also matter for literal patterns. Try to peel apart the
two bytes in "é" and match them under -i with/without PCRE_UTF.
Sorry for being late to the party, but doesn't "literal" in the
context of this thread mean the column "l" above, i.e. we are not
ignoring case and fixed or is_fixed member is set? So "under -i"
disqualifies as an example for "will also matter for literal",
doesn't it?
In hindsight, I guess we could have pushed a bit harder when René's
- if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
+ if (!opt->ignore_locale && is_utf8_locale() &&
!(!opt->ignore_case && (p->fixed || p->is_fixed)))
options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
in https://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/
(is that what is called 'rene2' above?) was raised on Oct 17th to
amend/fix Hamza's [v13 3/3]; that would have prevented 'master' from
having this breakage?
Carlo, in your [PATCH v2] in [off-list ref],
I see that the #else side for older PCREv2 users essentially reverts
what Hamza's [PATCH v13 3/3] did to this area.
+#else
+ if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
+ !(!opt->ignore_case && (p->fixed || p->is_fixed)))
+ options |= PCRE2_UTF;
+#endif
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.
So, I dunno. Comments from those involved in the discussion?
Thanks.
From: René Scharfe <hidden> Date: 2021-11-18 20:57:51
Am 18.11.21 um 19:17 schrieb Junio C Hamano:
Ævar Arnfjörð Bjarmason [off-list ref] writes:
quoted
quoted
Let's have a look at the map. Here are the differences between the
versions regarding use of PCRE2_UTF:
o: opt->ignore_locale
h: has_non_ascii(p->pattern)
i: is_utf8_locale()
l: !opt->ignore_case && (p->fixed || p->is_fixed)
o h i l master hamza rene2
0 0 0 0 0 1 0
0 0 0 1 0 1 0
0 0 1 0 0 1 1
0 0 1 1 0 1 0 <== 7812.13, confirmed using fprint() debugging
So http://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/
should not have this breakage, because it doesn't enable PCRE2_UTF for
literal patterns.
PCRE2_UTF will also matter for literal patterns. Try to peel apart the
two bytes in "é" and match them under -i with/without PCRE_UTF.
Sorry for being late to the party, but doesn't "literal" in the
context of this thread mean the column "l" above, i.e. we are not
ignoring case and fixed or is_fixed member is set? So "under -i"
disqualifies as an example for "will also matter for literal",
doesn't it?
Correct.
In hindsight, I guess we could have pushed a bit harder when René's
- if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
+ if (!opt->ignore_locale && is_utf8_locale() &&
!(!opt->ignore_case && (p->fixed || p->is_fixed)))
options |= (PCRE2_UTF | PCRE2_MATCH_INVALID_UTF);
in https://public-inbox.org/git/0ea73e7a-6d43-e223-ab2e-24c684102856@web.de/
(is that what is called 'rene2' above?) was raised on Oct 17th to
amend/fix Hamza's [v13 3/3]; that would have prevented 'master' from
having this breakage?
Yes, that the change I meant with "rene2".
Carlo, in your [PATCH v2] in [off-list ref],
I see that the #else side for older PCREv2 users essentially reverts
what Hamza's [PATCH v13 3/3] did to this area.
+#else
+ if (!opt->ignore_locale && is_utf8_locale() && has_non_ascii(p->pattern) &&
+ !(!opt->ignore_case && (p->fixed || p->is_fixed)))
+ options |= PCRE2_UTF;
+#endif
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.
René
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.
From: René Scharfe <hidden> Date: 2021-11-19 16:08:25
Am 19.11.21 um 08:00 schrieb Ævar Arnfjörð Bjarmason:
On Thu, Nov 18 2021, René Scharfe wrote:
quoted
Am 18.11.21 um 19:17 schrieb Junio C Hamano:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
[...]
quoted
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. 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/.
René
From: Carlo Arenas <hidden> Date: 2021-11-19 17:33:35
On Fri, Nov 19, 2021 at 8:08 AM René Scharfe [off-list ref] wrote:
Am 19.11.21 um 08:00 schrieb Ævar Arnfjörð Bjarmason:
quoted
On Thu, Nov 18 2021, René Scharfe wrote:
quoted
Am 18.11.21 um 19:17 schrieb Junio C Hamano:
quoted
Ævar Arnfjörð Bjarmason [off-list ref] writes:
[...]
quoted
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/