Re: [PATCH v2 6/7] config: implement --fixed-value with --get*

[Emily Shaffer <emilyshaffer@google.com>]

On Mon, Nov 23, 2020 at 04:05:06PM +0000, Derrick Stolee via GitGitGadget wrote:
> 

Allowing myself a sensible chuckle at the commit subject using glob
syntax on a series about regex matching. ;)

> 
> The config builtin does its own regex matching of values for the --get,
> --get-all, and --get-regexp modes. Plumb the existing 'flags' parameter
> to the get_value() method so we can initialize the value_regex argument
> as a fixed string instead of a regex pattern.
> 
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/config.c  | 15 ++++++++++-----
>  t/t1300-config.sh | 22 ++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin/config.c b/builtin/config.c
> index 3e49e04411..d3772b5efe 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
>  	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
>  		return 0;
> +	if (fixed_value && strcmp(value_regex, (value_?value_:"")))
> +		return 0;
Ooh, I can see you're matching style, but the combination of the
spaceless ternary and the trailing underscore is making me so itchy ;)

>  	if (regexp != NULL &&
>  	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
>  		return 0;
> @@ -298,7 +301,7 @@ static int collect_config(const char *key_, const char *value_, void *cb)
>  	return format_config(&values->items[values->nr++], key_, value_);
>  }
>  
> -static int get_value(const char *key_, const char *regex_)
> +static int get_value(const char *key_, const char *regex_, int flags)
Very reasonable, and I appreciate passing 'flags' instead of passing
'fixed' here. Room for growth :)

[snip] a bunch of straightforward plumbing-through.

> +test_expect_success '--get and --get-all with --fixed-value' '
> +	GLOB="a+b*c?d[e]f.g" &&
> +	rm -f config &&
Again this tells me that your other tests want 'test_when_finished'
instead.

> +	git config --file=config fixed.test bogus &&
> +	git config --file=config --add fixed.test "$GLOB" &&
> +
> +	git config --file=config --get fixed.test bogus &&
> +	test_must_fail git config --file=config --get fixed.test "$GLOB" &&
> +	git config --file=config --get --fixed-value fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config --get --fixed-value fixed.test non-existent &&
> +
> +	git config --file=config --get-all fixed.test bogus &&
> +	test_must_fail git config --file=config --get-all fixed.test "$GLOB" &&
> +	git config --file=config --get-all --fixed-value fixed.test "$GLOB" &&
> +	test_must_fail git config --file=config --get-all --fixed-value fixed.test non-existent &&
> +
> +	git config --file=config --get-regexp fixed+ bogus &&
> +	test_must_fail git config --file=config --get-regexp fixed+ "$GLOB" &&
> +	git config --file=config --get-regexp --fixed-value fixed+ "$GLOB" &&
> +	test_must_fail git config --file=config --get-regexp --fixed-value fixed+ non-existent
> +'

Otherwise the test seems fine to me, although I wouldn't yell if it grew
some comments :)

With the exception of the 'test_when_finished', which might not even
match style (I didn't look):

Reviewed-by: Emily Shaffer <emilyshaffer@google.com>