Re: [PATCH 5/8] builtin/config: canonicalize "value_regex" with `--type=bool`

[Junio C Hamano <gitster@pobox.com>]

Martin Ågren <martin.agren@gmail.com> writes:

> With `--type=bool`, we use the "value_regex" as a normal regex and do
> not use our knowledge about the different well-defined ways we have of
> spelling the boolean values. Let's consider a few examples.
>
> These output "true":
> 	git -c foo.bar=true config --type=bool --get foo.bar true
> 	git -c foo.bar=true config --type=bool --get foo.bar t

I wonder what

    git -c foo.bar=true config --type=bool --get foo.bar 't.*'

should say.  I think you and Peff discussed in the downthread to
make it more strict to require <value_regex> *NOT* regexp at all,
but one of the string representations of boolean we would take,
which means 't' (in your exmaple) and 't.*' would stop yielding
correct result, which I think is fine.

> Canonicalize the provided "value_regex", then canonicalize candidate
> values as we go through the config and compare the canonicalized values.

I think (for the reason stated earlier) this is OK, but we should
make sure that we are clear in the documentation that in this mode
<value_regex> is no longer a regex but a string.  We might even want
to have a separate SYNOPSIS entry that does not say regex at all,
something along the lines of ...

-'git config' [<file-option>] [--type=<type>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] [--show-origin] [-z|--null] --get name [value_regex]
+'git config' [<file-option>] --type=<type> [--show-origin] [-z|--null] --get name [value]

> If the parameter doesn't canonicalize, fall back to handling it as a
> regex, as before. This would happen in the second example above, but
> also if someone has hand-rolled their own canonicalisation with, e.g.,
> something like "^(on|On|ON|true|1|42)$".

I actually am OK with this fallback, too.  That would also mean the
additional SYNOPSIS entry unnecessary ;-).

> Do not rename the "value_regex" in the documentation. This commit can be
> seen as teaching `git config --type=bool` about a particular type of
> regex, where "true" matches "yes", but not "no". So arguably,
> "value_regex" still describes quite well what is going on.

Heh ;-) I probably should learn to blindly read thru the proposed
log message to the end before starting to think about ramifications
of the proposed changes myself.  Some of the reasoning in this
paragraph should be reflected to the explanation of the argument in
the documentation, so that the readers will know this is not a usual
regex at all.

> Signed-off-by: Martin Ågren <martin.agren@gmail.com>
> ---
>  Documentation/git-config.txt |  4 ++++
>  builtin/config.c             | 15 +++++++++++-
>  t/t1300-config.sh            | 45 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
> index 899e92a1c9..139750bbda 100644
> --- a/Documentation/git-config.txt
> +++ b/Documentation/git-config.txt
> @@ -43,6 +43,10 @@ outgoing values are canonicalize-able under the given <type>.  If no
>  `--type=<type>` is given, no canonicalization will be performed. Callers may
>  unset an existing `--type` specifier with `--no-type`.
>  
> +With `--type=bool`, if `value_regex` is given
> +and canonicalizes to a boolean value, it matches all entries
> +that canonicalize to the same boolean value.

... otherwise `value_regex` is used as a string, i.e. as if no
'--type=bool' is given.

> +	if (cmd_line_value.mode == boolean &&
> +	    git_parse_maybe_bool(value_) != cmd_line_value.boolean)
> +		return 0;
>  
>  	ALLOC_GROW(values->items, values->nr + 1, values->alloc);
>  	strbuf_init(&values->items[values->nr], 0);
> @@ -292,6 +296,15 @@ static int handle_value_regex(const char *regex_)
>  		return 0;
>  	}
>  
> +	if (type == TYPE_BOOL) {
> +		int boolval = git_parse_maybe_bool(regex_);
> +		if (boolval >= 0) {
> +			cmd_line_value.mode = boolean;
> +			cmd_line_value.boolean = boolval;
> +			return 0;
> +		}
> +	}
> +
>  	cmd_line_value.mode = regexp;

... which can be seen here.  OK.

> diff --git a/t/t1300-config.sh b/t/t1300-config.sh
> index a38cc143a1..e4906a893e 100755
> --- a/t/t1300-config.sh
> +++ b/t/t1300-config.sh
> @@ -427,6 +427,51 @@ test_expect_success 'no arguments, but no crash' '
>  	test_i18ngrep usage output
>  '
>  
> +test_expect_success 'setup config file with several boolean values' '
> +	cat >.git/config <<-\EOF
> +	[foo]
> +		n1 = no
> +		n2 = NO
> +		n3 = off
> +		n4 = false
> +		n5 = 0
> +		n6 =
> +		y1 = yes
> +		y2 = YES
> +		y3 = on
> +		y4 = true
> +		y5 = 1
> +		y6 = 42
> +		y7
> +	EOF
> +'
> +
> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (false)' '
> +	git config --type=bool --get-regexp "foo\..*" OFF >output &&
> +	test_line_count = 6 output &&
> +	! grep -v "^foo.n" output
> +'
> +
> +test_expect_success '--get-regexp canonicalizes value_regex with --type=bool (true)' '
> +	git config --type=bool --get-regexp "foo\..*" ON >output &&
> +	test_line_count = 7 output &&
> +	! grep -v "^foo.y" output
> +'
> +
> +test_expect_success '--get canonicalizes integer value_regex with --type=bool' '
> +	echo true >expect &&
> +	git config --type=bool --get foo.y2 1 >output &&
> +	test_cmp expect output
> +'
> +
> +test_expect_success '--type=bool with "non-bool" value_regex' '
> +	echo true >expect &&
> +	git config --type=bool --get foo.y4 "t.*" >output &&
> +	test_cmp expect output &&
> +	test_must_fail git config --type=bool --get foo.y4 "T.*" >output &&
> +	test_must_be_empty output
> +'
> +
>  test_expect_success 'setup simple config file' '
>  	q_to_tab >.git/config <<-\EOF
>  	[a.b]