Re: [PATCH 0/2] config: improve error message for boolean config

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 18, 2020, 17:22 UTC
Message-ID: <xmqqmu1nm23r.fsf@gitster.c.googlers.com>
In-Reply-To: <pull.841.git.git.1600395427.gitgitgadget@gmail.com>

"Andrew Klotz via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

> 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.