On Wed, Nov 17, 2010 at 11:36:30AM -0800, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html