Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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;
