Re: [PATCH v9 9/9] grep: simplify config parsing and option parsing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 27, 2022, 20:30 UTC
- Message-ID
- <xmqq35l82a7i.fsf@gitster.g>
- In-Reply-To
- <patch-v9-9.9-445467e87f7-20220127T115058Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 16 quoted lines
> In a preceding commit we changed grep_config() to be called after > grep_init(), which means that much of the complexity here can go > away. > > As before both "grep.patternType" and "grep.extendedRegexp" are > last-one-wins variable, with "grep.extendedRegexp" yielding to > "grep.patternType", except when "grep.patternType=default". > > Note that this applies as we parse the config, i.e. a sequence of: > > -c grep.patternType=perl > -c grep.extendedRegexp=true \ > -c grep.patternType=default > > should select ERE due to "grep.extendedRegexp=true and > grep.patternType=default".
Correct. The same reasoning would apply to:
-c grep.extendedRegexp=true \ -c grep.patternType=perl \ -c grep.patternType=default
which should also select ERE due to "grep.extendedRegexp=true and grep.patternType=default". As we re-check grep.extendedRegexp every time grep.patternType changes, and we keep extendedRegexp as a separate copy without getting lost like earlier rounds of this series, this should work.
Would this also work?
-c grep.extendedRegexp=false \ -c grep.patternType=default \ -c grep.extendedRegexp=true
We do keep extendedRegexp, but as soon as we read .patternType that is default, adjust_pattern_type() overwrites the pattern_type_option member with BRE, and the fact that .patternType was specified as "do whatever the .extendedRegexp says" is lost when we read the third one.
So, no, I am not sure this is correct.
Show 13 quoted lines
> diff --git a/grep.c b/grep.c
> index 60228a95a4f..f07a21ff1aa 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -48,6 +48,12 @@ static int parse_pattern_type_arg(const char *opt, const char *arg)
>
> define_list_config_array_extra(color_grep_slots, {"match"});
>
> +static void adjust_pattern_type(enum grep_pattern_type *pto, const int ero)
> +{
> + if (*pto == GREP_PATTERN_TYPE_UNSPECIFIED)
> + *pto = ero ? GREP_PATTERN_TYPE_ERE : GREP_PATTERN_TYPE_BRE;
> +}OK. Earlier rounds just replaced the UNSPECIFIED with hardcoded value "0", which was more or less pointless. I think this is easier to follow.
But as I said, "committing" ERE vs BRE in this manner is probably way too early and produce an incorrect result. Instead ...
> @@ -490,9 +446,9 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
... this is the right place to do the "see if pattern_type_option is 'default' and if so use 'extended_regexp_option' to commit to either BRE or ERE".
I guess that is what I have been repeating during the review of the past few rounds. Am I overlooking some other cases where that simpler-to-explain approach does not work?
Show 52 quoted lines
> 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 (!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"));
>
> @@ -544,14 +500,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 89a2ce51130..f89324e9aa9 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;
> int no_body_match;
> int body_hit;
> @@ -154,7 +153,6 @@ struct grep_opt {
> int allow_textconv;
> int extended;
> int use_reflog_filter;
> - int pcre2;
> int relative;
> int pathname;
> int null_following_name;
> @@ -183,6 +181,7 @@ struct grep_opt {
> .relative = 1, \
> .pathname = 1, \
> .max_depth = -1, \
> + .extended_regexp_option = -1, \I do not think you meant this. Uninitialized grep.extendedRegexp defaults to 0 (BRE), I think.
Show 24 quoted lines
> .pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED, \
> .colors = { \
> [GREP_COLOR_CONTEXT] = "", \
> @@ -202,7 +201,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 d6e0e2b23b5..dd301e30147 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2860,8 +2860,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);