| Message ID | beb0cfbb78e3ae69e64ff2ed57c8e628ccb187ce.1573670565.git.martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | builtin/config: canonicalize "value_regex" with `--type=bool[-or-int]` | expand |
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]
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. + When reading, the values are read from the system, global and repository local configuration files by default, and options `--system`, `--global`, `--local`, `--worktree` and diff --git a/builtin/config.c b/builtin/config.c index d812e73e2b..c9fe0c5752 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -15,9 +15,10 @@ static const char *const builtin_config_usage[] = { static char *key; static regex_t *key_regexp; static struct { - enum { none, regexp } mode; + enum { none, regexp, boolean } mode; regex_t *regexp; int do_not_match; /* used with `regexp` */ + int boolean; } cmd_line_value; static int show_keys; static int omit_values; @@ -278,6 +279,9 @@ static int collect_config(const char *key_, const char *value_, void *cb) !!regexec(cmd_line_value.regexp, value_ ? value_ : "", 0, NULL, 0))) return 0; + 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; if (regex_[0] == '!') { 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]
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 These produce no output: git -c foo.bar=true config --type=bool --get foo.bar True git -c foo.bar=true config --type=bool --get foo.bar 1 Canonicalize the provided "value_regex", then canonicalize candidate values as we go through the config and compare the canonicalized values. 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)$". This commit affects all modes that take a "value_regex", e.g., `--get-regexp` which can be used in a more useful way than the examples above might at first suggest: git config --type=bool --name-only --get-regexp '^foo\.' true This commit does change the behavior for certain usages, but it almost certainly does so for the better: We don't exclude config items based on how the config happens to spell "true" or "false". This commit would cause a regression if someone uses different synonyms for "true", knowing that Git handles them all the same in day-to-day operation, but relying on the encoding (using, say, integers 1-100) to signal some sort of confidence and using `git config` to query for various such "levels". I'm tempted to bet no-one will be hurt by this change. 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. 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(-)