| Message ID | pull.841.v2.git.git.1612833909210.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] config: improve error message for boolean config | expand |
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
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.
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