* [PATCH 0/2] config: improve error message for boolean config @ 2020-09-18 2:17 Andrew Klotz via GitGitGadget 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Andrew Klotz via GitGitGadget @ 2020-09-18 2:17 UTC (permalink / raw) To: git; +Cc: Andrew Klotz Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value. The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out fromdie_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values. Signed-off-by: Andrew Klotz agc.klotz@gmail.com [agc.klotz@gmail.com] Andrew Klotz (2): config: improve error message for boolean config formatting for error messages config.c | 26 ++++++++++++++------------ t/t0205-gettext-poison.sh | 2 +- 2 files changed, 15 insertions(+), 13 deletions(-) base-commit: 3a238e539bcdfe3f9eb5010fd218640c1b499f7a Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v1 Pull-Request: https://github.com/git/git/pull/841 -- gitgitgadget ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/2] config: improve error message for boolean config 2020-09-18 2:17 [PATCH 0/2] config: improve error message for boolean config Andrew Klotz via GitGitGadget @ 2020-09-18 2:17 ` Andrew Klotz via GitGitGadget 2020-09-18 18:10 ` Jeff King 2020-09-18 18:30 ` Phillip Wood 2020-09-18 2:17 ` [PATCH 2/2] formatting for error messages Andrew Klotz via GitGitGadget ` (2 subsequent siblings) 3 siblings, 2 replies; 13+ messages in thread From: Andrew Klotz via GitGitGadget @ 2020-09-18 2:17 UTC (permalink / raw) To: git; +Cc: Andrew Klotz, Andrew Klotz From: Andrew Klotz <agc.klotz@gmail.com> Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int. `GIT_TEST_GETTEXT_POISON` is a boolean so we no longer fail on evaluating it as an int in `git_config_int`. Because of that we can move the special translation case into the boolean config check where we are now failing with an updated message before with an invalid boolean value of `non-boolean`, its unclear what numeric is referring to: ``` fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit ``` now the error message mentions `non-boolean` is a bad boolean value: ``` fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' ``` Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> --- config.c | 22 ++++++++++++---------- t/t0205-gettext-poison.sh | 2 +- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/config.c b/config.c index 2bdff4457b..198d0d3216 100644 --- a/config.c +++ b/config.c @@ -996,15 +996,6 @@ static void die_bad_number(const char *name, const char *value) if (!value) value = ""; - if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) - /* - * We explicitly *don't* use _() here since it would - * cause an infinite loop with _() needing to call - * use_gettext_poison(). This is why marked up - * translations with N_() above. - */ - die(bad_numeric, value, name, error_type); - if (!(cf && cf->name)) die(_(bad_numeric), value, name, _(error_type)); @@ -1097,7 +1088,18 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return v; } *is_bool = 0; - return git_config_int(name, value); + if (git_parse_int(value, &v)) + return v; + + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) + /* + * We explicitly *don't* use _() here since it would + * cause an infinite loop with _() needing to call + * use_gettext_poison(). + */ + die("bad boolean config value '%s' for '%s'", value, name); + else + die(_("bad boolean config value '%s' for '%s'"), value, name); } int git_config_bool(const char *name, const char *value) diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh index f9fa16ad83..b66d34c6f2 100755 --- a/t/t0205-gettext-poison.sh +++ b/t/t0205-gettext-poison.sh @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error " test_done -- gitgitgadget ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] config: improve error message for boolean config 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget @ 2020-09-18 18:10 ` Jeff King 2020-09-18 18:30 ` Phillip Wood 1 sibling, 0 replies; 13+ messages in thread From: Jeff King @ 2020-09-18 18:10 UTC (permalink / raw) To: Andrew Klotz via GitGitGadget; +Cc: git, Andrew Klotz On Fri, Sep 18, 2020 at 02:17:06AM +0000, Andrew Klotz via GitGitGadget wrote: > diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh > index f9fa16ad83..b66d34c6f2 100755 > --- a/t/t0205-gettext-poison.sh > +++ b/t/t0205-gettext-poison.sh > @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant > > test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " > test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && > - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error > + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error > " This test is pretty subtle and depends on gettext-poison's inner workings. I wonder if it's worth adding a separate: git config --bool --default=foo does.not.exist test in t1300 or similar. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] config: improve error message for boolean config 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget 2020-09-18 18:10 ` Jeff King @ 2020-09-18 18:30 ` Phillip Wood 2020-09-18 19:14 ` Junio C Hamano 1 sibling, 1 reply; 13+ messages in thread From: Phillip Wood @ 2020-09-18 18:30 UTC (permalink / raw) To: Andrew Klotz via GitGitGadget, git Cc: Andrew Klotz, Jeff King, Junio C Hamano Hi Andrew On 18/09/2020 03:17, Andrew Klotz via GitGitGadget wrote: > From: Andrew Klotz <agc.klotz@gmail.com> > > Currently invalid boolean config values return messages about 'bad > numeric', which is slightly misleading when the error was due to a > boolean value. We can improve the developer experience by returning a > boolean error message when we know the value is neither a bool text or > int. This patch improves things for boolean config keys but git_config_bool_or_int() is used by status.submoduleSummary, merge.log and commit.verbose which can be either a number or a boolean (where a boolean generally means use a default number). It would be a more invasive change but I wonder if it would be better for git_config_bool() to have it's own error message rather sharing it with git_config_bool_or_int(). Best Wishes Phillip > `GIT_TEST_GETTEXT_POISON` is a boolean so we no longer fail on > evaluating it as an int in `git_config_int`. Because of that we can > move the special translation case into the boolean config check where > we are now failing with an updated message > > before with an invalid boolean value of `non-boolean`, its unclear what > numeric is referring to: > ``` > fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit > ``` > > now the error message mentions `non-boolean` is a bad boolean value: > ``` > fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' > ``` > > Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> > --- > config.c | 22 ++++++++++++---------- > t/t0205-gettext-poison.sh | 2 +- > 2 files changed, 13 insertions(+), 11 deletions(-) > > diff --git a/config.c b/config.c > index 2bdff4457b..198d0d3216 100644 > --- a/config.c > +++ b/config.c > @@ -996,15 +996,6 @@ static void die_bad_number(const char *name, const char *value) > if (!value) > value = ""; > > - if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) > - /* > - * We explicitly *don't* use _() here since it would > - * cause an infinite loop with _() needing to call > - * use_gettext_poison(). This is why marked up > - * translations with N_() above. > - */ > - die(bad_numeric, value, name, error_type); > - > if (!(cf && cf->name)) > die(_(bad_numeric), value, name, _(error_type)); > > @@ -1097,7 +1088,18 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) > return v; > } > *is_bool = 0; > - return git_config_int(name, value); > + if (git_parse_int(value, &v)) > + return v; > + > + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) > + /* > + * We explicitly *don't* use _() here since it would > + * cause an infinite loop with _() needing to call > + * use_gettext_poison(). > + */ > + die("bad boolean config value '%s' for '%s'", value, name); > + else > + die(_("bad boolean config value '%s' for '%s'"), value, name); > } > > int git_config_bool(const char *name, const char *value) > diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh > index f9fa16ad83..b66d34c6f2 100755 > --- a/t/t0205-gettext-poison.sh > +++ b/t/t0205-gettext-poison.sh > @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant > > test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " > test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && > - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error > + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error > " > > test_done > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] config: improve error message for boolean config 2020-09-18 18:30 ` Phillip Wood @ 2020-09-18 19:14 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2020-09-18 19:14 UTC (permalink / raw) To: Phillip Wood; +Cc: Andrew Klotz via GitGitGadget, git, Andrew Klotz, Jeff King Phillip Wood <phillip.wood123@gmail.com> writes: > This patch improves things for boolean config keys but > git_config_bool_or_int() is used by status.submoduleSummary, merge.log > and commit.verbose which can be either a number or a boolean (where a > boolean generally means use a default number). Excellent point. The rephrasing done by the patch as-is will be a regression. > It would be a more > invasive change but I wonder if it would be better for > git_config_bool() to have it's own error message rather sharing it > with git_config_bool_or_int(). You'd probably need a variant of die_bad_numer(), with an untested patch like below. config.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/config.c b/config.c index 8db9c77098..801d944d2b 100644 --- a/config.c +++ b/config.c @@ -1097,7 +1097,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) return v; } *is_bool = 0; - return git_config_int(name, value); + + if (!git_parse_int(value, &v)) + die_bad_number_or_bool(name, value); + return v; } int git_config_bool(const char *name, const char *value) ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/2] formatting for error messages 2020-09-18 2:17 [PATCH 0/2] config: improve error message for boolean config Andrew Klotz via GitGitGadget 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget @ 2020-09-18 2:17 ` Andrew Klotz via GitGitGadget 2020-09-18 17:22 ` [PATCH 0/2] config: improve error message for boolean config Junio C Hamano 2021-02-09 1:25 ` [PATCH v2] " Andrew Klotz via GitGitGadget 3 siblings, 0 replies; 13+ messages in thread From: Andrew Klotz via GitGitGadget @ 2020-09-18 2:17 UTC (permalink / raw) To: git; +Cc: Andrew Klotz, Andrew Klotz From: Andrew Klotz <agc.klotz@gmail.com> we no longer need to use `N_` instead of `_` here. we had been using `N_` instead of `_` in the case this function was being called by with `GIT_TEST_GETTEXT_POISON` leading to an infinite loop. Since we have moved `GIT_TEST_GETTEXT_POISON` out of this function, (it is evaluated as a boolean, not an int) we no longer need to handle the special case and can use `_`. Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> --- config.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index 198d0d3216..624fb30fdd 100644 --- a/config.c +++ b/config.c @@ -990,8 +990,8 @@ NORETURN static void die_bad_number(const char *name, const char *value) { const char *error_type = (errno == ERANGE) ? - N_("out of range") : N_("invalid unit"); - const char *bad_numeric = N_("bad numeric config value '%s' for '%s': %s"); + _("out of range") : _("invalid unit"); + const char *bad_numeric = _("bad numeric config value '%s' for '%s': %s"); if (!value) value = ""; -- gitgitgadget ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 0/2] config: improve error message for boolean config 2020-09-18 2:17 [PATCH 0/2] config: improve error message for boolean config Andrew Klotz via GitGitGadget 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget 2020-09-18 2:17 ` [PATCH 2/2] formatting for error messages Andrew Klotz via GitGitGadget @ 2020-09-18 17:22 ` Junio C Hamano 2021-02-09 1:25 ` [PATCH v2] " Andrew Klotz via GitGitGadget 3 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2020-09-18 17:22 UTC (permalink / raw) To: Andrew Klotz via GitGitGadget; +Cc: git, Andrew Klotz "Andrew Klotz via GitGitGadget" <gitgitgadget@gmail.com> writes: > Currently invalid boolean config values return messages about 'bad numeric', > which I found misleading when the error was due to a boolean string value. > This change makes the error message reflect the boolean value. > > The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean > value, moving its special case out fromdie_bad_number() and into > git_config_bool_or_int(). The approach does not make anything worse than what we currently have, which is good. I am undecided if we want to apply 2/2, or if we want to apply 1/2 alone without 2/2. If we applied 2/2, those who are reading the code in a year who forgot about this review thread would have to wonder if all values assigned to the variable bad_numeric are enclosed in _() and go up to find all assignments. Omitting 2/2 would keep _() around the message string fed to die(), so it may be easier to immediately see that the call to die is not missing basic i18n, but there is a risk to forget marking with N_(). If we were to use 2/2 in addition to 1/2, then squashing them into one commit will make the result easier to follow, because we no longer need an untranslated string in bad_numeric after 1/2 is applied. We are losing "the reason why we use N_() is..." comment in 1/2 anyway so doing what 2/2 does in the same commit would be more sensible than splitting these into two patches. I dunno. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2] config: improve error message for boolean config 2020-09-18 2:17 [PATCH 0/2] config: improve error message for boolean config Andrew Klotz via GitGitGadget ` (2 preceding siblings ...) 2020-09-18 17:22 ` [PATCH 0/2] config: improve error message for boolean config Junio C Hamano @ 2021-02-09 1:25 ` Andrew Klotz via GitGitGadget 2021-02-09 16:51 ` Jeff King 2021-02-11 20:30 ` [PATCH v3] " Andrew Klotz via GitGitGadget 3 siblings, 2 replies; 13+ messages in thread From: Andrew Klotz via GitGitGadget @ 2021-02-09 1:25 UTC (permalink / raw) To: git; +Cc: Jeff King, Phillip Wood, Andrew Klotz, Andrew Klotz From: Andrew Klotz <agc.klotz@gmail.com> Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int. before with an invalid boolean value of `non-boolean`, its unclear what numeric is referring to: ``` fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit ``` now the error message mentions `non-boolean` is a bad boolean value: ``` fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' ``` Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> --- config: improve error message for boolean config Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value. The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out from die_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values. changes since v1 * moved boolean error message change out of git_config_bool_or_int to just in git_config_bool and added die_bad_boolean instead of modifying die_bad_number. Signed-off-by: Andrew Klotz agc.klotz@gmail.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v2 Pull-Request: https://github.com/git/git/pull/841 Range-diff vs v1: 1: 689d84672422 ! 1: 32dd4ee1e373 config: improve error message for boolean config @@ Commit message boolean error message when we know the value is neither a bool text or int. - `GIT_TEST_GETTEXT_POISON` is a boolean so we no longer fail on - evaluating it as an int in `git_config_int`. Because of that we can - move the special translation case into the boolean config check where - we are now failing with an updated message - before with an invalid boolean value of `non-boolean`, its unclear what numeric is referring to: ``` @@ Commit message ## config.c ## @@ config.c: static void die_bad_number(const char *name, const char *value) - if (!value) - value = ""; - -- if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) -- /* -- * We explicitly *don't* use _() here since it would -- * cause an infinite loop with _() needing to call -- * use_gettext_poison(). This is why marked up -- * translations with N_() above. -- */ -- die(bad_numeric, value, name, error_type); -- - if (!(cf && cf->name)) - die(_(bad_numeric), value, name, _(error_type)); - -@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *is_bool) - return v; } - *is_bool = 0; -- return git_config_int(name, value); -+ if (git_parse_int(value, &v)) -+ return v; -+ + } + ++NORETURN ++static void die_bad_bool(const char *name, const char *value) ++{ + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) + /* + * We explicitly *don't* use _() here since it would @@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *i + die("bad boolean config value '%s' for '%s'", value, name); + else + die(_("bad boolean config value '%s' for '%s'"), value, name); - } ++} ++ + int git_config_int(const char *name, const char *value) + { + int ret; +@@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *is_bool) int git_config_bool(const char *name, const char *value) + { +- int discard; +- return !!git_config_bool_or_int(name, value, &discard); ++ int v = git_parse_maybe_bool(value); ++ if (0 <= v) ++ return v; ++ else ++ die_bad_bool(name, value); + } + + int git_config_string(const char **dest, const char *var, const char *value) ## t/t0205-gettext-poison.sh ## @@ t/t0205-gettext-poison.sh: test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant 2: 1e9caf1911d3 < -: ------------ formatting for error messages config.c | 21 +++++++++++++++++++-- t/t0205-gettext-poison.sh | 2 +- 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/config.c b/config.c index 4c0cf3a1c15d..5e4fd6b5561b 100644 --- a/config.c +++ b/config.c @@ -1030,6 +1030,20 @@ static void die_bad_number(const char *name, const char *value) } } +NORETURN +static void die_bad_bool(const char *name, const char *value) +{ + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) + /* + * We explicitly *don't* use _() here since it would + * cause an infinite loop with _() needing to call + * use_gettext_poison(). + */ + die("bad boolean config value '%s' for '%s'", value, name); + else + die(_("bad boolean config value '%s' for '%s'"), value, name); +} + int git_config_int(const char *name, const char *value) { int ret; @@ -1102,8 +1116,11 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) int git_config_bool(const char *name, const char *value) { - int discard; - return !!git_config_bool_or_int(name, value, &discard); + int v = git_parse_maybe_bool(value); + if (0 <= v) + return v; + else + die_bad_bool(name, value); } int git_config_string(const char **dest, const char *var, const char *value) diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh index f9fa16ad8363..b66d34c6f2bc 100755 --- a/t/t0205-gettext-poison.sh +++ b/t/t0205-gettext-poison.sh @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error " test_done base-commit: 66e871b6647ffea61a77a0f82c7ef3415f1ee79c -- gitgitgadget ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2] config: improve error message for boolean config 2021-02-09 1:25 ` [PATCH v2] " Andrew Klotz via GitGitGadget @ 2021-02-09 16:51 ` Jeff King 2021-02-09 21:02 ` Junio C Hamano 2021-02-11 20:30 ` [PATCH v3] " Andrew Klotz via GitGitGadget 1 sibling, 1 reply; 13+ messages in thread From: Jeff King @ 2021-02-09 16:51 UTC (permalink / raw) To: Andrew Klotz via GitGitGadget; +Cc: git, Phillip Wood, Andrew Klotz On Tue, Feb 09, 2021 at 01:25:08AM +0000, Andrew Klotz via GitGitGadget wrote: > Currently invalid boolean config values return messages about 'bad > numeric', which is slightly misleading when the error was due to a > boolean value. We can improve the developer experience by returning a > boolean error message when we know the value is neither a bool text or > int. Thanks for keeping at this. The goal makes sense. The implementation looks OK to me, but I had a few really tiny comments. > before with an invalid boolean value of `non-boolean`, its unclear what > numeric is referring to: > ``` > fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit > ``` > > now the error message mentions `non-boolean` is a bad boolean value: > ``` > fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' > ``` Our commit messages aren't generally formatted as markdown. So this looks a little nicer using indentation (which also happens to generate nice markdown output): numeric is referring to: fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit now the error message mentions `non-boolean` is a bad boolean value: fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' Not worth a re-roll on its own, though. > +NORETURN > +static void die_bad_bool(const char *name, const char *value) > +{ > + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) > + /* > + * We explicitly *don't* use _() here since it would > + * cause an infinite loop with _() needing to call > + * use_gettext_poison(). > + */ > + die("bad boolean config value '%s' for '%s'", value, name); > + else > + die(_("bad boolean config value '%s' for '%s'"), value, name); > +} The duplication is ugly, but I think it's the least-bad solution (and I still dream that GETTEXT_POISON may one day go away :) ). > @@ -1102,8 +1116,11 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) > > int git_config_bool(const char *name, const char *value) > { > - int discard; > - return !!git_config_bool_or_int(name, value, &discard); > + int v = git_parse_maybe_bool(value); > + if (0 <= v) > + return v; > + else > + die_bad_bool(name, value); I had to look at this a minute to be sure we always returned a value. But the compiler knows that die_bad_bool() doesn't return, and that we always take one of the two conditionals. I do think it might be easier to read as: int v = git_parse_maybe_bool(value); if (v < 0) die_bad_bool(name, value); return v; but I admit that's nit-picking. > diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh > index f9fa16ad8363..b66d34c6f2bc 100755 > --- a/t/t0205-gettext-poison.sh > +++ b/t/t0205-gettext-poison.sh > @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant > > test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " > test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && > - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error > + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error > " Do we want a separate test in t1300 that doesn't rely on GETTEXT_POISON continuing to hang around (the idea has been thrown around elsewhere of it maybe going away entirely)? -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2] config: improve error message for boolean config 2021-02-09 16:51 ` Jeff King @ 2021-02-09 21:02 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-02-09 21:02 UTC (permalink / raw) To: Jeff King; +Cc: Andrew Klotz via GitGitGadget, git, Phillip Wood, Andrew Klotz Jeff King <peff@peff.net> writes: > On Tue, Feb 09, 2021 at 01:25:08AM +0000, Andrew Klotz via GitGitGadget wrote: > >> Currently invalid boolean config values return messages about 'bad >> numeric', which is slightly misleading when the error was due to a >> boolean value. We can improve the developer experience by returning a >> boolean error message when we know the value is neither a bool text or >> int. > > Thanks for keeping at this. The goal makes sense. The implementation > looks OK to me, but I had a few really tiny comments. > >> before with an invalid boolean value of `non-boolean`, its unclear what >> numeric is referring to: >> ``` >> fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit >> ``` >> >> now the error message mentions `non-boolean` is a bad boolean value: >> ``` >> fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' >> ``` > > Our commit messages aren't generally formatted as markdown. So this > looks a little nicer using indentation (which also happens to generate > nice markdown output): > > numeric is referring to: > > fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit > > now the error message mentions `non-boolean` is a bad boolean value: > > fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' > > Not worth a re-roll on its own, though. > >> +NORETURN >> +static void die_bad_bool(const char *name, const char *value) >> +{ >> + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) >> + /* >> + * We explicitly *don't* use _() here since it would >> + * cause an infinite loop with _() needing to call >> + * use_gettext_poison(). >> + */ >> + die("bad boolean config value '%s' for '%s'", value, name); >> + else >> + die(_("bad boolean config value '%s' for '%s'"), value, name); >> +} > > The duplication is ugly, but I think it's the least-bad solution (and I > still dream that GETTEXT_POISON may one day go away :) ). > >> @@ -1102,8 +1116,11 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) >> >> int git_config_bool(const char *name, const char *value) >> { >> - int discard; >> - return !!git_config_bool_or_int(name, value, &discard); >> + int v = git_parse_maybe_bool(value); >> + if (0 <= v) >> + return v; >> + else >> + die_bad_bool(name, value); > > I had to look at this a minute to be sure we always returned a value. > But the compiler knows that die_bad_bool() doesn't return, and that we > always take one of the two conditionals. > > I do think it might be easier to read as: > > int v = git_parse_maybe_bool(value); > if (v < 0) > die_bad_bool(name, value); > return v; > > but I admit that's nit-picking. Combined together with the log message formatting, they already make it worth an updated patch. >> diff --git a/t/t0205-gettext-poison.sh b/t/t0205-gettext-poison.sh >> index f9fa16ad8363..b66d34c6f2bc 100755 >> --- a/t/t0205-gettext-poison.sh >> +++ b/t/t0205-gettext-poison.sh >> @@ -33,7 +33,7 @@ test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant >> >> test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " >> test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && >> - grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error >> + grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error >> " > > Do we want a separate test in t1300 that doesn't rely on GETTEXT_POISON > continuing to hang around (the idea has been thrown around elsewhere of > it maybe going away entirely)? Yeah, I do not think it is a good idea to add more test that depends on GETTEXT_POISON and assumes how it works. We certainly wish that we want to ensure more quality translation and more complete i18n coverage, but the "pretend as if the test passed if it tries to judge the success/failure by looking at the output string from the command that can be translated, to ferret out mistakenly marked machine readable string for translation", which is what the current GETTEXT_POISON thing is, does not contribute much to improving translation. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v3] config: improve error message for boolean config 2021-02-09 1:25 ` [PATCH v2] " Andrew Klotz via GitGitGadget 2021-02-09 16:51 ` Jeff King @ 2021-02-11 20:30 ` Andrew Klotz via GitGitGadget 2021-02-12 10:38 ` Jeff King 1 sibling, 1 reply; 13+ messages in thread From: Andrew Klotz via GitGitGadget @ 2021-02-11 20:30 UTC (permalink / raw) To: git; +Cc: Jeff King, Phillip Wood, Andrew Klotz, Andrew Klotz From: Andrew Klotz <agc.klotz@gmail.com> Currently invalid boolean config values return messages about 'bad numeric', which is slightly misleading when the error was due to a boolean value. We can improve the developer experience by returning a boolean error message when we know the value is neither a bool text or int. before with an invalid boolean value of `non-boolean`, its unclear what numeric is referring to: fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit now the error message mentions `non-boolean` is a bad boolean value: fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> --- config: improve error message for boolean config Currently invalid boolean config values return messages about 'bad numeric', which I found misleading when the error was due to a boolean string value. This change makes the error message reflect the boolean value. The current approach relies on GIT_TEST_GETTEXT_POISON being a boolean value, moving its special case out from die_bad_number() and into git_config_bool_or_int(). An alternative could be for die_bad_number() to handle boolean values when erroring, although the function name might need to change if it is handling non-numeric values. changes since v1 * moved boolean error message change out of git_config_bool_or_int to just in git_config_bool and added die_bad_boolean instead of modifying die_bad_number. changes since v2 * added a test for boolean config values * changed the condition to hit die_bad_bool from if (0 <= v) to if (v < 0) * removed change in get-text-poison.sh test Signed-off-by: Andrew Klotz agc.klotz@gmail.com Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-841%2FKlotzAndrew%2Fbetter_bool_errors-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-841/KlotzAndrew/better_bool_errors-v3 Pull-Request: https://github.com/git/git/pull/841 Range-diff vs v2: 1: 32dd4ee1e373 ! 1: 45a51e18c1d3 config: improve error message for boolean config @@ Commit message before with an invalid boolean value of `non-boolean`, its unclear what numeric is referring to: - ``` - fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit - ``` + fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit now the error message mentions `non-boolean` is a bad boolean value: - ``` - fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' - ``` + fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> @@ config.c: int git_config_bool_or_int(const char *name, const char *value, int *i - int discard; - return !!git_config_bool_or_int(name, value, &discard); + int v = git_parse_maybe_bool(value); -+ if (0 <= v) -+ return v; -+ else ++ if (v < 0) + die_bad_bool(name, value); ++ return v; } int git_config_string(const char **dest, const char *var, const char *value) - ## t/t0205-gettext-poison.sh ## -@@ t/t0205-gettext-poison.sh: test_expect_success 'eval_gettext: our eval_gettext() fallback has poison semant - - test_expect_success "gettext: invalid GIT_TEST_GETTEXT_POISON value doesn't infinitely loop" " - test_must_fail env GIT_TEST_GETTEXT_POISON=xyz git version 2>error && -- grep \"fatal: bad numeric config value 'xyz' for 'GIT_TEST_GETTEXT_POISON': invalid unit\" error -+ grep \"fatal: bad boolean config value 'xyz' for 'GIT_TEST_GETTEXT_POISON'\" error - " + ## t/t1300-config.sh ## +@@ t/t1300-config.sh: test_expect_success 'invalid unit' ' + test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual + ' - test_done ++test_expect_success 'invalid unit boolean' ' ++ git config commit.gpgsign "1true" && ++ test_cmp_config 1true commit.gpgsign && ++ test_must_fail git config --bool --get commit.gpgsign 2>actual && ++ test_i18ngrep "bad boolean config value .1true. for .commit.gpgsign." actual ++' ++ + test_expect_success 'line number is reported correctly' ' + printf "[bool]\n\tvar\n" >invalid && + test_must_fail git config -f invalid --path bool.var 2>actual && config.c | 20 ++++++++++++++++++-- t/t1300-config.sh | 7 +++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/config.c b/config.c index b922b4f28572..f90b633dba21 100644 --- a/config.c +++ b/config.c @@ -1180,6 +1180,20 @@ static void die_bad_number(const char *name, const char *value) } } +NORETURN +static void die_bad_bool(const char *name, const char *value) +{ + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) + /* + * We explicitly *don't* use _() here since it would + * cause an infinite loop with _() needing to call + * use_gettext_poison(). + */ + die("bad boolean config value '%s' for '%s'", value, name); + else + die(_("bad boolean config value '%s' for '%s'"), value, name); +} + int git_config_int(const char *name, const char *value) { int ret; @@ -1252,8 +1266,10 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool) int git_config_bool(const char *name, const char *value) { - int discard; - return !!git_config_bool_or_int(name, value, &discard); + int v = git_parse_maybe_bool(value); + if (v < 0) + die_bad_bool(name, value); + return v; } int git_config_string(const char **dest, const char *var, const char *value) diff --git a/t/t1300-config.sh b/t/t1300-config.sh index 936d72041bab..e0dd5d65ceda 100755 --- a/t/t1300-config.sh +++ b/t/t1300-config.sh @@ -675,6 +675,13 @@ test_expect_success 'invalid unit' ' test_i18ngrep "bad numeric config value .1auto. for .aninvalid.unit. in file .git/config: invalid unit" actual ' +test_expect_success 'invalid unit boolean' ' + git config commit.gpgsign "1true" && + test_cmp_config 1true commit.gpgsign && + test_must_fail git config --bool --get commit.gpgsign 2>actual && + test_i18ngrep "bad boolean config value .1true. for .commit.gpgsign." actual +' + test_expect_success 'line number is reported correctly' ' printf "[bool]\n\tvar\n" >invalid && test_must_fail git config -f invalid --path bool.var 2>actual && base-commit: c6102b758572c7515f606b2423dfe38934fe6764 -- gitgitgadget ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3] config: improve error message for boolean config 2021-02-11 20:30 ` [PATCH v3] " Andrew Klotz via GitGitGadget @ 2021-02-12 10:38 ` Jeff King 2021-02-12 19:52 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2021-02-12 10:38 UTC (permalink / raw) To: Andrew Klotz via GitGitGadget; +Cc: git, Phillip Wood, Andrew Klotz On Thu, Feb 11, 2021 at 08:30:53PM +0000, Andrew Klotz via GitGitGadget wrote: > From: Andrew Klotz <agc.klotz@gmail.com> > > Currently invalid boolean config values return messages about 'bad > numeric', which is slightly misleading when the error was due to a > boolean value. We can improve the developer experience by returning a > boolean error message when we know the value is neither a bool text or > int. > > before with an invalid boolean value of `non-boolean`, its unclear what > numeric is referring to: > fatal: bad numeric config value 'non-boolean' for 'commit.gpgsign': invalid unit > > now the error message mentions `non-boolean` is a bad boolean value: > fatal: bad boolean config value 'non-boolean' for 'commit.gpgsign' > > Signed-off-by: Andrew Klotz <agc.klotz@gmail.com> Thanks, all the changes here look good to me except for one curiosity: > diff --git a/config.c b/config.c > index b922b4f28572..f90b633dba21 100644 > --- a/config.c > +++ b/config.c > @@ -1180,6 +1180,20 @@ static void die_bad_number(const char *name, const char *value) > } > } > > +NORETURN > +static void die_bad_bool(const char *name, const char *value) > +{ > + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) > + /* > + * We explicitly *don't* use _() here since it would > + * cause an infinite loop with _() needing to call > + * use_gettext_poison(). > + */ > + die("bad boolean config value '%s' for '%s'", value, name); > + else > + die(_("bad boolean config value '%s' for '%s'"), value, name); > +} ...if this is rebased on the current master that does not have GIT_TEST_GETTEXT_POISON anymore, then I think this whole function can be simplified down to the final line. Since it looks like Junio applied this on top of v2.30.1, which still has GETTEXT_POISON, we need it here. But we should remember to remove it with the rest of the GETTEXT_POISON once it's all merged together. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3] config: improve error message for boolean config 2021-02-12 10:38 ` Jeff King @ 2021-02-12 19:52 ` Junio C Hamano 0 siblings, 0 replies; 13+ messages in thread From: Junio C Hamano @ 2021-02-12 19:52 UTC (permalink / raw) To: Jeff King, Ævar Arnfjörð Bjarmason Cc: Andrew Klotz via GitGitGadget, git, Phillip Wood, Andrew Klotz Jeff King <peff@peff.net> writes: > Thanks, all the changes here look good to me except for one curiosity: > >> diff --git a/config.c b/config.c >> index b922b4f28572..f90b633dba21 100644 >> --- a/config.c >> +++ b/config.c >> @@ -1180,6 +1180,20 @@ static void die_bad_number(const char *name, const char *value) >> } >> } >> >> +NORETURN >> +static void die_bad_bool(const char *name, const char *value) >> +{ >> + if (!strcmp(name, "GIT_TEST_GETTEXT_POISON")) >> + /* >> + * We explicitly *don't* use _() here since it would >> + * cause an infinite loop with _() needing to call >> + * use_gettext_poison(). >> + */ >> + die("bad boolean config value '%s' for '%s'", value, name); >> + else >> + die(_("bad boolean config value '%s' for '%s'"), value, name); >> +} > > ...if this is rebased on the current master that does not have > GIT_TEST_GETTEXT_POISON anymore, then I think this whole function can be > simplified down to the final line. > > Since it looks like Junio applied this on top of v2.30.1, which still > has GETTEXT_POISON, we need it here. But we should remember to remove it > with the rest of the GETTEXT_POISON once it's all merged together. Thanks for being extra careful. I sort-of considered that this was a bugfix and that was why it is made mergeable down to the maint track, but I do not mind making this one of the "incentives" to upgrade for our users ;-) I actually suspect that without any of us worrying about it, Ævar will take care of the cleaning up when the dust settles. Thanks. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-12 19:56 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-18 2:17 [PATCH 0/2] config: improve error message for boolean config Andrew Klotz via GitGitGadget 2020-09-18 2:17 ` [PATCH 1/2] " Andrew Klotz via GitGitGadget 2020-09-18 18:10 ` Jeff King 2020-09-18 18:30 ` Phillip Wood 2020-09-18 19:14 ` Junio C Hamano 2020-09-18 2:17 ` [PATCH 2/2] formatting for error messages Andrew Klotz via GitGitGadget 2020-09-18 17:22 ` [PATCH 0/2] config: improve error message for boolean config Junio C Hamano 2021-02-09 1:25 ` [PATCH v2] " Andrew Klotz via GitGitGadget 2021-02-09 16:51 ` Jeff King 2021-02-09 21:02 ` Junio C Hamano 2021-02-11 20:30 ` [PATCH v3] " Andrew Klotz via GitGitGadget 2021-02-12 10:38 ` Jeff King 2021-02-12 19:52 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).