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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2008, 21:25 UTC
Message-ID: <7vir0wfqrz.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <alpine.LFD.1.00.0802101225110.2896@woody.linux-foundation.org>

Linus Torvalds <torvalds@linux-foundation.org> writes:

Show 5 quoted lines

> It may be a bit odd, but it automatically means that codepaths that simply 
> don't want to care about the subtle difference between "true" and "empty" 
> will just automatically work, because to them the two cases will look 
> identical, because the strings will compare the same - they have the same 
> data, just different addresses.

I should have mentioned the reason why I did not suggest doing it this way in my [Janitor] message.

It is not "suttle difference between true and empty". Empty means false, and with this approach, it switches the meaning of valueless form of config to quite the opposite.

In addition to fixing existing breakages, a piece of code that knew NULL is true and empty is false and coded accordingly, i.e.

	if (!value)
        	Ah we have true;
	else if (!*value)
        	Ok this is false;
	else if (!strcmp(value, "something special")
        	Ok, this is not bool but special;
	else
        	return git_config_bool(var, value);

will now need to be changed to:

	if (value == config_true)
        	Ah we have true;
	else if (!*value)
        	Ok this is false;
	else if (!strcmp(value, "something special")
        	Ok, this is not bool but special;
	else
        	return git_config_bool(var, value);

if you do this.  And the code that was already broken:

	if (!strcmp(value, "somevalue")
		Ok let's use somevalue;
	else if (!strcmp(value, "someothervalue")
		Ok let's use someothervalue;
	else
        	die("oops we do not understand '%s'", value);

still need to be fixed to:

	if (value == config_true)
        	die("oops '%s' is not a bool", var);
	else if (!strcmp(value, "somevalue")
		Ok let's use somevalue;
	else if (!strcmp(value, "someothervalue")
		Ok let's use someothervalue;
	else
        	die("oops we do not understand '%s'", value);

So it does not buy us anything. That is why I did not suggest doing it that way.

Show 6 quoted lines

> This also means that code (notably the value regexp parsing) that does 
> something like
>
> 	value ? value : ""
>
> just automatically can go away, and just use 'value' directly.

Yes, but that's broken already, isn't it?