Re: [PATCH 5/8] builtin/config: canonicalize "value_regex" with `--type=bool`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 21, 2019, 05:37 UTC
- Message-ID
- <xmqqtv6x94d0.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <beb0cfbb78e3ae69e64ff2ed57c8e628ccb187ce.1573670565.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 7 quoted lines
> 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.
Show 18 quoted lines
> 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.
Show 20 quoted lines
> + 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.
Show 56 quoted lines
> 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]