Re: [PATCH 7/8] grep: simplify config parsing, change grep.<rx config> interaction
From: Taylor Blau <hidden>
Date: 2021-11-08 23:04:05
On Sat, Nov 06, 2021 at 10:10:53PM +0100, Ævar Arnfjörð Bjarmason wrote:
I.e. a user would correctly expect this to keep working:
# ERE grep
git -c grep.extendedRegexp=true grep <pattern>
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>
But probably not for this to ignore the new "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>OK, so this is the case that we'd be "breaking". And I think that the new behavior you're outlining here (where a higher-precedence grep.patternType=default overrides a lower-precedence grep.extendedRegexp=true, resulting in using BRE over ERE) makes more sense. At least, it makes more sense if your expectation of "default" is "the default matching behavior", not "fallthrough to grep.extendedRegexp". In any case, I am sensitive to breaking existing user workflows, but this seems so obscure to me that I have a hard time expecting that m(any?) users will even notice this at all. The situation I'm most concerned about is having grep.extendedRegexp set in, say, /etc/gitconfig and grep.patternType=default set at a higher-precedence level.
--- Documentation/config/grep.txt | 3 +- Documentation/git-grep.txt | 3 +-
Not the fault of your patch, but these two are annoyingly (and subtly) different from one another. Could we clean this up and put everything in Documentation/config/grep.txt (and then include that in the CONFIGURATION section of Documentation/git-grep.txt)?
quoted hunk ↗ jump to hunk
builtin/grep.c | 10 ++--- grep.c | 71 +++++------------------------------ grep.h | 6 +-- revision.c | 2 - t/t7810-grep.sh | 2 +- 7 files changed, 17 insertions(+), 80 deletions(-)diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index 44abe45a7ca..2669b1757d3 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt@@ -12,8 +12,7 @@ grep.patternType:: grep.extendedRegexp:: If set to true, enable `--extended-regexp` option by default. This - option is ignored when the `grep.patternType` option is set to a value - other than 'default'. + option is ignored when the `grep.patternType` option is set. grep.threads:: Number of grep worker threads to use.diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt index 3d393fbac1b..078dfeadf50 100644 --- a/Documentation/git-grep.txt +++ b/Documentation/git-grep.txt@@ -348,8 +348,7 @@ grep.patternType:: grep.extendedRegexp:: If set to true, enable `--extended-regexp` option by default. This - option is ignored when the `grep.patternType` option is set to a value - other than 'default'. + option is ignored when the `grep.patternType` option is set.
Makes sense, and matches your description.
quoted hunk ↗ jump to hunk
diff --git a/grep.c b/grep.c index fb3f63c63ef..dda8e536fe3 100644 --- a/grep.c +++ b/grep.c@@ -60,8 +60,10 @@ int grep_config(const char *var, const char *value, void *cb) if (userdiff_config(var, value) < 0) return -1; - if (!strcmp(var, "grep.extendedregexp")) { - opt->extended_regexp_option = git_config_bool(var, value); + if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED && + !strcmp(var, "grep.extendedregexp") && + git_config_bool(var, value)) { + opt->pattern_type_option = GREP_PATTERN_TYPE_ERE; return 0; }
And here's our "as long as we haven't set the pattern type already via grep.patternType, allow grep.extendedRegexp to set it". But the same "only set when unspecified" condition *isn't* in place for grep.patternType, which is what makes us prefer values from that configuration over the other. Makes sense. Everything else looks good here, too. Thanks, Taylor