{"thread":{"id":"11804","subject":"[bug] generic issue with git_config handlers","startedAt":"2008-01-31T09:16:27Z","lastAt":"2008-02-07T05:45:23Z","messageCount":8,"participants":["Pierre Habouzit","Junio C Hamano","Christian Couder","Johannes Sixt"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"66964","messageId":"20080131091627.GB24403@artemis.madism.org","threadId":"11804","inReplyTo":null,"subject":"[bug] generic issue with git_config handlers","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-01-31T09:16:27Z","receivedAt":"2008-01-31T09:16:27Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"\n One of my co-workers stumbled upon a misfeature of the git config\nparser. The following syntax is allowed:\n\n [section]\n\t foo\n\n\n I saw that this is a feature, though as a consequence, the \"value\"\npassed to git_config handlers may be NULL, and a _lot_ of git config\nhandlers don't know this could happen. This becomes an issue when you do\nsomething like:\n\n[user]\n name\n\n--> every git command segfaults basically\n\n[alias]\n foo\n\n--> `git foo` segfaults\n\n\n I wanted to fix that, and generate nicer errors than a crash, changing\ngit_config to also take a boolean argument telling if the caller expects\n\"value\" to be NULL, or would like to reject it, though the code has so\nmany callbacks to fix, and I have too little time right now, that I just\ndrop the thing on the list, hoping that some nice soul will take care of\nthe issue.\n\nCheers,\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"66968","messageId":"7v1w7ye48j.fsf@gitster.siamese.dyndns.org","threadId":"11804","inReplyTo":"20080131091627.GB24403@artemis.madism.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-31T09:25:32Z","receivedAt":"2008-01-31T09:25:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pierre Habouzit writes:\n\n> One of my co-workers stumbled upon a misfeature of the git config\n> parser. The following syntax is allowed:\n>\n> [section]\n> \t foo\n\nYeah, that is how \"truth\" value of boolean is spelled.\n\n> [user]\n> name\n\nThat's very unfortunate. Whatever is expecting string value\nshould check for NULL. Fix should probably be easy enough for\nany git-hacker-wannabe to tackle ;-)\n"},{"id":"66975","messageId":"20080131101037.GG24403@artemis.madism.org","threadId":"11804","inReplyTo":"7v1w7ye48j.fsf@gitster.siamese.dyndns.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2008-01-31T10:10:37Z","receivedAt":"2008-01-31T10:10:37Z","isPatch":false,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:\n> Pierre Habouzit writes:\n> \n> > One of my co-workers stumbled upon a misfeature of the git config\n> > parser. The following syntax is allowed:\n> >\n> > [section]\n> > \t foo\n> \n> Yeah, that is how \"truth\" value of boolean is spelled.\n> \n> > [user]\n> > name\n> \n> That's very unfortunate. Whatever is expecting string value\n> should check for NULL. Fix should probably be easy enough for\n> any git-hacker-wannabe to tackle ;-)\n\n I think so too, though my count is something like 40 functions to\ninvestigate (the 40 handlers) and where it recurses into ;) Too much\nwork for the time I have right now.\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"67320","messageId":"200802040727.44522.chriscool@tuxfamily.org","threadId":"11804","inReplyTo":"20080131101037.GG24403@artemis.madism.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-04T06:27:44Z","receivedAt":"2008-02-04T06:27:44Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :\n> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:\n> > Pierre Habouzit writes:\n> > > One of my co-workers stumbled upon a misfeature of the git config\n> > > parser. The following syntax is allowed:\n> > >\n> > > [section]\n> > > \t foo\n> >\n> > Yeah, that is how \"truth\" value of boolean is spelled.\n> >\n> > > [user]\n> > > name\n> >\n> > That's very unfortunate. Whatever is expecting string value\n> > should check for NULL. Fix should probably be easy enough for\n> > any git-hacker-wannabe to tackle ;-)\n>\n> I think so too, though my count is something like 40 functions to\n> investigate (the 40 handlers) and where it recurses into ;) Too much\n> work for the time I have right now.\n\nI would suggest this patch:\n\n---8<---\ndiff --git a/config.c b/config.c\nindex 526a3f4..92613c5 100644\n--- a/config.c\n+++ b/config.c\n@@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name, \nunsigned in\n if (!value)\n return -1;\n }\n- return fn(name, value);\n+ return fn(name, value ? value : \"\");\n }\n\n static int get_extended_base_var(char *name, int baselen, int c)\n---8<---\n\nbut it breaks some test cases.\n\n$ ./t1300-repo-config.sh -d -i -v\n\n[...]\n\n* expecting success: git config --get-regexp novalue > output &&\n cmp output expect\noutput expect differ: char 17, line 1\n* FAIL 34: get-regexp variable with no value\n git config --get-regexp novalue > output &&\n cmp output expect\n\n$ cat output | hexdump -C\n00000000 6e 6f 76 61 6c 75 65 2e 76 61 72 69 61 62 6c 65 |\nnovalue.variable|\n00000010 20 0a | .|\n00000012\n\n$ cat expect | hexdump -C\n00000000 6e 6f 76 61 6c 75 65 2e 76 61 72 69 61 62 6c 65 |\nnovalue.variable|\n00000010 0a |.|\n00000011\n\nI don't know if the added space is a big problem.\n\nIt comes from the following code in builtin-config.c:44\n\n\tif (show_keys) {\n\t\tif (value_)\n\t\t\tprintf(\"%s%c\", key_, key_delim);\n\t\telse\n\t\t\tprintf(\"%s\", key_);\n\nwhere \"value_\" is now \"\" instead of NULL.\n\nAt this point, as I don't know much the code in these files, I think I could \nvery well use some advice from people more familiar with this.\n\nThanks in advance,\nChristian.\n"},{"id":"67382","messageId":"47A74503.4090201@viscovery.net","threadId":"11804","inReplyTo":"200802040727.44522.chriscool@tuxfamily.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-02-04T17:01:55Z","receivedAt":"2008-02-04T17:01:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Christian Couder schrieb:\n> Le jeudi 31 janvier 2008, Pierre Habouzit a écrit :\n>> On Thu, Jan 31, 2008 at 09:25:32AM +0000, Junio C Hamano wrote:\n>>> Pierre Habouzit writes:\n>>>> One of my co-workers stumbled upon a misfeature of the git config\n>>>> parser. The following syntax is allowed:\n>>>>\n>>>> [section]\n>>>> \t foo\n>>> Yeah, that is how \"truth\" value of boolean is spelled.\n>>>\n>>>> [user]\n>>>> name\n>>> That's very unfortunate. Whatever is expecting string value\n>>> should check for NULL. Fix should probably be easy enough for\n>>> any git-hacker-wannabe to tackle ;-)\n>> I think so too, though my count is something like 40 functions to\n>> investigate (the 40 handlers) and where it recurses into ;) Too much\n>> work for the time I have right now.\n> \n> I would suggest this patch:\n> \n> ---8<---\n> diff --git a/config.c b/config.c\n> index 526a3f4..92613c5 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -139,7 +139,7 @@ static int get_value(config_fn_t fn, char *name, \n> unsigned in\n> if (!value)\n> return -1;\n> }\n> - return fn(name, value);\n> + return fn(name, value ? value : \"\");\n> }\n\nYou can't. The reason is that get_config_bool() treats value == NULL and\n*value == '\\0' differently. *That's* the most unfortunate part of it. :-(\n\n-- Hannes\n"},{"id":"67446","messageId":"200802050013.20335.chriscool@tuxfamily.org","threadId":"11804","inReplyTo":"47A74503.4090201@viscovery.net","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-04T23:13:20Z","receivedAt":"2008-02-04T23:13:20Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le lundi 4 février 2008, Johannes Sixt a écrit :\n> Christian Couder schrieb:\n> > return -1;\n> > }\n> > - return fn(name, value);\n> > + return fn(name, value ? value : \"\");\n> > }\n>\n> You can't. The reason is that get_config_bool() treats value == NULL and\n> *value == '\\0' differently. *That's* the most unfortunate part of it. :-(\n\nYou are right. We have this (in config.c:299):\n\nint git_config_bool(const char *name, const char *value)\n{\n\tif (!value)\n\t\treturn 1;\n\tif (!*value)\n\t\treturn 0;\n\tif (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n\t\treturn 1;\n\tif (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n\t\treturn 0;\n\treturn git_config_int(name, value) != 0;\n}\n\nVery unfortunate.\n\nI finally had the following patch that passed all tests (it changed only one \ntest), in case someone wants to suggest that we change git_config_bool, \nhint, hint!\n\nThanks,\nChristian.\n\n---8<---\ndiff --git a/builtin-config.c b/builtin-config.c\nindex e4a12e3..b92cf4b 100644\n--- a/builtin-config.c\n+++ b/builtin-config.c\n@@ -20,7 +20,7 @@ static enum { T_RAW, T_INT, T_BOOL } type = T_RAW;\n\n static int show_all_config(const char *key_, const char *value_)\n {\n- if (value_)\n+ if (value_ && *value_)\n printf(\"%s%c%s%c\", key_, delim, value_, term);\n else\n printf(\"%s%c\", key_, term);\n@@ -42,7 +42,7 @@ static int show_config(const char* key_, const char* \nvalue_)\n return 0;\n\n if (show_keys) {\n- if (value_)\n+ if (value_ && *value_)\n printf(\"%s%c\", key_, key_delim);\n else\n printf(\"%s\", key_);\ndiff --git a/config.c b/config.c\nindex 526a3f4..a2c7214 100644\n--- a/config.c\n+++ b/config.c\n@@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name, \nunsigned in\n while (c == ' ' || c == '\\t')\n c = get_next_char();\n\n- value = NULL;\n+ value = \"\";\n if (c != '\\n') {\n if (c != '=')\n return -1;\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex a786c5c..deb11dc 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -611,8 +611,7 @@ foo\n barQsection.sub=section.val3\n\n\n-Qsection.sub=section.val4\n-Qsection.sub=section.val5Q\n+Qsection.sub=section.val4Qsection.sub=section.val5Q\n EOF\n\n git config --null --list | tr '\\000' 'Q' > result\n---8<---\n"},{"id":"67450","messageId":"7vk5lki815.fsf@gitster.siamese.dyndns.org","threadId":"11804","inReplyTo":"200802050013.20335.chriscool@tuxfamily.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-02-05T00:03:34Z","receivedAt":"2008-02-05T00:03:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Very unfortunate.\n>\n> I finally had the following patch that passed all tests (it changed only one \n> test), in case someone wants to suggest that we change git_config_bool, \n> hint, hint!\n\nSorry, I do not get what you are hinting at. The fact that you\npassed all the tests suggests that we have a gap in the test\ncoverage for these two, so you are inviting more tests from\nothers?\n\n> diff --git a/config.c b/config.c\n> index 526a3f4..a2c7214 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -131,7 +131,7 @@ static int get_value(config_fn_t fn, char *name, \n> unsigned in\n> while (c == ' ' || c == '\\t')\n> c = get_next_char();\n>\n> - value = NULL;\n> + value = \"\";\n> if (c != '\\n') {\n> if (c != '=')\n> return -1;\n\nAs long as you have this, I do not think you can avoid breaking\nexisting repositories that have:\n\n [core]\n autocrlf\n\t\tfilemode =\n\nand expect git to say \"Ah, core.autocrlf is set to true, and\nfilemode is not trustable, so I need to do a MS-DOG\".\n\n\t$ git config --bool core.autocrlf\n true\n $ git config --bool core.filemode\n false\n\nYour \"builtin-config.c\" patch looks better than before (which\nwould segfault), but I think\n\n\t$ git config --bool --list\n\ncould pay attention to the \"type\" thing set earlier, just like\nshow_config() does.\n"},{"id":"67752","messageId":"200802070645.23840.chriscool@tuxfamily.org","threadId":"11804","inReplyTo":"7vk5lki815.fsf@gitster.siamese.dyndns.org","subject":"Re: [bug] generic issue with git_config handlers","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2008-02-07T05:45:23Z","receivedAt":"2008-02-07T05:45:23Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Le mardi 5 février 2008, Junio C Hamano a écrit :\n> Christian Couder writes:\n> > Very unfortunate.\n> >\n> > I finally had the following patch that passed all tests (it changed\n> > only one test), in case someone wants to suggest that we change\n> > git_config_bool, hint, hint!\n>\n> Sorry, I do not get what you are hinting at. \n\nWell, I wanted someone else to suggest that we deprecate using \"\" as a \nboolean false.\n\nSomething like this (completely untested) :\n\ndiff --git a/config.c b/config.c\nindex 526a3f4..44afeaa 100644\n--- a/config.c\n+++ b/config.c\n@@ -300,8 +300,19 @@ int git_config_bool(const char *name, const char \n*value)\n {\n if (!value)\n return 1;\n- if (!*value)\n+ if (!*value) {\n+ fprintf(stderr,\n+ \"Warning: using an empty value for boolean config \"\n+ \"variables is deprecated.\\n\"\n+ \"An empty value currently means 'false' as a \"\n+ \"boolean, but may very well means 'true' in the \"\n+ \"future!\\n\"\n+ \"Please consider using a 'false' value explicitely \"\n+ \"for variable '%s', so that your config is future \"\n+ \"proof. You can do that using:\\n\"\n+ \"\\tgit config %s false\\n\", name, name)\n return 0;\n+ }\n if (!strcasecmp(value, \"true\") || !strcasecmp(value, \"yes\"))\n return 1;\n if (!strcasecmp(value, \"false\") || !strcasecmp(value, \"no\"))\n\nSo that in a future version we can use \"\" internaly for both no value and \nempty value variables and consider them meaning the same.\n\n> The fact that you\n> passed all the tests suggests that we have a gap in the test\n> coverage for these two, so you are inviting more tests from\n> others?\n\nI just sent a patch to close this gap.\n\n[...]\n\n> Your \"builtin-config.c\" patch looks better than before (which\n> would segfault), but I think\n>\n> \t$ git config --bool --list\n>\n> could pay attention to the \"type\" thing set earlier, just like\n> show_config() does.\n\nI didn't try this, but I will.\n\nThanks,\nChristian.\n"}]}