Re: Alternative approach to the git config NULL value checking patches..

From: Christian Couder <chriscool@tuxfamily.org>
Date: Feb 11, 2008, 07:22 UTC
Message-ID: <200802110822.08902.chriscool@tuxfamily.org>
In-Reply-To: <alpine.LFD.1.00.0802101532070.2920@woody.linux-foundation.org>

Le lundi 11 février 2008, Linus Torvalds a écrit :

Show 14 quoted lines

> On Sun, 10 Feb 2008, Junio C Hamano wrote:
> >
> > My answer to your question is: "It is not different
> > from checking against NULL at all."
>
> Of course it is.
>
> Not using NULL means that things like
>
> 	strcmp()
> 	atoi()
> 	..
>
> all work and automatically get the right answer and don't need to care.

I agree.

> Take a look at the NULL-compare patches. 90% of them are just noise.

That's right.

Now the 10% are only when we have a boolean variable and we want it to be "false" when there is:

[foo]
	var =

while:

[foo]
	var

is considered "true".

And let's face it, it's not obvious at all why it should be false if there is "var =" and true when there is only "var". Is it a Microsoft standard ? Do we really care about it ?

I also doubt that many users willingly use "var =" to mean "false". In my opinion it is much more likely (95% against 5%) to be a typo than someone so lazy as to prefer only "var =" over "var = false".

So let's do them (and ourself too) a favor and deprecate "var =" to mean false. I will post my patch to do this.

By the way deprecating does not mean breaking it and not fixing where it breaks. It just means we think it's bad and it should not be used. We can even decide to keep the same boolean meaning (that is "false") for "var =" latter if we have a good way to deal with the cruft and if we really don't want to break things.

And even if we latter change "var =" to mean "true", we can still keep a warning by checking using Linus' trick:

int git_config_bool(const char *name, const char *value)
{
        if (value == config_true)
                return 1;
        if (!*value) {
                fprintf(stderr,
                        "Warning: using an empty value for boolean config "
                        "variables is deprecated and dangerous.\n"
                        "An empty value now means 'true' as a "
                        "boolean, but meant 'false' in previous git "
                        "versions!\n"
                        "Please consider using a 'true' (or 'false') value "
                        "explicitely for variable '%s', so that your "
                        "config is git version independant. "
                        "You can do that using for example:\n"
                        "\tgit config %s true\n", name, name);
               return 1;
       }
       if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
               return 1;
       if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))
		return 0;
	return git_config_int(name, value) != 0;
}

Or we could even (using Linus' trick) make it an error and just "die" in this case.

Christian (now urgently looking for a flame proof suit).