git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Andrew Klotz via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Phillip Wood <phillip.wood123@gmail.com>,
	Andrew Klotz <agc.klotz@gmail.com>
Subject: Re: [PATCH v2] config: improve error message for boolean config
Date: Tue, 09 Feb 2021 13:02:18 -0800	[thread overview]
Message-ID: <xmqqczx90xb9.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <YCK9iRyQGTl+5J4g@coredump.intra.peff.net> (Jeff King's message of "Tue, 9 Feb 2021 11:51:21 -0500")

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.

  reply	other threads:[~2021-02-09 21:44 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqczx90xb9.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=agc.klotz@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood123@gmail.com \
    --subject='Re: [PATCH v2] config: improve error message for boolean config' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this 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).