Re: [PATCH v2 4/8] grep: consistently use "p->fixed" in compile_regexp()

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Jul 29, 2019, 09:05 UTC
Message-ID: <871ry96wiq.fsf@evledraar.gmail.com>
In-Reply-To: <CAPUEspgay3RnLH3pdEWyktgn8XeuiKZ8PYPNB_38gyxffmh5Jw@mail.gmail.com>

On Mon, Jul 29 2019, Carlo Arenas wrote:

Show 28 quoted lines

> On Fri, Jul 26, 2019 at 8:09 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>>
>> It's less confusing to use that variable consistently that switch back
>> & forth between the two.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  grep.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/grep.c b/grep.c
>> index 9c2b259771..b94e998680 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -616,7 +616,7 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>>                 die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2"));
>>
>>         pat_is_fixed = is_fixed(p->pattern, p->patternlen);
>> -       if (opt->fixed || pat_is_fixed) {
>> +       if (p->fixed || pat_is_fixed) {
>
> at the end of this series we have:
>
>   if (p->fixed || p->is_fixed)
>
> which doesn't make sense; at least with opt->fixed it was clear that
> what was meant is that grep was passed -P

I assume you mean "was passed -F...".

> maybe is_fixed shouldn't exist and fixed when applied to the pattern
> means we had determined it was a fixed
> pattern and overridden the user selection of engine.

They're two flags because p->fixed is "--fixed-strings", and p->is_fixed is "there's no metachars here". So the former case needs escaping, as the code just below might do (the two aren't mutually exclusive).

I don't get how you think we can always fold them into one flag, but maybe I'm missing something...

Show 8 quoted lines

> that at least will give us a logical way to fix the pattern reported
> in [1] and that currently requires the user to know
> git's grep internals and know he can skip the "is_fixed" optimization
> by doing something like :
>
>   $ git grep 'foo[ ]bar'
>
> [1] https://public-inbox.org/git/20190728235427.41425-1-carenas@gmail.com/

As I noted in a reply there this seems like a way to fix a bug in "next" with a config knob. Yes we should fix the bug, but we've had the kwset code in git for years without needing this distinction, so after we work out the bugs I don't see why we'd need this.

The reason we ignore the user's choice here is because you might e.g. set grep.patternType=extended in your config, and you'd still want grepping for a fixed "foo" to be fast.