[bug] generic issue with git_config handlers

[bug] generic issue with git_config handlers

[Pierre Habouzit]

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


  One of my co-workers stumbled upon a misfeature of the git config
parser. The following syntax is allowed:

    [section]
	 foo


  I saw that this is a feature, though as a consequence, the "value"
passed to git_config handlers may be NULL, and a _lot_ of git config
handlers don't know this could happen. This becomes an issue when you do
something like:

[user]
    name

--> every git command segfaults basically

[alias]
    foo

--> `git foo` segfaults


  I wanted to fix that, and generate nicer errors than a crash, changing
git_config to also take a boolean argument telling if the caller expects
"value" to be NULL, or would like to reject it, though the code has so
many callbacks to fix, and I have too little time right now, that I just
drop the thing on the list, hoping that some nice soul will take care of
the issue.

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

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

Re: [bug] generic issue with git_config handlers

[Junio C Hamano]

Pierre Habouzit <madcoder@debian.org> writes:

>   One of my co-workers stumbled upon a misfeature of the git config
> parser. The following syntax is allowed:
>
>     [section]
> 	 foo

Yeah, that is how "truth" value of boolean is spelled.

> [user]
>     name

That's very unfortunate.  Whatever is expecting string value
should check for NULL.  Fix should probably be easy enough for
any git-hacker-wannabe to tackle ;-)

Re: [bug] generic issue with git_config handlers

[Pierre Habouzit]

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

On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
> Pierre Habouzit <madcoder@debian.org> writes:
> 
> >   One of my co-workers stumbled upon a misfeature of the git config
> > parser. The following syntax is allowed:
> >
> >     [section]
> > 	 foo
> 
> Yeah, that is how "truth" value of boolean is spelled.
> 
> > [user]
> >     name
> 
> That's very unfortunate.  Whatever is expecting string value
> should check for NULL.  Fix should probably be easy enough for
> any git-hacker-wannabe to tackle ;-)

  I think so too, though my count is something like 40 functions to
investigate (the 40 handlers) and where it recurses into ;) Too much
work for the time I have right now.

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

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

Re: [bug] generic issue with git_config handlers

[Christian Couder]

Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :
> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
> > Pierre Habouzit <madcoder@debian.org> writes:
> > >   One of my co-workers stumbled upon a misfeature of the git config
> > > parser. The following syntax is allowed:
> > >
> > >     [section]
> > > 	 foo
> >
> > Yeah, that is how "truth" value of boolean is spelled.
> >
> > > [user]
> > >     name
> >
> > That's very unfortunate.  Whatever is expecting string value
> > should check for NULL.  Fix should probably be easy enough for
> > any git-hacker-wannabe to tackle ;-)
>
>   I think so too, though my count is something like 40 functions to
> investigate (the 40 handlers) and where it recurses into ;) Too much
> work for the time I have right now.

I would suggest this patch:

---8<---
diff --git a/config.c b/config.c
index 526a3f4..92613c5 100644
--- a/config.c
+++ b/config.c
@@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name, 
unsigned in
                if (!value)
                        return -1;
        }
-       return fn(name, value);
+       return fn(name, value ? value : "");
 }

 static int get_extended_base_var(char *name, int baselen, int c)
---8<---

but it breaks some test cases.

$ ./t1300-repo-config.sh -d -i -v

[...]

* expecting success: git config --get-regexp novalue > output &&
         cmp output expect
output expect differ: char 17, line 1
* FAIL 34: get-regexp variable with no value
        git config --get-regexp novalue > output &&
                 cmp output expect

$ cat output | hexdump -C
00000000  6e 6f 76 61 6c 75 65 2e  76 61 72 69 61 62 6c 65  |
novalue.variable|
00000010  20 0a                                             | .|
00000012

$ cat expect | hexdump -C
00000000  6e 6f 76 61 6c 75 65 2e  76 61 72 69 61 62 6c 65  |
novalue.variable|
00000010  0a                                                |.|
00000011

I don't know if the added space is a big problem.

It comes from the following code in builtin-config.c:44

	if (show_keys) {
		if (value_)
			printf("%s%c", key_, key_delim);
		else
			printf("%s", key_);

where "value_" is now "" instead of NULL.

At this point, as I don't know much the code in these files, I think I could 
very well use some advice from people more familiar with this.

Thanks in advance,
Christian.

Re: [bug] generic issue with git_config handlers

[Johannes Sixt]

Christian Couder schrieb:
> Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :
>> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:
>>> Pierre Habouzit <madcoder@debian.org> writes:
>>>>   One of my co-workers stumbled upon a misfeature of the git config
>>>> parser. The following syntax is allowed:
>>>>
>>>>     [section]
>>>> 	 foo
>>> Yeah, that is how "truth" value of boolean is spelled.
>>>
>>>> [user]
>>>>     name
>>> That's very unfortunate.  Whatever is expecting string value
>>> should check for NULL.  Fix should probably be easy enough for
>>> any git-hacker-wannabe to tackle ;-)
>>   I think so too, though my count is something like 40 functions to
>> investigate (the 40 handlers) and where it recurses into ;) Too much
>> work for the time I have right now.
> 
> I would suggest this patch:
> 
> ---8<---
> diff --git a/config.c b/config.c
> index 526a3f4..92613c5 100644
> --- a/config.c
> +++ b/config.c
> @@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name, 
> unsigned in
>                 if (!value)
>                         return -1;
>         }
> -       return fn(name, value);
> +       return fn(name, value ? value : "");
>  }

You can't. The reason is that get_config_bool() treats value == NULL and
*value == '\0' differently. *That's* the most unfortunate part of it. :-(

-- Hannes

Re: [bug] generic issue with git_config handlers

[Christian Couder]

Le lundi 4 février 2008, Johannes Sixt a écrit :
> Christian Couder schrieb:
> >                         return -1;
> >         }
> > -       return fn(name, value);
> > +       return fn(name, value ? value : "");
> >  }
>
> You can't. The reason is that get_config_bool() treats value == NULL and
> *value == '\0' differently. *That's* the most unfortunate part of it. :-(

You are right. We have this (in config.c:299):

int git_config_bool(const char *name, const char *value)
{
	if (!value)
		return 1;
	if (!*value)
		return 0;
	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;
}

Very unfortunate.

I finally had the following patch that passed all tests (it changed only one 
test), in case someone wants to suggest that we change git_config_bool, 
hint, hint!

Thanks,
Christian.

---8<---
diff --git a/builtin-config.c b/builtin-config.c
index e4a12e3..b92cf4b 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_ && *value_)
                printf("%s%c%s%c", key_, delim, value_, term);
        else
                printf("%s%c", key_, term);
@@ -42,7 +42,7 @@ static int show_config(const char* key_, const char* 
value_)
                return 0;

        if (show_keys) {
-               if (value_)
+               if (value_ && *value_)
                        printf("%s%c", key_, key_delim);
                else
                        printf("%s", key_);
diff --git a/config.c b/config.c
index 526a3f4..a2c7214 100644
--- a/config.c
+++ b/config.c
@@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name, 
unsigned in
        while (c == ' ' || c == '\t')
                c = get_next_char();

-       value = NULL;
+       value = "";
        if (c != '\n') {
                if (c != '=')
                        return -1;
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index a786c5c..deb11dc 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -611,8 +611,7 @@ foo
 barQsection.sub=section.val3


-Qsection.sub=section.val4
-Qsection.sub=section.val5Q
+Qsection.sub=section.val4Qsection.sub=section.val5Q
 EOF

 git config --null --list | tr '\000' 'Q' > result
---8<---

Re: [bug] generic issue with git_config handlers

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Very unfortunate.
>
> I finally had the following patch that passed all tests (it changed only one 
> test), in case someone wants to suggest that we change git_config_bool, 
> hint, hint!

Sorry, I do not get what you are hinting at.  The fact that you
passed all the tests suggests that we have a gap in the test
coverage for these two, so you are inviting more tests from
others?

> diff --git a/config.c b/config.c
> index 526a3f4..a2c7214 100644
> --- a/config.c
> +++ b/config.c
> @@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name, 
> unsigned in
>         while (c == ' ' || c == '\t')
>                 c = get_next_char();
>
> -       value = NULL;
> +       value = "";
>         if (c != '\n') {
>                 if (c != '=')
>                         return -1;

As long as you have this, I do not think you can avoid breaking
existing repositories that have:

        [core]
                autocrlf
		filemode =

and expect git to say "Ah, core.autocrlf is set to true, and
filemode is not trustable, so I need to do a MS-DOG".

	$ git config --bool core.autocrlf
        true
        $ git config --bool core.filemode
        false

Your "builtin-config.c" patch looks better than before (which
would segfault), but I think

	$ git config --bool --list

could pay attention to the "type" thing set earlier, just like
show_config() does.

Re: [bug] generic issue with git_config handlers

[Christian Couder]

Le mardi 5 février 2008, Junio C Hamano a écrit :
> Christian Couder <chriscool@tuxfamily.org> writes:
> > Very unfortunate.
> >
> > I finally had the following patch that passed all tests (it changed
> > only one test), in case someone wants to suggest that we change
> > git_config_bool, hint, hint!
>
> Sorry, I do not get what you are hinting at. 

Well, I wanted someone else to suggest that we deprecate using "" as a 
boolean false.

Something like this (completely untested) :

diff --git a/config.c b/config.c
index 526a3f4..44afeaa 100644
--- a/config.c
+++ b/config.c
@@ -300,8 +300,19 @@ int git_config_bool(const char *name, const char 
*value)
 {
        if (!value)
                return 1;
-       if (!*value)
+       if (!*value) {
+               fprintf(stderr,
+                       "Warning: using an empty value for boolean config "
+                       "variables is deprecated.\n"
+                       "An empty value currently means 'false' as a "
+                       "boolean, but may very well means 'true' in the "
+                       "future!\n"
+                       "Please consider using a 'false' value explicitely "
+                       "for variable '%s', so that your config is future "
+                       "proof. You can do that using:\n"
+                       "\tgit config %s false\n", name, name)
                return 0;
+       }
        if (!strcasecmp(value, "true") || !strcasecmp(value, "yes"))
                return 1;
        if (!strcasecmp(value, "false") || !strcasecmp(value, "no"))

So that in a future version we can use "" internaly for both no value and 
empty value variables and consider them meaning the same.

> The fact that you
> passed all the tests suggests that we have a gap in the test
> coverage for these two, so you are inviting more tests from
> others?

I just sent a patch to close this gap.

[...]

> Your "builtin-config.c" patch looks better than before (which
> would segfault), but I think
>
> 	$ git config --bool --list
>
> could pay attention to the "type" thing set earlier, just like
> show_config() does.

I didn't try this, but I will.

Thanks,
Christian.