Re: [PATCH v4 3/4] config: correct bad boolean env value error message

From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, Calvin Wan <calvinwan@google.com>,
	phillip.wood123@gmail.com
Subject: Re: [PATCH v4 3/4] config: correct bad boolean env value error message
Date: Fri, 29 Sep 2023 16:03:44 -0700	[thread overview]
Message-ID: <xmqqa5t4inu7.fsf@gitster.g> (raw)
In-Reply-To: <e4c20a81f93e2a4e2ad65920920ead690b162e25.1696021277.git.jonathantanmy@google.com> (Jonathan Tan's message of "Fri, 29 Sep 2023 14:20:50 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

> From: Calvin Wan <calvinwan@google.com>
>
> An incorrectly defined boolean environment value would result in the
> following error message:
>
> bad boolean config value '%s' for '%s'
>
> This is a misnomer since environment value != config value. Instead of
> calling git_config_bool() to parse the environment value, mimic the
> functionality inside of git_config_bool() but with the correct error
> message.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  config.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)

Makes sense.

I briefly wondered if there are ways to share more code, but this
seems to be the best we can do.  The duplication is not too bad to
begin with anyway.

Looking good.  Will queue.

> diff --git a/config.c b/config.c
> index 3846a37be9..7dde0aaa02 100644
> --- a/config.c
> +++ b/config.c
> @@ -2133,7 +2133,14 @@ void git_global_config(char **user_out, char **xdg_out)
>  int git_env_bool(const char *k, int def)
>  {
>  	const char *v = getenv(k);
> -	return v ? git_config_bool(k, v) : def;
> +	int val;
> +	if (!v)
> +		return def;
> +	val = git_parse_maybe_bool(v);
> +	if (val < 0)
> +		die(_("bad boolean environment value '%s' for '%s'"),
> +		    v, k);
> +	return val;
>  }
>  
>  /*