grep: remove regflags from the public grep_opt API

Refactor calls to the grep machinery to always pass opt.ignore_case & opt.extended_regexp_option instead of setting the equivalent regflags bits. The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log: make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was really just plastering over the code smell which this change fixes. The reason for adding the extensive commentary here is that I discovered some subtle complexity in implementing this that really should be called out explicitly to future readers. Before this change we'd rely on the difference between `extended_regexp_option` and `regflags` to serve as a membrane between our preliminary parsing of grep.extendedRegexp and grep.patternType, and what we decided to do internally. Now that those two are the same thing, it's necessary to unset `extended_regexp_option` just before we commit in cases where both of those config variables are set. See 84befcd0a4 ("grep: add a grep.patternType configuration setting", 2012-08-03) for the code and documentation related to that. The explanation of why the if/else branches in grep_commit_pattern_type() are ordered the way they are exists in that commit message, but I think it's worth calling this subtlety out explicitly with a comment for future readers. Even though grep_commit_pattern_type() is the only caller of grep_set_pattern_type_option() it's simpler to reset the extended_regexp_option flag in the latter, since 2/3 branches in the former would otherwise need to reset it, this way we can do it in one place. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Jun 29, 2017 at 22:22 UTC 07a3d4117390841ed6884a9eac836918491df0a9
4 files changed +34 -14
builtin/grep.c
-2
@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
1169
1170 if (!opt.pattern_list)
1171 die(_("no pattern given."));
1172 - if (!opt.fixed && opt.ignore_case)
1173 - opt.regflags |= REG_ICASE;
1172
1173 /*
1174 * We have to find "--" in a separate pass, because its presence
grep.c
+34 -9
@@ -35,7 +35,6 @@ void init_grep_defaults(void)
35 memset(opt, 0, sizeof(*opt));
36 opt->relative = 1;
37 opt->pathname = 1;
38 - opt->regflags = REG_NEWLINE;
38 opt->max_depth = -1;
39 opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
40 color_set(opt->color_context, "");
@@ -153,7 +152,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
152 opt->linenum = def->linenum;
153 opt->max_depth = def->max_depth;
154 opt->pathname = def->pathname;
156 - opt->regflags = def->regflags;
155 opt->relative = def->relative;
156 opt->output = def->output;
157
@@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
167
168 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, struct grep_opt *opt)
169 {
170 + /*
171 + * When committing to the pattern type by setting the relevant
172 + * fields in grep_opt it's generally not necessary to zero out
173 + * the fields we're not choosing, since they won't have been
174 + * set by anything. The extended_regexp_option field is the
175 + * only exception to this.
176 + *
177 + * This is because in the process of parsing grep.patternType
178 + * & grep.extendedRegexp we set opt->pattern_type_option and
179 + * opt->extended_regexp_option, respectively. We then
180 + * internally use opt->extended_regexp_option to see if we're
181 + * compiling an ERE. It must be unset if that's not actually
182 + * the case.
183 + */
184 + if (pattern_type != GREP_PATTERN_TYPE_ERE &&
185 + opt->extended_regexp_option)
186 + opt->extended_regexp_option = 0;
187 +
188 switch (pattern_type) {
189 case GREP_PATTERN_TYPE_UNSPECIFIED:
190 /* fall through */
@@ -177,7 +193,7 @@ static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, st
193 break;
194
195 case GREP_PATTERN_TYPE_ERE:
180 - opt->regflags |= REG_EXTENDED;
196 + opt->extended_regexp_option = 1;
197 break;
198
199 case GREP_PATTERN_TYPE_FIXED:
@@ -207,6 +223,11 @@ void grep_commit_pattern_type(enum grep_pattern_type pattern_type, struct grep_o
223 else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
224 grep_set_pattern_type_option(opt->pattern_type_option, opt);
225 else if (opt->extended_regexp_option)
226 + /*
227 + * This branch *must* happen after setting from the
228 + * opt->pattern_type_option above, we don't want
229 + * grep.extendedRegexp to override grep.patternType!
230 + */
231 grep_set_pattern_type_option(GREP_PATTERN_TYPE_ERE, opt);
232 }
233
@@ -572,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
593 {
594 struct strbuf sb = STRBUF_INIT;
595 int err;
575 - int regflags = opt->regflags;
596 + int regflags = REG_NEWLINE;
597
598 basic_regex_quote_buf(&sb, p->pattern);
599 if (opt->ignore_case)
@@ -591,12 +612,12 @@ static void compile_fixed_regexp(struct grep_pat *p, struct grep_opt *opt)
612
613 static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
614 {
594 - int icase, ascii_only;
615 + int ascii_only;
616 int err;
617 + int regflags = REG_NEWLINE;
618
619 p->word_regexp = opt->word_regexp;
620 p->ignore_case = opt->ignore_case;
599 - icase = opt->regflags & REG_ICASE || p->ignore_case;
621 ascii_only = !has_non_ascii(p->pattern);
622
623 /*
@@ -614,10 +635,10 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
635 if (opt->fixed ||
636 has_null(p->pattern, p->patternlen) ||
637 is_fixed(p->pattern, p->patternlen))
617 - p->fixed = !icase || ascii_only;
638 + p->fixed = !p->ignore_case || ascii_only;
639
640 if (p->fixed) {
620 - p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
641 + p->kws = kwsalloc(p->ignore_case ? tolower_trans_tbl : NULL);
642 kwsincr(p->kws, p->pattern, p->patternlen);
643 kwsprep(p->kws);
644 return;
@@ -641,7 +662,11 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
662 return;
663 }
664
644 - err = regcomp(&p->regexp, p->pattern, opt->regflags);
665 + if (p->ignore_case)
666 + regflags |= REG_ICASE;
667 + if (opt->extended_regexp_option)
668 + regflags |= REG_EXTENDED;
669 + err = regcomp(&p->regexp, p->pattern, regflags);
670 if (err) {
671 char errbuf[1024];
672 regerror(err, &p->regexp, errbuf, 1024);
grep.h
-1
@@ -162,7 +162,6 @@ struct grep_opt {
162 char color_match_selected[COLOR_MAXLEN];
163 char color_selected[COLOR_MAXLEN];
164 char color_sep[COLOR_MAXLEN];
165 - int regflags;
165 unsigned pre_context;
166 unsigned post_context;
167 unsigned last_shown;
revision.c
-2
@@ -1362,7 +1362,6 @@ void init_revisions(struct rev_info *revs, const char *prefix)
1362 init_grep_defaults();
1363 grep_init(&revs->grep_filter, prefix);
1364 revs->grep_filter.status_only = 1;
1365 - revs->grep_filter.regflags = REG_NEWLINE;
1365
1366 diff_setup(&revs->diffopt);
1367 if (prefix && !revs->diffopt.prefix) {
@@ -2022,7 +2021,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
2021 revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_ERE;
2022 } else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
2023 revs->grep_filter.ignore_case = 1;
2025 - revs->grep_filter.regflags |= REG_ICASE;
2024 DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
2025 } else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
2026 revs->grep_filter.pattern_type_option = GREP_PATTERN_TYPE_FIXED;