Re: [PATCH v2 7/8] grep: simplify config parsing, change grep.<rx config> interaction
From: Junio C Hamano <hidden>
Date: 2021-11-12 19:19:32
Ævar Arnfjörð Bjarmason [off-list ref] 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"?