| Message ID | patch-v2-8.8-cc904d93b26-20211110T013632Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | grep: simplify & delete code by changing obscure cfg variable behavior | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > In the preceding commit we changed how a "grep.patternType=default" > set after "grep.extendedRegexp=true" would be handled so that the last > set would win, but a "grep.extendedRegexp=true" would only be used if > "grep.patternType" was set to a value other than "default". I am getting the feeling that my suspicion about your confusion in 7/8 is right. The grep.patternType=default was merely a backward compatibility measure for those who were used to grep.extendedRegexp=true/false way of doing things, and "default" never meant to mean "basic". It merely was an instruction to "honor the old extendedRegexp variable for this old timer". The intention all along was that patternType was a more flexible single true way to set the type to supersede extendedRegexp but we couldn't discontinue the support for the latter all of a sudden, and the "default" was invented as the transition mechanism, but it ended up as the mechanism to choose either new (setting patternType to some other value) or old (patternType is set to default, making the value given to extendedRegexp is the only thing that matters) world to live in. If you want to change anything in this area, the right thing to do is rather to _deprecate_ extendedRegexp and eventually remove extendedRegexp together with the "default" setting to patternType, I would think, not to change the semantics of extendedRegexp in any way. Thanks.
diff --git a/Documentation/config/grep.txt b/Documentation/config/grep.txt index f4b7d3041fb..33e5f3827bc 100644 --- a/Documentation/config/grep.txt +++ b/Documentation/config/grep.txt @@ -11,8 +11,7 @@ grep.patternType:: value 'default' will return to the default matching behavior. grep.extendedRegexp:: - If set to true, enable `--extended-regexp` option by default. This - option is ignored when the `grep.patternType` option is set. + Deprecated synonym for 'grep.patternType=extended`. grep.threads:: Number of grep worker threads to use. If unset (or set to 0), Git will diff --git a/grep.c b/grep.c index dda8e536fe3..ef8746d85f0 100644 --- a/grep.c +++ b/grep.c @@ -33,9 +33,8 @@ static const char *color_grep_slots[] = { static int parse_pattern_type_arg(const char *opt, const char *arg) { - if (!strcmp(arg, "default")) - return GREP_PATTERN_TYPE_UNSPECIFIED; - else if (!strcmp(arg, "basic")) + if (!strcmp(arg, "basic") || + !strcmp(arg, "default")) return GREP_PATTERN_TYPE_BRE; else if (!strcmp(arg, "extended")) return GREP_PATTERN_TYPE_ERE; @@ -60,8 +59,7 @@ int grep_config(const char *var, const char *value, void *cb) if (userdiff_config(var, value) < 0) return -1; - if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED && - !strcmp(var, "grep.extendedregexp") && + if (!strcmp(var, "grep.extendedregexp") && git_config_bool(var, value)) { opt->pattern_type_option = GREP_PATTERN_TYPE_ERE; return 0; diff --git a/grep.h b/grep.h index e4e548aed90..8ef70d125ff 100644 --- a/grep.h +++ b/grep.h @@ -94,8 +94,7 @@ enum grep_expr_node { }; enum grep_pattern_type { - GREP_PATTERN_TYPE_UNSPECIFIED = 0, - GREP_PATTERN_TYPE_BRE, + GREP_PATTERN_TYPE_BRE = 0, GREP_PATTERN_TYPE_ERE, GREP_PATTERN_TYPE_FIXED, GREP_PATTERN_TYPE_PCRE @@ -179,7 +178,6 @@ struct grep_opt { .relative = 1, \ .pathname = 1, \ .max_depth = -1, \ - .pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \ .colors = { \ [GREP_COLOR_CONTEXT] = "", \ [GREP_COLOR_FILENAME] = "", \ diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index a59a9726357..afca938a4d0 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -461,7 +461,7 @@ do ' test_expect_success "grep $L with grep.patternType=basic and grep.extendedRegexp=true" ' - echo "${HC}ab:a+bc" >expected && + echo "${HC}ab:abc" >expected && git \ -c grep.patternType=basic \ -c grep.extendedRegexp=true \
In the preceding commit we changed how a "grep.patternType=default" set after "grep.extendedRegexp=true" would be handled so that the last set would win, but a "grep.extendedRegexp=true" would only be used if "grep.patternType" was set to a value other than "default". Thus a user who had old config and set "grep.extendedRegexp=true" in their ~/.gitconfig expecting ERE behavior would be opted-in to say "perl" regexes if a system "/etc/gitconfig" started setting "grep.patternType=perl". These funny semantics of only paying attention to a set if another key is not set to a given value aren't how we treat other config keys, so let's do away with this caveat for consistency. The new semantics are simple, a "grep.extendedRegexp=true" is an exact synonym for specifying "grep.patternType=extended" in the config. We'll keep ignoring ""grep.extendedRegexp=false", although arguably we could treat it as a "grep.patternType=basic". As argued in the preceding commit I think this behavior came about because we were conflating the state of our code's own internal "default" value with what we found in explicit user config. See 84befcd0a4a (grep: add a grep.patternType configuration setting, 2012-08-03) for that past behavior. Let's further change the documentation to note that "grep.extendedRegexp" is a deprecated synonym, perhaps we'll be able to remove it at some point in the future and do away with this special-case entirely. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config/grep.txt | 3 +-- grep.c | 8 +++----- grep.h | 4 +--- t/t7810-grep.sh | 2 +- 4 files changed, 6 insertions(+), 11 deletions(-)