| Message ID | 0f48ab4fc344b3cc226d0a45d13530022208ff3e.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 try to canonicalize the "value_regex". If it > doesn't canonicalize, we continue and handle the "value_regex" as an > ordinary regex. This is deliberate -- we do not want to cause existing > scripts to fail. > > This does mean that users might be at risk of missing out on config > values depending on which representation they use in their config file: > > $ git config foo.bar on > $ git config foo.baz true > $ git config --type=bool --get-regex "foo\.*" tru > foo.baz true > $ # oops! missing foo.bar > > Let's start warning when the "value_regex" doesn't look like a boolean. > Document our intention of eventually requiring the canonicalization to > pass. While I actually think this warning is counter-productive, if we were to do so, value-regex given for a bool-or-int type should also be warned if it does not name a boolean value and is not an integer. With the way you laid out the "use enum to tell what kind of token value_regex argument has" pattern, I think support for "--type=int" to normalize human-readable numbers would also fall out naturally, which is nice.
On Thu, 21 Nov 2019 at 06:43, Junio C Hamano <gitster@pobox.com> wrote: > > Martin Ågren <martin.agren@gmail.com> writes: > > > With `--type=bool`, we try to canonicalize the "value_regex". If it > > doesn't canonicalize, we continue and handle the "value_regex" as an > > ordinary regex. This is deliberate -- we do not want to cause existing > > scripts to fail. > > > > This does mean that users might be at risk of missing out on config > > values depending on which representation they use in their config file: > > > > $ git config foo.bar on > > $ git config foo.baz true > > $ git config --type=bool --get-regex "foo\.*" tru > > foo.baz true > > $ # oops! missing foo.bar > > > > Let's start warning when the "value_regex" doesn't look like a boolean. > > Document our intention of eventually requiring the canonicalization to > > pass. > > While I actually think this warning is counter-productive, if we > were to do so, value-regex given for a bool-or-int type should also > be warned if it does not name a boolean value and is not an integer. Ok, noted. > With the way you laid out the "use enum to tell what kind of token > value_regex argument has" pattern, I think support for "--type=int" > to normalize human-readable numbers would also fall out naturally, > which is nice. Well, that's what I was hoping for at least... ;-) Martin
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt index 864375b1ec..43310ca3c0 100644 --- a/Documentation/git-config.txt +++ b/Documentation/git-config.txt @@ -46,6 +46,8 @@ unset an existing `--type` specifier with `--no-type`. With `--type=bool` or `--type=bool-or-int`, if `value_regex` is given and canonicalizes to a boolean value, it matches all entries that canonicalize to the same boolean value. +The support for non-canonicalizing values of `value_regex` with +`--type=bool` is deprecated. When reading, the values are read from the system, global and repository local configuration files by default, and options diff --git a/builtin/config.c b/builtin/config.c index 4e274d4867..2af62b95f8 100644 --- a/builtin/config.c +++ b/builtin/config.c @@ -303,6 +303,8 @@ static int handle_value_regex(const char *regex_) cmd_line_value.boolean = boolval; return 0; } + warning(_("value_regex '%s' cannot be canonicalized " + "to a boolean value"), regex_); } if (type == TYPE_BOOL_OR_INT) { diff --git a/t/t1300-config.sh b/t/t1300-config.sh index f0e9a21dc4..3e067c211d 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -466,8 +466,9 @@ test_expect_success '--get canonicalizes integer value_regex with --type=bool' ' test_expect_success '--type=bool with "non-bool" value_regex' ' echo true >expect && - git config --type=bool --get foo.y4 "t.*" >output && + git config --type=bool --get foo.y4 "t.*" >output 2>err && test_cmp expect output && + test_i18ngrep "cannot be canonicalized" err && test_must_fail git config --type=bool --get foo.y4 "T.*" >output && test_must_be_empty output '
With `--type=bool`, we try to canonicalize the "value_regex". If it doesn't canonicalize, we continue and handle the "value_regex" as an ordinary regex. This is deliberate -- we do not want to cause existing scripts to fail. This does mean that users might be at risk of missing out on config values depending on which representation they use in their config file: $ git config foo.bar on $ git config foo.baz true $ git config --type=bool --get-regex "foo\.*" tru foo.baz true $ # oops! missing foo.bar Let's start warning when the "value_regex" doesn't look like a boolean. Document our intention of eventually requiring the canonicalization to pass. Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- Documentation/git-config.txt | 2 ++ builtin/config.c | 2 ++ t/t1300-config.sh | 3 ++- 3 files changed, 6 insertions(+), 1 deletion(-)