Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 8, 2021, 18:10 UTC
Message-ID: <xmqqmtu8tzo5.fsf@gitster.g>
In-Reply-To: <patch-1.6-bd70181ffb-20210408T133125Z-avarab@gmail.com>

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Subject: Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true

A few unproductive nitpicks.

The question "why does '[core] ignorecase' mean core.ignorecase is true?" has only a single answer, which is "because that is how we designed (loosely imitating .ini format) and implemented."

> It's not very intuitive that git_parse_maybe_bool_text() would
> consider NULL to be a true value. Add a small comment about it.

Do you mean

	if (!value)
		return 1;

is unintuitive?  Or do you mean

	[core]
		ignorecase

being a valid way to say "the filesystem ignores case differences"?

The answer to the question is crucial to justify the title. Only when "the config syntax is perfectly understandable, the way the logic to parse that syntax is expressed in C is not necessarily intuitive" is true, the comment makes sense, I would think.

Show 21 quoted lines

> See a789ca70e7 (config: teach "git -c" to recognize an empty string,
> 2014-08-04) for the behavior of "git -c", but we'll end up here in
> both the config file parsing and command-line parsing cases.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  config.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/config.c b/config.c
> index 6428393a41..fc28dbd97c 100644
> --- a/config.c
> +++ b/config.c
> @@ -1229,6 +1229,10 @@ ssize_t git_config_ssize_t(const char *name, const char *value)
>  static int git_parse_maybe_bool_text(const char *value)
>  {
>  	if (!value)
> +		/*
> +		 * "[foo]\nbar\n" and "-c foo.bar" on the command-line
> +		 * are true.
> +		 */

This is a "we do not explain why '[foo] bar' is true, but to be prepared to parse it, we check if value is NULL" comment. And I think it is a good one, except that I do not see the point of writing "\n" at all. Replacing them with spaces would make it far easier to read.

		/*
		 * "[foo] bar" in the configuration file, and
		 * "git -c foo.bar" on the command-line, mean
		 * that the variable foo.bar is true.  A NULL is
		 * passed as 'value' in these cases.
		 */

>  		return 1;
>  	if (!*value)
>  		return 0;