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

So I realize that people have worked on checking for NULL, but I'm kind of of the opinion that we could just decide to say that "value" just should never be NULL at all, and marking the special case "true" value differently.

Namely by just using a _special_ empty string that can be distinguished from any regular user-generated empty strings.

This fairly simple patch just uses a special static "config_true" array variable to specify that special case that we used to use NULL for, and then replaces the few explicit places where we knew that NULL was special in the config handling with testing whether the value is that special array.

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.

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.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

Hmm?

 builtin-config.c |   10 +++++-----
 cache.h          |    1 +
 config.c         |    7 ++++---
 3 files changed, 10 insertions(+), 8 deletions(-)

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

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.

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

On Sun, 10 Feb 2008, Junio C Hamano wrote:

No it does not.

And that was done by my patch. 

And this is different from checking against NULL exactly how?

No, it was what the regex parsing logic was - empty matches empty. Whether we really want it or not, I dunno.

		Linus

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

Exactly. My answer to your question is: "It is not different from checking against NULL at all."

The first one (i.e. you needed to do so in your patch) shows that the codepath that is already doing the right thing needs to be modified. If we do not introduce config_true, we do not even have to. You need additional change to correctly functioning codepaths that you should not have to do.

The second one shows that even if we introduce config_true, such an already broken codepath needs to be fixed to check with either NULL or config_true anyway. The need for fix the codepath has not been reduced. Changing the rule does not help for this class of codepath.

But as you seem to imply, it might make sense to equate

	[some-random-section]
        	some-random-variable

to

	[some-random-section]
        	some-random-variable = ""

for variables that cannot possibly have any meaningful "bool" semantics. This third class of variables is a possible benefit your patch brings in. The code can be lax for these variables.

However, it would make things inconsistent ("this variable is bool and the above two forms mean completely opposite things, while that variable is not bool and they mean the same thing"). I am just having a hard time convincing myself that this little detail does not matter.

Junio C Hamano <gitster@pobox.com> writes:

Having said all that, it might be an option to change your patch slightly to say:

	const char config_true[] = "true";

IOW, make

	[section]
        	var

equivalent to

	[section]
		var = "true"

Existing codepaths that deal with variables that were originally bool but now bool + more might get upset if we did so, though. An example I could think of offhand was "d7f4633 (Make AutoCRLF ternary variable" but that one is safe.

On Sun, Feb 10, 2008 at 10:47:02PM +0000, Junio C Hamano wrote:

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.

On Mon, 11 Feb 2008, Pierre Habouzit wrote:

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

On Sun, 10 Feb 2008, Junio C Hamano wrote:

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.

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

Yes, the 10% that actually *cares* will look exactly the same (the difference being comparing against NULL or config_true), but all the stuff that compares against a special string can just ignore the fact that the special truth value will never ever match that string.

		Linus

On Sun, 10 Feb 2008, Linus Torvalds wrote:

And here's an example of this kind of effect. I'm not actually suggesting you apply this patch, but tell me it isn't simpler done this way?

So this is where it *does* make a difference whether we use NULL or config_bool, and where config_bool is simply better: it allows a config routine to simply never care..

		Linus

---
 color.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

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

Yes, that is a good example of simplification.

But that applies only to "originally bool but now has additional states" kind of variables.

For a variable that is never about boolean, if the original code said:

	if (!strcmp(var, "section.variable"))
        	foo = xstrdup(value);

it is wrong (would strdup NULL), and the correct fix would be:

	if (!strcmp(var, "section.variable")) {
		if (!value)
			die("missing value for '%s'", var);
        	foo = xstrdup(value);
	}

It does not make much of a difference if that "if (!value)" becomes "if (value == config_true)". If you omit that check, as your "user.name" example shows, foo may get an empty string or a string "true", neither of which is what the user intended to say.

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

I agree.

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).

Christian Couder <chriscool@tuxfamily.org> writes:

Some variables started their life as boolean and "[foo] var" is true and "[foo] var = false" (or "= 0") is false for them; later they acquired third or more options to have string values.

For such a variable:

	[foo] var	    ; boolean "true"
        [foo] var = "true"  ; boolean "true"
        [foo] var = "false" ; boolean "false"
	[foo] var = ""	    ; boolean "false", too.
        [foo] var =         ; perhaps misspelled, perhaps the same as above.
	[foo] var = "value" ; string (new semantics)

and adding a warning for the fifth one (but not fourth one) might be a good idea.

I have a bit stronger opinion on keeping "[foo] var" (without an equal sign) to stay "true". Especially, making it exactly the same as an empty string is unacceptable, if it is done without Linus's trick to make it distinguishable internally.

Luckily, the calls to get_config_set() we internally make in-tree (e.g. git-init sets core.filemode and friends) are all very explicit and do not write "[foo] var" form of truth. But many existing docs written outside our (my) control that google finds say "Put '[core] autocrlf' in your .git/config". You do not want to break the repositories configured following such an advice.

Don't do it if you cannot differentiate between the fourth and the fifth in the above listing.

On Sun, 10 Feb 2008, Junio C Hamano wrote:

Shouldn't it be simply:

	if (!strcmp(value, "something special"))
		Ok, this is not bool but special;
	else
		return git_config_bool(var, value);

That is, don't check for special true or special false at all, but have git_config_bool() sort them out? And Linus's code means that you can do the strcmp without worrying about getting a segfault on special true.

If it isn't changed, you'd get the message "oops we do not understand ''" for either true or false empty values, which seems, if anything, better.

	-Daniel
*This .sig left intentionally blank*