Re: [PATCH v2 20/29] grep: remove redundant `regflags &= ~REG_EXTENDED` assignments

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

On Mon, May 15, 2017 at 8:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Remove redundant assignments to the "regflags" variable. There are no
>> code paths that have previously set the regflags to anything, and
>> certainly not to `|= REG_EXTENDED`.
>>
>> This code gave the impression that it had to reset its environment,
>> but it doesn't. This dates back to the initial introduction of
>> git-grep in commit 5010cb5fcc ("built-in "git grep"", 2006-04-30).
>
> Back in 5010cb5fcc, we did do "opt.regflags &= ~REG_EXTENDED" upon
> seeing "-G" on the command line and flipped the bit on upon seeing
> "-E", but I think that was perfectly sensible and it would have been
> a bug if we didn't.  They were part of the command line parsing that
> could have seen "-E" on the command line earlier.
>
> If we want to find a commit to assign blame to, I think it is more
> correct to say that this came from the same one as 19/29 fixes.
>
> When cca2c172 ("git-grep: do not die upon -F/-P when
> grep.extendedRegexp is set.", 2011-05-09) switched the command line
> parsing to "read into a 'tentatively this is what we saw the last'
> variable and then finally commit just once", we didn't touch
> opt.regflags for PCRE and FIXED, but we still had to flip regflags
> between BRE and ERE, because parsing of grep.extendedregexp
> configuration variable directly touched opt.regflags back then,
> which was done by b22520a3 ("grep: allow -E and -n to be turned on
> by default via configuration", 2011-03-30).  When 84befcd0 ("grep:
> add a grep.patternType configuration setting", 2012-08-03)
> introduced extended_regexp_option field, we stopped flipping
> regflags while reading the configuration, and that was when we
> should have noticed and stopped dropping REG_EXTENDED bit in the
> "now we can commit what type to use" helper function.

Thanks for the history. I'll amend the commit message to note this.

> In any case, I think this change is safe to do in the current
> codebase.  I wonder if this and 19/29 should be a single patch,
> though, as the unnecessary bit-flipping all are blamed to the same
> origin.

Squashing these two makes sense. I'll do that.

>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>>  grep.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/grep.c b/grep.c
>> index 59ae7809f2..bf6c2494fd 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -179,7 +179,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
>>       case GREP_PATTERN_TYPE_BRE:
>>               opt->fixed = 0;
>>               opt->pcre = 0;
>> -             opt->regflags &= ~REG_EXTENDED;
>>               break;
>>
>>       case GREP_PATTERN_TYPE_ERE:
>> @@ -191,7 +190,6 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
>>       case GREP_PATTERN_TYPE_FIXED:
>>               opt->fixed = 1;
>>               opt->pcre = 0;
>> -             opt->regflags &= ~REG_EXTENDED;
>>               break;
>>
>>       case GREP_PATTERN_TYPE_PCRE:
>> @@ -414,10 +412,9 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
>>  {
>>       struct strbuf sb = STRBUF_INIT;
>>       int err;
>> -     int regflags;
>> +     int regflags = opt->regflags;
>>
>>       basic_regex_quote_buf(&sb, p->pattern);
>> -     regflags = opt->regflags & ~REG_EXTENDED;
>>       if (opt->ignore_case)
>>               regflags |= REG_ICASE;
>>       err = regcomp(&p->regexp, sb.buf, regflags);