Re: [PATCH v2] config: improve error message for boolean config

[Junio C Hamano <gitster@pobox.com>]

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.