Re: [PATCH 11/12] grep: change the internal PCRE code & header names to be PCRE1

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Apr 11, 2017 at 6:51 PM, Brandon Williams <bmwill@google.com> wrote:
> On 04/11, Jeff King wrote:
>> On Tue, Apr 11, 2017 at 01:02:56PM +0200, Ęvar Arnfjörš Bjarmason wrote:
>>
>> > >> Yes, this is a bug. I'll need to add a git_options along with
>> > >> submodule_options and pass -c grep.patternType=....
>> > >
>> > > Maybe that's an indication we should have --pcre1-regexp and
>> > > --pcre2-regexp, so we don't have to resort to config tweaking.
>> >
>> > I'd rather not. To reply to both your
>> > <20170411103018.dkq5gangx3vcxhp4@sigill.intra.peff.net> & this, one
>> > thing I was trying to do in this series (and I don't think I went far
>> > enough in "grep & rev-list doc: stop promising libpcre for
>> > --perl-regexp") was to stop promising some specific version of PCRE.
>>
>> We don't necessarily have to document them. This is just in the general
>> rule of "if there's config, there should be command-line to override
>> it". Because without that, you get this exact situation where you have
>> to bolt on "-c" options to another part of the command line, which gets
>> awkward.
>>
>> I'm also not sure it would be strictly correct, if the sub-program runs
>> other sub-programs. Providing "-c" affects all child processes, whereas
>> command-line options are propagated manually. So imagine you have a
>> process tree like:
>>
>>   grep
>>    \-grep
>>       \-textconv
>>
>> I.e., grep recurses to a submodule which then has to kick off a textconv
>> filter for one of the files. If you use "-c" to pass options to the
>> second grep, then those options will continue to have an effect inside
>> the textconv filter. Which _probably_ doesn't run git commands that
>> would care, but technically it could do anything.
>>
>> > I.e. as far as the user is concerned they just want perl-y regexes,
>> > but they most likely don't care about the 1% featureset of those
>> > regexes where the various implementations of "perl-y regex" actually
>> > differ, because those cases tend to be really obscure syntax.
>>
>> Yeah, that's what led me to the "why are we even worrying about run-time
>> switching" direction. I'd think a build-time switch would be enough for
>> people to test, and it makes all of this type of complexity go away.
>
> Yeah I agree with Jeff that we should probably avoid needing to pass a
> config option down in addition to a command line switch to do perl
> regex's.  I didn't take too hard of a look at how that would be done in
> the grep code, but it might be slightly more involved than just changing
> the enum name.
>
> From [12/12] it looks like the main purpose of this series is to use a
> more preferment version of PCRE, if all else is equal it doesn't really
> make much sense to have both versions to be select-able at runtime.  Is
> there any benefit of being able to do that, that I'm missing?

Not really no. I don't think any git user is ever going to be using
both pcre1 & pcre2 at runtime.

This grew more organically out of how I started to hack the code. Due
to how different the two APIs are it's much less messier to have a new
set of wrapper functions than to ifdef around v1 & v2. Once I had all
the code & config flags it was easy to shimmy it up so I could switch
between the two, and it was handy for performance testing.

But the guy developing it is hardly the main target audience for a
feature like this, but on the other hand it's easy to support...

I'm hacking up a v2 of this series. It includes some extra goodies
like bugfixes, v1 JIT support, and I'll try to tack a patch at the end
that removes this facility to switch between the two at runtime, and
we can see if that looks better than not having it.