Re: [PATCH v2 7/8] grep: simplify config parsing, change grep.<rx config> interaction

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

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

> Change the interaction between "grep.patternType=default" and
> "grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true"
> synonymous with setting "grep.patternType=extended".

This description alone is not quite understandable.  It is not
saying much more than the single line title, and presense of it does
not seem to improve the understanding by the readers.

> When "grep.patternType" was introduced in 84befcd0a4a (grep: add a
> grep.patternType configuration setting, 2012-08-03) we made two
> seemingly contradictory promises:
>
>  1. You can set "grep.patternType", and "[setting it to] 'default'
>     will return to the default matching behavior".
>
>  2. Support the existing "grep.extendedRegexp" option, but ignore it
>     when the new "grep.patternType" is set, *except* "when the
>     `grep.patternType` option is set. to a value other than 'default'".

OK, so setting grep.patternType=default makes grep.extendedRegexp to
be taken into account.  By grep.patternType to something else, the
other one is ignored.  2. is a very explicit way to say so.  Where
did you get 1. from?  If you have this paragraph in the log message
in mind, I agree that it is less than ideally phrased, but ...

    Rather than adding an additional setting for grep.fooRegexp for
    current and future pattern matching options, add a
    grep.patternType setting that can accept appropriate values for
    modifying the default grep pattern matching behavior. The
    current values are "basic", "extended", "fixed", "perl" and
    "default" for setting -G, -E, -F, -P and the default behavior
    respectively.

... with the understanding of 2. (which is in what the commit adds
to Documentation/config.txt), it is reasonable to understand that
"the default behaviour" is "use BRE or ERE, depending on the setting
of grep.extendedRegexp".

Doesn't the code behave that way?  I think the above is exactly how
the commit wanted to make the code behave.

> I think that 84befcd0a4a probably didn't intend this behavior, but
> instead ended up conflating our internal "unspecified" state with a
> user's explicit desire to set the configuration back to the
> default.

I am not sure where that comes from, but if I imagine somebody
confuses between "default" and "basic" and considers "default" a
synonym for "basic", I can sort-of understand it.  Is it what is
happening here?

But it is not what the original .patternType patch wanted to do back
then, and it is not what we want to see now.

> I.e. a user would correctly expect this to keep working:
>
>     # ERE grep
>     git -c grep.extendedRegexp=true grep <pattern>

This makes sense.

> And likewise for "grep.patternType=default" to take precedence over
> the disfavored "grep.extendedRegexp" option, i.e. the usual "last set
> wins" semantics.
>
>     # BRE grep
>     git -c grep.extendedRegexp=true -c grep.patternType=basic grep <pattern>

This makes sense, too.

Do either of the above two not work as you expect (i.e. the first
use ERE and the second use BRE)?

What I have trouble with is that it is unclear if you are describing
what should happen (in the above, I said "makes sense", to show my
agreement, assuming that it is the case), or if you are describing
what does happen that you disagree with. 

Another thing I have trouble with is your mention of "keep working".
Are you proposing to deliberately break what is working as users
correctly expect?  Why?

> But probably not for this to ignore the favored "grep.patternType"
> option entirely, say if /etc/gitconfig was still setting
> "grep.extendedRegexp", but "~/.gitconfig" used the new
> "grep.patternType" (and wanted to use the "default" value):
>
>     # Was ERE, now BRE
>     git -c grep.extendedRegexp=true grep.patternType=default grep <pattern>

I do not quite get your "Was X, now Y" label.  What did you want to
say with that?

Also I am not sure what you exactly mean when you say "and wanted to
use the 'default' value".  There is no single "THE" default value.
If patternType=default is the last patterntype (it may be set in
many places, but the last one should win), the user is telling that
the last-one-wins setting of extendedRegexp is to be honored.  So,
if grep.extendedRegexp in /etc/gitconfig is the last one defined, we
would choose between BRE and ERE depending on that setting.

Isn't that what is happening in the current code?

Or are all the above explanation result of simple misunderstanding
that setting to "default" means setting to "basic"?