* Re: [PATCH 1/2] config: improve error message for boolean config @ 2021-01-23 23:15 Andrew Klotz 0 siblings, 0 replies; 5+ messages in thread From: Andrew Klotz @ 2021-01-23 23:15 UTC (permalink / raw) To: gitster; +Cc: phillip.wood123, peff, gitgitgadget, git, agc.klotz Thanks for the feedback, I updated the branch and moved: - boolean error message change out of git_config_bool_or_int to just in git_config_bool - added die_bad_boolean instead of modifying die_bad_number. The advantage here is we only use a bool error message when we know the value is a bool. Is this more along the lines of the suggestion? ^ permalink raw reply [flat|nested] 5+ messages in thread
* [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 0 siblings, 1 reply; 5+ 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] 5+ messages in thread
* [PATCH 1/2] config: improve error message for boolean config 2020-09-18 2:17 [PATCH 0/2] " 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 0 siblings, 2 replies; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ 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; 5+ 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] 5+ messages in thread
end of thread, other threads:[~2021-01-23 23:18 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-01-23 23:15 [PATCH 1/2] config: improve error message for boolean config Andrew Klotz -- strict thread matches above, loose matches on Subject: below -- 2020-09-18 2:17 [PATCH 0/2] " 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
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).