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

From: Junio C Hamano <gitster@pobox.com>
Date: May 24, 2017, 06:00 UTC
Message-ID: <xmqqh90ahjzy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170523192453.14172-6-avarab@gmail.com>

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

Show 6 quoted lines

> 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.

Show 48 quoted lines

> 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.