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

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Feb 10, 2008, 23:50 UTC
Message-ID: <alpine.LFD.1.00.0802101541380.2920@woody.linux-foundation.org>
In-Reply-To: <20080210232920.GH5129@artemis.madism.org>

On Mon, 11 Feb 2008, Pierre Habouzit wrote:

Show 9 quoted lines

>
> > Having said all that, it might be an option to change your patch
> > slightly to say:
> > 
> > 	const char config_true[] = "true";
> 
> I was about to suggest the same, and testing against "config_true" just
> becomes an optimization, but isn't required. Seems an appropriate hack
> to me.

Well, I had the thing actually written that way, but it breaks some of the test-suite. Whether the test-suite actually *should* test for what it tests for is obviously debatable, but it does. It does test that when you do

	git config --get novalue.variable

you are expected to get an empty result. Is that good? Probably not. But it's what you get traditionally, and it's what the tests actually test for (t/t1300-repo-config.sh in case you care).

But yes, I actually think it might be an improvement and have that thing return "true" (which is what happens if you make the 'config_true' array contain that string). And that allows removal of one test from the "git_config_bool()" function, but on the other hand, it does result in some strange stuff too..

In particular, *if* somebody just takes a string blindly and uses

	result = xstrdup(value);

then with my patch it would then use an empty string for whatever it happened to pick. So having something like

	[user]
		name

will mean that your name is empty (which will actually trigger an error if you try to commit).

In contrast, if you do that 'config_true[] = "true"' thing, then that config file entry will make your name be the string "true", which is just _odd_.

So I think an empty config_true actually has a nicer and more easily explainable failure case. It causes empty strings when mis-used, not arbitrary and strange "true" strings.

Whatever.

		Linus