Re: [PATCH v2 5/7] grep: un-break building with PCRE < 8.32

From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Jeff King" <peff@peff.net>,
	"Jeffrey Walton" <noloader@gmail.com>,
	"Michał Kiedrowicz" <michal.kiedrowicz@gmail.com>,
	"J Smith" <dark.panda@gmail.com>,
	"Victor Leschuk" <vleschuk@gmail.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Fredrik Kuivinen" <frekui@gmail.com>,
	"Brandon Williams" <bmwill@google.com>,
	"Stefan Beller" <sbeller@google.com>,
	"Johannes Schindelin" <johannes.schindelin@gmx.de>,
	"Simon Ruderich" <simon@ruderich.org>
Subject: Re: [PATCH v2 5/7] grep: un-break building with PCRE < 8.32
Date: Wed, 24 May 2017 15:00:33 +0900	[thread overview]
Message-ID: <xmqqh90ahjzy.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <20170523192453.14172-6-avarab@gmail.com> (=?utf-8?B?IsOGdmFy?= =?utf-8?B?IEFybmZqw7Zyw7A=?= Bjarmason"'s message of "Tue, 23 May 2017 19:24:51 +0000")

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Amend my change earlier in this series ("grep: add support for the
> PCRE v1 JIT API", 2017-04-11) to un-break the build on PCRE v1
> versions earlier than 8.32.
> ...
> So just take the easy way out and disable the JIT on any version older
> than 8.32.

The above were very understandable, but I had quite a hard time
parsing first sentence of the next paragraph, especially everything
after "because".  In the end I think I figured out what you wanted
to say (so you do not have to explain it to me in a response), but I
wish there were an easier-to-understand way to write the same thing.

> The reason this change isn't part of the initial change PCRE JIT
> support is because possibly slightly annoying someone who's bisecting
> with an ancient PCRE is worth it to have a cleaner history showing
> which parts of the implementation are only used for ancient PCRE
> versions. This also makes it easier to revert this change if we ever
> decide to stop supporting those old versions.
>
> 1. http://www.pcre.org/original/changelog.txt ("28. Introducing a
>    native interface for JIT. Through this interface, the
>    compiled[...]")
> 2. https://bugs.exim.org/show_bug.cgi?id=2121
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  grep.c | 8 ++++----
>  grep.h | 5 +++++
>  2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 49e9aed457..3c0c30f033 100644
> --- a/grep.c
> +++ b/grep.c
> ...
> @@ -418,7 +418,7 @@ static void free_pcre1_regexp(struct grep_pat *p)
>  {
>  	pcre_free(p->pcre1_regexp);
>  
> -#ifdef PCRE_CONFIG_JIT
> +#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT
>  	if (p->pcre1_jit_on) {
>  		pcre_free_study(p->pcre1_extra_info);
>  		pcre_jit_stack_free(p->pcre1_jit_stack);
> diff --git a/grep.h b/grep.h
> index 14f47189f9..73ef0ef8ec 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -3,6 +3,11 @@
>  #include "color.h"
>  #ifdef USE_LIBPCRE1
>  #include <pcre.h>
> +#ifdef PCRE_CONFIG_JIT
> +#if PCRE_MAJOR >= 8 && PCRE_MINOR >= 32
> +#define GIT_PCRE1_CAN_DO_MODERN_JIT
> +#endif
> +#endif
>  #ifndef PCRE_STUDY_JIT_COMPILE
>  #define PCRE_STUDY_JIT_COMPILE 0
>  #endif

After reading the patch, I do not necessarily agree with your
pros-and-cons between keeping the patches as separate steps and
squashing them into one, though.  Even if this were squashed into
[PATCH 4/7], the logic to set GIT_PCRE1_CAN_DO_MODERN_JIT based on
PCRE_CONFIG_JIT and PCRE's version in this patch is well isolated to
a single place, and it is easy to spot what needs to be done when we
decide to lose the version-based GIT_PCRE1_CAN_DO_MODERN_JIT from
our code after making sure that nobody uses versions older than
8.32.

By the time we will make such a decision, it is likely that we no
longer remember if "#ifdef GIT_PCRE1_CAN_DO_MODERN_JIT" we have in
our code at that moment in the future were all already present in
this patch.  An update to drop support for older PCRE is likely to
be done by finding the above part in grep.h to remove the
version-dependent part, while still keeping #ifdef based on
GIT_PCRE1_CAN_DO_MODERN_JIT in the *.c code.  Being able to revert
this patch does not help there.

We might also do a find-and-replace of GIT_PCRE1_CAN_DO_MODERN_JIT
to PCRE_CONFIG_JIT when we drop support for older PCRE, but I do not
think we would assume that it is sufficient to revert this patch
when we do so.  We may have added more #ifdef on the GIT_MODERN_JIT
symbol we now need to change to PCRE_CONFIG_JIT, so that will be
done by find-and-replace of the then-current code, not by reverting
this patch.

So in that sense, I do not think keeping them separate in practice
has the "makes it easier to revert this change" benefit.

If I were doing these two patches, I'd squash them together into
one, rename GIT_PCRE1_CAN_DO_MODERN_JIT to GIT_PCRE1_USE_JIT, and
explain in the log message why we turn it off for versions older
than 8.32 like you did in the log message for thsi patch.  

The reason for the "rename" is because I might also be tempted to
allow users of newer version to manually decline GIT_PCRE1_USE_JIT
in Makefile/config.mak; i.e. we may decide not to USE something even
if we CAN, and the #ifdef symbol you are using is about the decision
to USE or not USE, not necessarily if the library CAN.

Thanks.