Re: [PATCH v10 9/9] grep: simplify config parsing and option parsing

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 4, 2022, 23:41 UTC
Message-ID: <xmqqk0eai4i8.fsf@gitster.g>
In-Reply-To: <patch-v10-9.9-ccbdfa48315-20220204T211534Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

Show 19 quoted lines

> Note that as the previously added tests indicate this cannot be done
> on-the-fly as we see the config variables, without introducing more
> state keeping. I.e. if we see:
>
>     -c grep.extendedRegexp=false
>     -c grep.patternType=default
>     -c extendedRegexp=true
>
> We need to select ERE, since grep.patternType=default unselects that
> variable, which normally has higher precedence, but we also need to
> select BRE in cases of:
>
>     -c grep.extendedRegexp=true \
>     -c grep.extendedRegexp=false
>
> Which would not be the case for this, which select ERE:
>
>     -c grep.patternType=extended \
>     -c grep.extendedRegexp=false

I think the latter two examples can lose the backslash at the end (and all of them can lose "-c"). We can rewrite the preamble of the first one to clarify what we are trying to say with this notation, perhaps like

	I.e. if we see these configuration variable definitions in
	this order:

Show 55 quoted lines

> -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);
> -}

It is great that we can lose this, together with the associated fields like fixed and pcre2.

Show 8 quoted lines

> @@ -488,11 +432,16 @@ static void compile_regexp(struct grep_pat *p, struct grep_opt *opt)
>  	int err;
>  	int regflags = REG_NEWLINE;
>  
> +	if (opt->pattern_type_option == GREP_PATTERN_TYPE_UNSPECIFIED)
> +		opt->pattern_type_option = (opt->extended_regexp_option
> +					    ? GREP_PATTERN_TYPE_ERE
> +					    : GREP_PATTERN_TYPE_BRE);

It is nice that we can forget about .extended_regrexp_option member after this point, and .pattern_type_option will be the only thing that matters.

>  	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;

This makes readers wonder if we can further lose members in p (specifically .fixed), but cleaning up the grep_opt members is already a great progress.

Looking good. Other than minor tweaks I mentioned on the proposed log message, I didn't see anything wrong in this version.

Thanks.