Am 16.12.22 um 23:52 schrieb Junio C Hamano: > Mathias Krause <minipli@xxxxxxxxxxxxxx> writes: > >> Such a change was already proposed 4 years ago [1] but wasn't merged for >> unknown reasons. >> >> 1. https://lore.kernel.org/r/20181209230024.43444-3-carenas@xxxxxxxxx > > This part does not belong to the log message but should go below > three-dash line. If you have a more concrete "it was rejected > because ...", to help judging why this version improves upon the > previous attempt and it is clear the reason for past rejection no > longer applies, that is a very different story, though. > > The way I read the original thread (assuming that the lore archive > is showing all relevant messages there) is that RFC was reviewed > well and everybody was happy about the direction it took. It even > received fairly concrete suggestions for the real, non-RFC version, > but that never materialized. It had a follow-up RFC[1] half a year later, adding a config option and, after some more discussion, even command line switches[2]. But not just IMHO is that far too much, but even you suggested to simply revert back to the initial RFC and implement the automatic fallback[3], basically merging it with a proper changelog[4]. As that never happened, I took up the work and tried to do just that. 1. https://lore.kernel.org/git/20190728235427.41425-1-carenas@xxxxxxxxx/ 2. https://lore.kernel.org/git/20190729105955.44390-1-carenas@xxxxxxxxx/ 3. https://lore.kernel.org/git/xmqqh874vikk.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/ 4. https://lore.kernel.org/git/xmqqef1zmkp5.fsf@xxxxxxxxxxxxxxxxxxxxxxxxx/ > It is very clear that the posted patch was not yet in a mergeable > state as-is; "for unknown reasons" is less than helpful. > > Now, we learned a more concrete reason, i.e. "it got tons of help > improving the draft into the final version, but the help was > discarded and the final version did not materialize", does the > attempt this time around improve on it sufficiently to make it > mergeable, or is it sufficiently different that it needs a fresh > review from scratch? If the latter, then "for unknown reasons" > becomes even less relevant. Sorry for not digging up that information for the initial patch submission. I'll add it to v2. > The rest of the proposed log message, and the change itself, both > look very reasonable and well explained, at least to me. Thanks a lot for the detailed review. I'll update the commit log accordingly but will wait for more feedback to see on which solution we want to settle on. > Thanks. > > >> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@xxxxxxxxx> >> Signed-off-by: Mathias Krause <minipli@xxxxxxxxxxxxxx> # tweaked changelog, added comment >> --- >> grep.c | 17 +++++++++++++++-- >> 1 file changed, 15 insertions(+), 2 deletions(-) >> >> diff --git a/grep.c b/grep.c >> index 06eed694936c..f2ada528b21d 100644 >> --- a/grep.c >> +++ b/grep.c >> @@ -317,8 +317,21 @@ static void compile_pcre2_pattern(struct grep_pat *p, const struct grep_opt *opt >> pcre2_config(PCRE2_CONFIG_JIT, &p->pcre2_jit_on); >> if (p->pcre2_jit_on) { >> jitret = pcre2_jit_compile(p->pcre2_pattern, PCRE2_JIT_COMPLETE); >> - if (jitret) >> - die("Couldn't JIT the PCRE2 pattern '%s', got '%d'\n", p->pattern, jitret); >> + if (jitret) { >> + /* >> + * Even though pcre2_config(PCRE2_CONFIG_JIT, ...) >> + * indicated JIT support, the library might still >> + * fail to generate JIT code for various reasons, >> + * e.g. when SELinux's 'deny_execmem' or PaX's >> + * MPROTECT prevent creating W|X memory mappings. >> + * >> + * Instead of faling hard, fall back to interpreter >> + * mode, just as if the pattern was prefixed with >> + * '(*NO_JIT)'. >> + */ >> + p->pcre2_jit_on = 0; >> + return; >> + } >> >> /* >> * The pcre2_config(PCRE2_CONFIG_JIT, ...) call just
