| Message ID | patch-7.8-677a8f8520f-20211106T210711Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | grep: simplify & delete code by changing obscure cfg variable behavior | expand |
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)? > 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. > 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
On Mon, Nov 08 2021, Taylor Blau wrote: > 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. *nod*, but the only user who'd end up with that is someone who's trying to override grep.extendedRegexp but failing to do it, so this would help. Or someone who'd read the docs, understood that we promised that would do nothing, and inserted that just to test us, but that seems unlikely :) Or, I suppose someone who's entirely confused, and will continue being even more confused now that behavior changes on a git upgrade from ERE to BRE. I'm hoping the last two paragraphs describe no-one & that this is safe to do. >> --- >> 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)? I've got a large series to do that for all of these, but opted to skip that particular digression here (even for just grep.txt it's a bit distracting).
On Tue, Nov 09, 2021 at 03:01:12AM +0100, Ævar Arnfjörð Bjarmason wrote: > Or someone who'd read the docs, understood that we promised that would > do nothing, and inserted that just to test us, but that seems unlikely > :) > > Or, I suppose someone who's entirely confused, and will continue being > even more confused now that behavior changes on a git upgrade from ERE > to BRE. > > I'm hoping the last two paragraphs describe no-one & that this is safe > to do. Yeah, I agree that it seems very unlikely that anybody would actually be affected here, so I'm comfortable with the change. > >> --- > >> 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)? > > I've got a large series to do that for all of these, but opted to skip > that particular digression here (even for just grep.txt it's a bit > distracting). I think that you could include a preparatory clean-up in this series to make the Documentation/config/grep.txt included by Documentation/git-grep.txt without being too distracting. But if punting on it now works better for you, I don't think either makes a reviewer's job substantially different. Thanks, Taylor
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. grep.threads:: Number of grep worker threads to use. If unset (or set to 0), Git will diff --git a/builtin/grep.c b/builtin/grep.c index 7f95f44e948..a4964baf9c0 100644 --- a/builtin/grep.c +++ b/builtin/grep.c @@ -849,7 +849,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) int i; int dummy; int use_index = 1; - int pattern_type_arg = GREP_PATTERN_TYPE_UNSPECIFIED; int allow_revs; struct option options[] = { @@ -883,16 +882,16 @@ int cmd_grep(int argc, const char **argv, const char *prefix) N_("descend at most <depth> levels"), PARSE_OPT_NONEG, NULL, 1 }, OPT_GROUP(""), - OPT_SET_INT('E', "extended-regexp", &pattern_type_arg, + OPT_SET_INT('E', "extended-regexp", &opt.pattern_type_option, N_("use extended POSIX regular expressions"), GREP_PATTERN_TYPE_ERE), - OPT_SET_INT('G', "basic-regexp", &pattern_type_arg, + OPT_SET_INT('G', "basic-regexp", &opt.pattern_type_option, N_("use basic POSIX regular expressions (default)"), GREP_PATTERN_TYPE_BRE), - OPT_SET_INT('F', "fixed-strings", &pattern_type_arg, + OPT_SET_INT('F', "fixed-strings", &opt.pattern_type_option, N_("interpret patterns as fixed strings"), GREP_PATTERN_TYPE_FIXED), - OPT_SET_INT('P', "perl-regexp", &pattern_type_arg, + OPT_SET_INT('P', "perl-regexp", &opt.pattern_type_option, N_("use Perl-compatible regular expressions"), GREP_PATTERN_TYPE_PCRE), OPT_GROUP(""), @@ -986,7 +985,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, grep_usage, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_STOP_AT_NON_OPTION); - grep_commit_pattern_type(pattern_type_arg, &opt); if (use_index && !startup_info->have_repository) { int fallback = 0; 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; } @@ -115,62 +117,6 @@ void grep_init(struct grep_opt *opt, struct repository *repo) opt->header_tail = &opt->header_list; } -static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt) -{ - /* - * When committing to the pattern type by setting the relevant - * fields in grep_opt it's generally not necessary to zero out - * the fields we're not choosing, since they won't have been - * set by anything. The extended_regexp_option field is the - * only exception to this. - * - * This is because in the process of parsing grep.patternType - * & grep.extendedRegexp we set opt->pattern_type_option and - * opt->extended_regexp_option, respectively. We then - * internally use opt->extended_regexp_option to see if we're - * compiling an ERE. It must be unset if that's not actually - * the case. - */ - if (pattern_type != GREP_PATTERN_TYPE_ERE && - opt->extended_regexp_option) - opt->extended_regexp_option = 0; - - switch (pattern_type) { - case GREP_PATTERN_TYPE_UNSPECIFIED: - /* fall through */ - - case GREP_PATTERN_TYPE_BRE: - break; - - case GREP_PATTERN_TYPE_ERE: - opt->extended_regexp_option = 1; - break; - - case GREP_PATTERN_TYPE_FIXED: - opt->fixed = 1; - break; - - case GREP_PATTERN_TYPE_PCRE: - opt->pcre2 = 1; - break; - } -} - -void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_opt *opt) -{ - if (pattern_type != GREP_PATTERN_TYPE_UNSPECIFIED) - grep_set_pattern_type_option(pattern_type, opt); - else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED) - grep_set_pattern_type_option(opt->pattern_type_option, opt); - else if (opt->extended_regexp_option) - /* - * This branch *must* happen after setting from the - * opt->pattern_type_option above, we don't want - * grep.extendedRegexp to override grep.patternType! - */ - grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt); -} - static struct grep_pat *create_grep_pat(const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t, @@ -492,9 +438,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt) p->word_regexp = opt->word_regexp; p->ignore_case = opt->ignore_case; - p->fixed = opt->fixed; + p->fixed = opt->pattern_type_option == GREP_PATTERN_TYPE_FIXED; - if (memchr(p->pattern, 0, p->patternlen) && !opt->pcre2) + if (opt->pattern_type_option != GREP_PATTERN_TYPE_PCRE && + memchr(p->pattern, 0, p->patternlen)) die(_("given pattern contains NULL byte (via -f <file>). This is only supported with -P under PCRE v2")); p->is_fixed = is_fixed(p->pattern, p->patternlen); @@ -545,14 +492,14 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt) return; } - if (opt->pcre2) { + if (opt->pattern_type_option == GREP_PATTERN_TYPE_PCRE) { compile_pcre2_pattern(p, opt); return; } if (p->ignore_case) regflags |= REG_ICASE; - if (opt->extended_regexp_option) + if (opt->pattern_type_option == GREP_PATTERN_TYPE_ERE) regflags |= REG_EXTENDED; err = regcomp(&p->regexp, p->pattern, regflags); if (err) { diff --git a/grep.h b/grep.h index 30a7dfd3294..e4e548aed90 100644 --- a/grep.h +++ b/grep.h @@ -143,7 +143,6 @@ struct grep_opt { int unmatch_name_only; int count; int word_regexp; - int fixed; int all_match; #define GREP_BINARY_DEFAULT 0 #define GREP_BINARY_NOMATCH 1 @@ -152,7 +151,6 @@ struct grep_opt { int allow_textconv; int extended; int use_reflog_filter; - int pcre2; int relative; int pathname; int null_following_name; @@ -161,8 +159,7 @@ struct grep_opt { int max_depth; int funcname; int funcbody; - int extended_regexp_option; - int pattern_type_option; + enum grep_pattern_type pattern_type_option; int ignore_locale; char colors[NR_GREP_COLORS][COLOR_MAXLEN]; unsigned pre_context; @@ -201,7 +198,6 @@ struct grep_opt { int grep_config(const char *var, const char *value, void *); void grep_init(struct grep_opt *, struct repository *repo); -void grep_commit_pattern_type(enum grep_pattern_type, struct grep_opt *opt); void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t); void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t); diff --git a/revision.c b/revision.c index 9f9b0d2429e..ed29d245c89 100644 --- a/revision.c +++ b/revision.c @@ -2864,8 +2864,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s diff_setup_done(&revs->diffopt); - grep_commit_pattern_type(GREP_PATTERN_TYPE_UNSPECIFIED, - &revs->grep_filter); if (!is_encoding_utf8(get_log_output_encoding())) revs->grep_filter.ignore_locale = 1; compile_grep_patterns(&revs->grep_filter); diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh index 6b6423a07c3..a59a9726357 100755 --- a/t/t7810-grep.sh +++ b/t/t7810-grep.sh @@ -443,7 +443,7 @@ do ' test_expect_success "grep $L with grep.extendedRegexp=true and grep.patternType=default" ' - echo "${HC}ab:abc" >expected && + echo "${HC}ab:a+bc" >expected && git \ -c grep.extendedRegexp=true \ -c grep.patternType=default \
Change the interaction between "grep.patternType=default" and "grep.extendedRegexp=true" to make setting "grep.extendedRegexp=true" synonymous with setting "grep.patternType=extended". This changes our existing config parsing behavior as detailed below, but in a way that's consistent with how we parse other configuration. Pedantically speaking we're probably breaking past promises here, but I doubt that this will impact anyone in practice. The reduction in complexity and resulting consistency with other default config behavior is worth it. 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'". 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.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> I think that in practice nobody or almost nobody is going to be relying on this obscure interaction, and as shown here it makes the config parsing much simpler. We no longer have to carry a complex state machine in "grep_commit_pattern_type()" and "grep_set_pattern_type_option()". We can also do away with the "int fixed" and "int pcre2" members in favor of using "pattern_type_option" directly in "grep.c", as well as dropping the "pattern_type_arg" variable in "builtin/grep.c" in favor of using the "pattern_type_option" member directly. See my 07a3d411739 (grep: remove regflags from the public grep_opt API, 2017-06-29) for addition of the two comments being removed here, i.e. the complexity noted in that commit is now going away. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- Documentation/config/grep.txt | 3 +- Documentation/git-grep.txt | 3 +- 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(-)