On Tue, Apr 11, 2017 at 6:51 PM, Brandon Williams <bmwill@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> & 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.