Re: [PATCH 7/8] grep: simplify config parsing, change grep.<rx config> interaction

[Taylor Blau <me@ttaylorr.com>]

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