Re: [PATCH v5 7/7] grep: simplify config parsing and option parsing

[Junio C Hamano <gitster@pobox.com>]

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

> Simplify the parsing of "grep.patternType" and
> "grep.extendedRegexp". This changes no behavior, but gets rid of
> complex parsing logic that isn't needed anymore.
>
> When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
> grep.patternType configuration setting, 2012-08-03) we promised that:
>
>  1. You can set "grep.patternType", and "[setting it to] 'default'
>     will return to the default matching behavior".

You need to call it into readers' attention that back then the
author of that commit meant by "the default" to mean "whatever the
configuration system specified before the patch applied, i.e. with
grep.extendedRegexp".

>  2. We'd support the existing "grep.extendedRegexp" option, but ignore
>     it when the new "grep.patternType" option is set. We said we'd
>     only ignore the older "grep.extendedRegexp" option "when the
>     `grep.patternType` option is set. to a value other than
>     'default'".

Yes, in short, you can think of it this way.

 - We use grep.patternType as the source of truth.  It is a usual
   last-one-wins variable, with values like fixed, basic, pcre,
   etc.

 - There however is a special value 'default', which may mean
   'basic' or 'extended', depending on what grep.extendedRegexp is
   set to.

> In a preceding commit we changed grep_config() to be called after
> grep_init(), which means that much of the complexity here can go
> away.
>
> Now as before when we only understand a "grep.extendedRegexp" setting
> of "true", and if "grep.patterntype=default" is set we'll interpret it
> as "grep.patterntype=basic",

Is that a typo?  If extendedRegexp is set to 'true', then the
'default' would mean 'extended', so I would expect that we'd
see it as the same as setting it 'grep.patternType=extended'.

> except if we previously saw a
> "grep.extendedRegexp", then it's interpreted as
> "grep.patterntype=extended".

I am not sure what this means.  grep.extendedRegexp is also a usual
last-one-wins variable.  If you had this series:

    git \
    -c grep.extendedRegexp = false \
    -c grep.extendedRegexp = true \
    -c grep.patternType = default \
    some-command-that-take-regexp

the last grep.extendedRegexp is true, and the last grep.patternType
is default, so the command sould work on extended.  It is also true
if you had

    git \
    -c grep.extendedRegexp = false \
    -c grep.patternType = default \
    -c grep.extendedRegexp = true \
    some-command-that-take-regexp

That is why it is important to remember the fact that patternType
was give as "default" before you finish reading the configuration
and you are sure you know the last value of grep.extendedRegexp.
Only after that, you can resolve what that "default" means between
"basic" and "extended".  Trying to interpret "default" as soon as
you see it in grep.patternType and trying to make it into either
"basic" or "extended" will not work unless you know you have the
final value of grep.extendedRegexp.

> We don't need grep_commit_pattern_type() anymore,...

And I think that is what this function wanted to say: "we now have
seen all necessary, so we can commit what pattern type we are going
to use; before this point, we couldn't tell what 'default' meant".

So I am not sure how any change that says we do not need the
"commit" phase can be correct.