Re: [bug] generic issue with git_config handlers

Pierre Habouzit [off-list ref] 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 ;-)

On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:

Pierre Habouzit [off-list ref] writes:

quoted

  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.

quoted

[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

Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :

On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:

quoted

Pierre Habouzit [off-list ref] writes:

quoted

  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.

quoted

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

Christian Couder schrieb:

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

Le lundi 4 février 2008, Johannes Sixt a écrit :

Christian Couder schrieb:

quoted

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