Re: [PATCH 1/2] log.decorate: accept 0/1 bool values

[Jeff King <peff@peff.net>]

On Wed, Nov 17, 2010 at 11:36:30AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > We cannot simply add 0/1 support to git_config_maybe_bool.
> > That would confuse git_config_bool_or_int, which may want to
> > distinguish the integer values "0" and "1" from bools.
> 
> ... e.g. number one spelled as "1" do mean one not true in contexts like
> status.submodulesummary, where "true" means "unlimited" and "1" means
> "limit to one".  It surely is necessary to avoid breakage there.

Makes sense. I didn't even bother checking, as the fact that we export
it as "git config --bool-or-int" was enough for me that we need to keep
the behavior the same.

> A caller that uses git_config_maybe_bool() expects to see 0/1 when the
> input looks like a boolean, and your patch looks like the right thing to
> do.

It's possible we could have another type of caller, but I suspect they
would want git_config_bool_or_int instead. The exception would be one who
wants a bool, an int, _or_ an arbitrary string. If somebody writes such
a thing, they are free to expose the new git_config_maybe_bool_text.

> The repertoire of parsers that involve elements that are possibly boolean
> are now:
> 
>  - git_config_bool(): takes "0/false/no/..." or "$n/true/yes/..." (where
>    any non-zero number $n is taken as true [*1*]), or more traditional
> 
>    [section]
>        var
> 
>    without any equal sign, which means true.  Errors out if the input is
>    not a boolean.
> 
>  - git_config_maybe_bool(): similar, and returns -1 (not a bool), 0
>    (false) and 1 (true).  "0" is false, "1" is true.  But all other
>    numbers are not boolean;

Yeah, I did notice that. Technically it is a regression in my 2/2, in
that "pager.foo = 2" used to mean "use the pager for foo" and now means
"use a pager called 2". But I am willing to write that off as insanity,
especially since we never documented that behavior (and in fact we
explicitly document the allowable values as yes/no, 0/1, true/false, and
on/off).

I don't think it is worth closing the hole for no reason on other config
options, but I am certainly fine with breaking it in the case of
pager.*.

> Perhaps we would want to add Documentation/technical/api-config.txt
> someday?   Hint, hint....

I'll put it on my todo, right after refactoring the awful mess of the
config code itself. :)

-Peff