git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, "Lin Sun" <lin.sun@zoom.us>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"David Aguilar" <davvid@gmail.com>
Subject: Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true
Date: Thu, 08 Apr 2021 11:10:50 -0700	[thread overview]
Message-ID: <xmqqmtu8tzo5.fsf@gitster.g> (raw)
In-Reply-To: <patch-1.6-bd70181ffb-20210408T133125Z-avarab@gmail.com> (=?utf-8?B?IsOGdmFyCUFybmZqw7Zyw7A=?= Bjarmason"'s message of "Thu, 8 Apr 2021 15:34:25 +0200")

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

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

  reply	other threads:[~2021-04-08 18:10 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-08 13:34 [PATCH 0/5] config: support --type=bool-or-auto for "tristate" parsing Ævar Arnfjörð Bjarmason
2021-04-08 13:34 ` [PATCH 1/5] config.c: add a comment about why value=NULL is true Ævar Arnfjörð Bjarmason
2021-04-08 18:10   ` Junio C Hamano [this message]
2021-04-08 13:34 ` [PATCH 2/5] config tests: test for --bool-or-str Ævar Arnfjörð Bjarmason
2021-04-08 18:21   ` Junio C Hamano
2021-04-08 23:11     ` Ævar Arnfjörð Bjarmason
2021-04-08 13:34 ` [PATCH 3/5] git-config: document --bool-or-str and --type=bool-or-str Ævar Arnfjörð Bjarmason
2021-04-08 18:22   ` Junio C Hamano
2021-04-08 13:34 ` [PATCH 4/5] config.c: add a "tristate" helper Ævar Arnfjörð Bjarmason
2021-04-08 18:33   ` Junio C Hamano
2021-04-08 23:23     ` Ævar Arnfjörð Bjarmason
2021-04-08 23:51       ` Junio C Hamano
2021-04-09  1:33         ` Ævar Arnfjörð Bjarmason
2021-04-09 12:53           ` Junio C Hamano
2021-04-08 23:54       ` Junio C Hamano
2021-04-09 20:05     ` Jeff King
2021-04-09 22:11       ` Junio C Hamano
2021-04-10  1:23         ` Jeff King
2021-04-10  1:43           ` Junio C Hamano
2021-04-08 13:34 ` [PATCH 5/5] config: add --type=bool-or-auto switch Ævar Arnfjörð Bjarmason
2021-04-08 18:36   ` 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=xmqqmtu8tzo5.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=davvid@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=lin.sun@zoom.us \
    --subject='Re: [PATCH 1/5] config.c: add a comment about why value=NULL is true' \
    /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).