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

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

[Linus Torvalds]

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

diff --git a/builtin-config.c b/builtin-config.c
index e4a12e3..a58f2a1 100644
--- a/builtin-config.c
+++ b/builtin-config.c
@@ -20,7 +20,7 @@ static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;
 
 static int show_all_config(const char *key_, const char *value_)
 {
-	if (value_)
+	if (value_ != config_true)
 		printf("%s%c%s%c", key_, delim, value_, term);
 	else
 		printf("%s%c", key_, term);
@@ -38,11 +38,11 @@ static int show_config(const char* key_, const char* value_)
 	if (use_key_regexp && regexec(key_regexp, key_, 0, NULL, 0))
 		return 0;
 	if (regexp != NULL &&
-	    (do_not_match ^ !!regexec(regexp, (value_?value_:""), 0, NULL, 0)))
+	    (do_not_match ^ !!regexec(regexp, value_, 0, NULL, 0)))
 		return 0;
 
 	if (show_keys) {
-		if (value_)
+		if (value_ != config_true)
 			printf("%s%c", key_, key_delim);
 		else
 			printf("%s", key_);
@@ -50,11 +50,11 @@ static int show_config(const char* key_, const char* value_)
 	if (seen && !do_all)
 		dup_error = 1;
 	if (type == T_INT)
-		sprintf(value, "%d", git_config_int(key_, value_?value_:""));
+		sprintf(value, "%d", git_config_int(key_, value_));
 	else if (type == T_BOOL)
 		vptr = git_config_bool(key_, value_) ? "true" : "false";
 	else
-		vptr = value_?value_:"";
+		vptr = value_;
 	seen++;
 	if (dup_error) {
 		error("More than one value for the key %s: %s",
diff --git a/cache.h b/cache.h
index 549f4bb..ffe99fc 100644
--- a/cache.h
+++ b/cache.h
@@ -589,6 +589,7 @@ extern int git_config_set_multivar(const char *, const char *, const char *, int
 extern int git_config_rename_section(const char *, const char *);
 extern const char *git_etc_gitconfig(void);
 extern int check_repository_format_version(const char *var, const char *value);
+extern const char config_true[];
 
 #define MAX_GITNAME (1000)
 extern char git_default_email[MAX_GITNAME];
diff --git a/config.c b/config.c
index 498259e..ba381d3 100644
--- a/config.c
+++ b/config.c
@@ -10,6 +10,7 @@
 
 #define MAXNAME (256)
 
+const char config_true[1] = "";
 static FILE *config_file;
 static const char *config_file_name;
 static int config_linenr;
@@ -114,7 +115,7 @@ static inline int iskeychar(int c)
 static int get_value(config_fn_t fn, char *name, unsigned int len)
 {
 	int c;
-	char *value;
+	const char *value;
 
 	/* Get the full name */
 	for (;;) {
@@ -131,7 +132,7 @@ static int get_value(config_fn_t fn, char *name, unsigned int len)
 	while (c == ' ' || c == '\t')
 		c = get_next_char();
 
-	value = NULL;
+	value = config_true;
 	if (c != '\n') {
 		if (c != '=')
 			return -1;
@@ -298,7 +299,7 @@ unsigned long git_config_ulong(const char *name, const char *value)
 
 int git_config_bool(const char *name, const char *value)
 {
-	if (!value)
+	if (value == config_true)
 		return 1;
 	if (!*value)
 		return 0;

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

[Junio C Hamano]

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

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

> 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?

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

[Linus Torvalds]

On Sun, 10 Feb 2008, Junio C Hamano wrote:
> 
> 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.

No it does not.

> will now need to be changed to:
> 
> 	if (value == config_true)
>         	Ah we have true;
> 	else if (!*value)
>         	Ok this is false;

And that was done by my patch. 

> 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;

And this is different from checking against NULL exactly how?

> > 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?

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

		Linus

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

[Junio C Hamano]

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

> On Sun, 10 Feb 2008, Junio C Hamano wrote:
> ...
>> will now need to be changed to:
>> 
>> 	if (value == config_true)
>>         	Ah we have true;
>> 	else if (!*value)
>>         	Ok this is false;
>
> And that was done by my patch. 
>
>> 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;
>
> And this is different from checking against NULL exactly how?

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.

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

[Junio C Hamano]

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

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

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.

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

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1315 bytes --]

On Sun, Feb 10, 2008 at 10:47:02PM +0000, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > 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.
> 
> 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.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

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

[Linus Torvalds]

On Sun, 10 Feb 2008, Junio C Hamano wrote:
> >
> > And this is different from checking against NULL exactly how?
> 
> Exactly.  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.

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

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

[Linus Torvalds]

On Sun, 10 Feb 2008, Linus Torvalds wrote:
> 
> 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.

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

diff --git a/color.c b/color.c
index cb70340..e5fbf01 100644
--- a/color.c
+++ b/color.c
@@ -118,15 +118,12 @@ bad:
 
 int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
 {
-	if (value) {
-		if (!strcasecmp(value, "never"))
-			return 0;
-		if (!strcasecmp(value, "always"))
-			return 1;
-		if (!strcasecmp(value, "auto"))
-			goto auto_color;
-	}
-
+	if (!strcasecmp(value, "never"))
+		return 0;
+	if (!strcasecmp(value, "always"))
+		return 1;
+	if (!strcasecmp(value, "auto"))
+		goto auto_color;
 	/* Missing or explicit false to turn off colorization */
 	if (!git_config_bool(var, value))
 		return 0;

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

[Linus Torvalds]

On Mon, 11 Feb 2008, Pierre Habouzit wrote:
>
> > 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

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

[Junio C Hamano]

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

> 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?

Yes, that is a good example of simplification.

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

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.

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

[Christian Couder]

Le lundi 11 février 2008, Linus Torvalds a écrit :
> 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).

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

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

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.

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

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

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

[Daniel Barkalow]

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

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > 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);

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

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*