{"thread":{"id":"20595","subject":"git config -> \"fatal: bad config file\"","startedAt":"2009-08-14T13:38:02Z","lastAt":"2009-09-04T08:40:34Z","messageCount":14,"participants":["David Reitter","Michael J Gruber","Jakub Narebski","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"120632","messageId":"A5CDBB91-E889-4849-953A-2C1DB4A04513@gmail.com","threadId":"20595","inReplyTo":null,"subject":"git config -> \"fatal: bad config file\"","fromName":"David Reitter","fromEmail":"david.reitter@gmail.com","sentAt":"2009-08-14T13:38:02Z","receivedAt":"2009-08-14T13:38:02Z","isPatch":false,"sender":{"key":"david.reitter@gmail.com","avatar":"https://gravatar.com/avatar/0b9f43c735737567aec3272fea5afa461ffa049f180103c1aab629aa6c1207f1?d=mp&s=160"},"body":"I made a mistake editing my config file using \"git config -e\".  This  \ncaused git commands to fail.\n\nTrying to fix the problem, I did\n\ngit config -e\nfatal: bad config file line 7 in .git/config\n\nI think the refusal to edit a broken config file is not a good idea.   \nIt's easy to fix for me of course by editing .git/config directly, but  \ngit-config should probably not read the config file at all.\n\nThanks for your consideration.\n"},{"id":"120633","messageId":"4A85724C.5060406@drmicha.warpmail.net","threadId":"20595","inReplyTo":"A5CDBB91-E889-4849-953A-2C1DB4A04513@gmail.com","subject":"Re: git config -> \"fatal: bad config file\"","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-08-14T14:18:52Z","receivedAt":"2009-08-14T14:18:52Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"David Reitter venit, vidit, dixit 14.08.2009 15:38:\n> I made a mistake editing my config file using \"git config -e\".  This  \n> caused git commands to fail.\n> \n> Trying to fix the problem, I did\n> \n> git config -e\n> fatal: bad config file line 7 in .git/config\n> \n> I think the refusal to edit a broken config file is not a good idea.   \n> It's easy to fix for me of course by editing .git/config directly, but  \n> git-config should probably not read the config file at all.\n> \n> Thanks for your consideration.\n> \n\ngit needs to read the file because the editor could be configured there!\nThe only option would be to make git config -e continue past that error.\n\nMichael\n"},{"id":"120634","messageId":"2D345CF1-9534-46F2-B0DF-ADA4EFDB5A51@gmail.com","threadId":"20595","inReplyTo":"4A85724C.5060406@drmicha.warpmail.net","subject":"Re: git config -> \"fatal: bad config file\"","fromName":"David Reitter","fromEmail":"david.reitter@gmail.com","sentAt":"2009-08-14T14:26:07Z","receivedAt":"2009-08-14T14:26:07Z","isPatch":false,"sender":{"key":"david.reitter@gmail.com","avatar":"https://gravatar.com/avatar/0b9f43c735737567aec3272fea5afa461ffa049f180103c1aab629aa6c1207f1?d=mp&s=160"},"body":"On Aug 14, 2009, at 10:18 AM, Michael J Gruber wrote:\n\n> git needs to read the file because the editor could be configured  \n> there!\n> The only option would be to make git config -e continue past that  \n> error.\n\n\nSyntax errors in .git/config could lead to warnings. Since the file is  \nprimarily line-oriented anyways (except for groups), recovery should  \nbe easy.\n\nAlso, you could have git-config -e edit a temporary file, check the  \nfile for errors after editing and then move it to .git/config.\n"},{"id":"120635","messageId":"m3ocqio4gz.fsf@localhost.localdomain","threadId":"20595","inReplyTo":"4A85724C.5060406@drmicha.warpmail.net","subject":"Re: git config -> \"fatal: bad config file\"","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-14T14:28:23Z","receivedAt":"2009-08-14T14:28:23Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n> David Reitter venit, vidit, dixit 14.08.2009 15:38:\n\n> > I made a mistake editing my config file using \"git config -e\".  This  \n> > caused git commands to fail.\n> > \n> > Trying to fix the problem, I did\n> > \n> > git config -e\n> > fatal: bad config file line 7 in .git/config\n> > \n> > I think the refusal to edit a broken config file is not a good idea.   \n> > It's easy to fix for me of course by editing .git/config directly, but  \n> > git-config should probably not read the config file at all.\n> > \n> > Thanks for your consideration.\n> > \n> \n> git needs to read the file because the editor could be configured there!\n> The only option would be to make git config -e continue past that error.\n\nWell, it shows you which file the error is, but I think as a special\ncase \"git config [file] --edit\" should show also (absolute?) pathname\nof the file it tired to open.\n\ncore.editor can be in any place.\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"120637","messageId":"a812f567b4541ce55e9c60037a047488a0893c36.1250262273.git.git@drmicha.warpmail.net","threadId":"20595","inReplyTo":"A5CDBB91-E889-4849-953A-2C1DB4A04513@gmail.com","subject":"[PATCH] git-config: Parse config files leniently","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-08-14T15:10:48Z","receivedAt":"2009-08-14T15:10:48Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, git config dies as soon as there is a parsing error. This is\nespecially unfortunate in case a user tries to correct config mistakes\nusing git config -e.\n\nInstead, issue a warning only and treat the rest of the line as a\ncomment (ignore it). This benefits not only git config -e users.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\nReported-by: David Reitter <david.reitter@gmail.com>\n---\nThe diff and stat are overstatements. I had to shift that whole for loop\nby one tab stop but diff (with or without --color-words) does not recognize\nthis. Nothing inside the loop is changed.\nThe outer while makes sure that\n* we switch to comment mode after a parsing error\n* we return eventually no matter whether the file ends with \\n or not.\n\nTest had to be adjusted as well.\n\n config.c                |   80 ++++++++++++++++++++++++----------------------\n t/t1303-wacky-config.sh |    4 +-\n 2 files changed, 44 insertions(+), 40 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex e87edea..5e0af5d 100644\n--- a/config.c\n+++ b/config.c\n@@ -207,50 +207,54 @@ static int git_parse_file(config_fn_t fn, void *data)\n \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n \tconst unsigned char *bomptr = utf8_bom;\n \n-\tfor (;;) {\n-\t\tint c = get_next_char();\n-\t\tif (bomptr && *bomptr) {\n-\t\t\t/* We are at the file beginning; skip UTF8-encoded BOM\n-\t\t\t * if present. Sane editors won't put this in on their\n-\t\t\t * own, but e.g. Windows Notepad will do it happily. */\n-\t\t\tif ((unsigned char) c == *bomptr) {\n-\t\t\t\tbomptr++;\n+\twhile (!config_file_eof) {\n+\t\tfor (;;) {\n+\t\t\tint c = get_next_char();\n+\t\t\tif (bomptr && *bomptr) {\n+\t\t\t\t/* We are at the file beginning; skip UTF8-encoded BOM\n+\t\t\t\t * if present. Sane editors won't put this in on their\n+\t\t\t\t * own, but e.g. Windows Notepad will do it happily. */\n+\t\t\t\tif ((unsigned char) c == *bomptr) {\n+\t\t\t\t\tbomptr++;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t} else {\n+\t\t\t\t\t/* Do not tolerate partial BOM. */\n+\t\t\t\t\tif (bomptr != utf8_bom)\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\t/* No BOM at file beginning. Cool. */\n+\t\t\t\t\tbomptr = NULL;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif (c == '\\n') {\n+\t\t\t\tif (config_file_eof)\n+\t\t\t\t\treturn 0;\n+\t\t\t\tcomment = 0;\n \t\t\t\tcontinue;\n-\t\t\t} else {\n-\t\t\t\t/* Do not tolerate partial BOM. */\n-\t\t\t\tif (bomptr != utf8_bom)\n+\t\t\t}\n+\t\t\tif (comment || isspace(c))\n+\t\t\t\tcontinue;\n+\t\t\tif (c == '#' || c == ';') {\n+\t\t\t\tcomment = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (c == '[') {\n+\t\t\t\tbaselen = get_base_var(var);\n+\t\t\t\tif (baselen <= 0)\n \t\t\t\t\tbreak;\n-\t\t\t\t/* No BOM at file beginning. Cool. */\n-\t\t\t\tbomptr = NULL;\n+\t\t\t\tvar[baselen++] = '.';\n+\t\t\t\tvar[baselen] = 0;\n+\t\t\t\tcontinue;\n \t\t\t}\n-\t\t}\n-\t\tif (c == '\\n') {\n-\t\t\tif (config_file_eof)\n-\t\t\t\treturn 0;\n-\t\t\tcomment = 0;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (comment || isspace(c))\n-\t\t\tcontinue;\n-\t\tif (c == '#' || c == ';') {\n-\t\t\tcomment = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (c == '[') {\n-\t\t\tbaselen = get_base_var(var);\n-\t\t\tif (baselen <= 0)\n+\t\t\tif (!isalpha(c))\n+\t\t\t\tbreak;\n+\t\t\tvar[baselen] = tolower(c);\n+\t\t\tif (get_value(fn, data, var, baselen+1) < 0)\n \t\t\t\tbreak;\n-\t\t\tvar[baselen++] = '.';\n-\t\t\tvar[baselen] = 0;\n-\t\t\tcontinue;\n \t\t}\n-\t\tif (!isalpha(c))\n-\t\t\tbreak;\n-\t\tvar[baselen] = tolower(c);\n-\t\tif (get_value(fn, data, var, baselen+1) < 0)\n-\t\t\tbreak;\n+\t\twarning(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\t\tcomment = 1;\n \t}\n-\tdie(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\treturn -1;\n }\n \n static int parse_unit_factor(const char *end, unsigned long *val)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 080117c..be850c5 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -9,7 +9,7 @@ setup() {\n }\n \n check() {\n-\techo \"$2\" >expected\n+\tprintf \"$2\\n\" >expected\n \tgit config --get \"$1\" >actual 2>&1\n \ttest_cmp actual expected\n }\n@@ -44,7 +44,7 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n test_expect_success 'do not crash on special long config line' '\n \tsetup &&\n \tgit config section.key \"$LONG_VALUE\" &&\n-\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+\tcheck section.key \"warning: bad config file line 2 in .git/config\\nwarning: bad config file line 2 in .git/config\"\n '\n \n test_done\n-- \n1.6.4.225.gb589e\n"},{"id":"120647","messageId":"7veirejhqq.fsf@alter.siamese.dyndns.org","threadId":"20595","inReplyTo":"a812f567b4541ce55e9c60037a047488a0893c36.1250262273.git.git@drmicha.warpmail.net","subject":"Re: [PATCH] git-config: Parse config files leniently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-14T19:52:13Z","receivedAt":"2009-08-14T19:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Currently, git config dies as soon as there is a parsing error. This is\n> especially unfortunate in case a user tries to correct config mistakes\n> using git config -e.\n>\n> Instead, issue a warning only and treat the rest of the line as a\n> comment (ignore it). This benefits not only git config -e users.\n\n... a broken sentence in the middle?  I would have expected the \"not only\"\nfollowed by \"but also\"; the question is \"but also what?\"\n\nHopefully the benefit is not that it now allows all the other commands to\ncause unspecified types of damage to the repository by following iffy\nsettings obtained from a broken configuration file.\n\n> Reported-by: David Reitter <david.reitter@gmail.com>\n> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n\n> Test had to be adjusted as well.\n\nThe change to the test demonstrates the issue rather well.  The check()\nshell function does not check the exit value from \"git config --get\", but\nin a real script that cares to check and stop on error, this change will\nnow let the script go on, leaving the breakage unnoticed.  I suspect\ncommand implemented in C, that call git_config(), will also have the same\nissue, and I cannot convince myself this is a good change in general,\noutside the scope of helping \"git config -e\".\n\nBut I may be being overly cautious.\n\nBy the way, why did you have to change s/echo/printf/?  Can't you give two\nlines in a single argument without \"\\n\" escape?\n\n> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\n> index 080117c..be850c5 100755\n> --- a/t/t1303-wacky-config.sh\n> +++ b/t/t1303-wacky-config.sh\n> @@ -9,7 +9,7 @@ setup() {\n>  }\n>  \n>  check() {\n> -\techo \"$2\" >expected\n> +\tprintf \"$2\\n\" >expected\n>  \tgit config --get \"$1\" >actual 2>&1\n>  \ttest_cmp actual expected\n>  }\n> @@ -44,7 +44,7 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n>  test_expect_success 'do not crash on special long config line' '\n>  \tsetup &&\n>  \tgit config section.key \"$LONG_VALUE\" &&\n> -\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n> +\tcheck section.key \"warning: bad config file line 2 in .git/config\\nwarning: bad config file line 2 in .git/config\"\n>  '\n>  \n>  test_done\n"},{"id":"120958","messageId":"4A89A5B8.9040405@drmicha.warpmail.net","threadId":"20595","inReplyTo":"7veirejhqq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-config: Parse config files leniently","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-08-17T18:47:20Z","receivedAt":"2009-08-17T18:47:20Z","isPatch":true,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 14.08.2009 21:52:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>> Currently, git config dies as soon as there is a parsing error. This is\n>> especially unfortunate in case a user tries to correct config mistakes\n>> using git config -e.\n>>\n>> Instead, issue a warning only and treat the rest of the line as a\n>> comment (ignore it). This benefits not only git config -e users.\n> \n> ... a broken sentence in the middle?  I would have expected the \"not only\"\n> followed by \"but also\"; the question is \"but also what?\"\n\nI don't see any broken sentence here. \"Benefit\" is a verb as well as a noun.\n\n\"not only git config -e users\" but also everyone else: Unparseable lines\nare ignored, the rest is parsed.\n\n> \n> Hopefully the benefit is not that it now allows all the other commands to\n> cause unspecified types of damage to the repository by following iffy\n> settings obtained from a broken configuration file.\n\nThe only possible problem that I see is when a section heading is not\nparsed because of a forgotten \"[\", say, and the following settings are\nput in the wrong section because of that.\n\n>> Reported-by: David Reitter <david.reitter@gmail.com>\n>> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>\n> \n>> Test had to be adjusted as well.\n> \n> The change to the test demonstrates the issue rather well.  The check()\n> shell function does not check the exit value from \"git config --get\", but\n> in a real script that cares to check and stop on error, this change will\n> now let the script go on, leaving the breakage unnoticed.  I suspect\n> command implemented in C, that call git_config(), will also have the same\n> issue, and I cannot convince myself this is a good change in general,\n> outside the scope of helping \"git config -e\".\n> \n> But I may be being overly cautious.\n\nMy first version had the lenient mode for \"git config -e\" only, which\nrequired a new global int (or, alternatively, changing all callers).\n\nOne could issue a warning and return -1 rather than success. I'm afraid\ngit_config() callers don't check return values but rely on git_config()\ndieing in case there are parsing problems.\n\n> \n> By the way, why did you have to change s/echo/printf/?  Can't you give two\n> lines in a single argument without \"\\n\" escape?\n\nBecause \"printf\" is more portable then \"echo -e\". At least I hope so ;)\n[ One could use a \"here document\", of course. Is that preferable? ]\n\nMichael\n"},{"id":"120969","messageId":"7vvdkmte4p.fsf@alter.siamese.dyndns.org","threadId":"20595","inReplyTo":"4A89A5B8.9040405@drmicha.warpmail.net","subject":"Re: [PATCH] git-config: Parse config files leniently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-17T19:49:10Z","receivedAt":"2009-08-17T19:49:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n\n> Junio C Hamano venit, vidit, dixit 14.08.2009 21:52:\n>> Michael J Gruber <git@drmicha.warpmail.net> writes:\n>> ...\n>> But I may be being overly cautious.\n>\n> My first version had the lenient mode for \"git config -e\" only, which\n> required a new global int (or, alternatively, changing all callers).\n\nWithout looking at the actual patch, that \"single global that is only used\nby builtin_config() when using -e\" sounds safer.\n\nBut still I think I may be being overly cautious.\n\n>> By the way, why did you have to change s/echo/printf/?  Can't you give two\n>> lines in a single argument without \"\\n\" escape?\n>\n> Because \"printf\" is more portable then \"echo -e\". At least I hope so ;)\n> [ One could use a \"here document\", of course. Is that preferable? ]\n\nWhat I meant was to give literally two lines, like this:\n\n    check section.key 'warning: bad config file line 2 in .git/config\nwarning: bad config file line 2 in .git/config'\n\nI do not see a need for a here-doc.\n\nThe rest is tangent you can ignore.\n\n>>> Instead, issue a warning only and treat the rest of the line as a\n>>> comment (ignore it). This benefits not only git config -e users.\n>> \n>> ... a broken sentence in the middle?  I would have expected the \"not only\"\n>> followed by \"but also\"; the question is \"but also what?\"\n>\n> I don't see any broken sentence here. \"Benefit\" is a verb as well as a noun.\n\nYes, and I think you read me correctly.\n\ns/broken/chopped in the middle, missing 'but also X'/;\n\nand you just explained that you meant \"everyone else\" by that X in the\nmissing part of the sentence.\n"},{"id":"122331","messageId":"f29b5893b7022f53d380504fe201303acd9ee3da.1251922441.git.git@drmicha.warpmail.net","threadId":"20595","inReplyTo":"7vvdkmte4p.fsf@alter.siamese.dyndns.org","subject":"[PATCHv2] git-config: Parse config files leniently","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-09-02T20:17:10Z","receivedAt":"2009-09-02T20:17:10Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Currently, git config dies as soon as there is a parsing error. This is\nespecially unfortunate in case a user tries to correct config mistakes\nusing git config -e.\n\nInstead, issue a warning only and treat the rest of the line as a\ncomment (ignore it). This benefits not only git config -e users but\nalso everyone else.\n\nSigned-off-by: Michael J Gruber <git@drmicha.warpmail.net>\nReported-by: David Reitter <david.reitter@gmail.com>\n---\n config.c                |   80 ++++++++++++++++++++++++----------------------\n t/t1303-wacky-config.sh |    3 +-\n 2 files changed, 44 insertions(+), 39 deletions(-)\n\nSo, after a business trip, vacation, ... I'm finally returning to this patch.\nI addressed the echo/printf issue as suggested and clarified the commit message.\n\nRegarding the global int for switching on/off lenient parsing: I reinstated my\nv0 of the patch only to find out (again) that setup_git_directory_gently is the\nproblem here. We would have to turn on lenient parsing before we even know that\n\"-e\" is supplied. So, the only option would be to have \"git config\" use lenient\nparsing in all modes (edit/get/set) and have other git commands die fatally on\nerroneous config.\n\nSo, here's v2 which has lenient config parsing for everyone because I don't see\na way to have it for \"git config -e\" only. If you prefer to have it for all of\n\"git config\" only, that version is in another branch head now...\n\nCheers,\nMichael\n\ndiff --git a/config.c b/config.c\nindex e87edea..5e0af5d 100644\n--- a/config.c\n+++ b/config.c\n@@ -207,50 +207,54 @@ static int git_parse_file(config_fn_t fn, void *data)\n \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n \tconst unsigned char *bomptr = utf8_bom;\n \n-\tfor (;;) {\n-\t\tint c = get_next_char();\n-\t\tif (bomptr && *bomptr) {\n-\t\t\t/* We are at the file beginning; skip UTF8-encoded BOM\n-\t\t\t * if present. Sane editors won't put this in on their\n-\t\t\t * own, but e.g. Windows Notepad will do it happily. */\n-\t\t\tif ((unsigned char) c == *bomptr) {\n-\t\t\t\tbomptr++;\n+\twhile (!config_file_eof) {\n+\t\tfor (;;) {\n+\t\t\tint c = get_next_char();\n+\t\t\tif (bomptr && *bomptr) {\n+\t\t\t\t/* We are at the file beginning; skip UTF8-encoded BOM\n+\t\t\t\t * if present. Sane editors won't put this in on their\n+\t\t\t\t * own, but e.g. Windows Notepad will do it happily. */\n+\t\t\t\tif ((unsigned char) c == *bomptr) {\n+\t\t\t\t\tbomptr++;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t} else {\n+\t\t\t\t\t/* Do not tolerate partial BOM. */\n+\t\t\t\t\tif (bomptr != utf8_bom)\n+\t\t\t\t\t\tbreak;\n+\t\t\t\t\t/* No BOM at file beginning. Cool. */\n+\t\t\t\t\tbomptr = NULL;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif (c == '\\n') {\n+\t\t\t\tif (config_file_eof)\n+\t\t\t\t\treturn 0;\n+\t\t\t\tcomment = 0;\n \t\t\t\tcontinue;\n-\t\t\t} else {\n-\t\t\t\t/* Do not tolerate partial BOM. */\n-\t\t\t\tif (bomptr != utf8_bom)\n+\t\t\t}\n+\t\t\tif (comment || isspace(c))\n+\t\t\t\tcontinue;\n+\t\t\tif (c == '#' || c == ';') {\n+\t\t\t\tcomment = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (c == '[') {\n+\t\t\t\tbaselen = get_base_var(var);\n+\t\t\t\tif (baselen <= 0)\n \t\t\t\t\tbreak;\n-\t\t\t\t/* No BOM at file beginning. Cool. */\n-\t\t\t\tbomptr = NULL;\n+\t\t\t\tvar[baselen++] = '.';\n+\t\t\t\tvar[baselen] = 0;\n+\t\t\t\tcontinue;\n \t\t\t}\n-\t\t}\n-\t\tif (c == '\\n') {\n-\t\t\tif (config_file_eof)\n-\t\t\t\treturn 0;\n-\t\t\tcomment = 0;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (comment || isspace(c))\n-\t\t\tcontinue;\n-\t\tif (c == '#' || c == ';') {\n-\t\t\tcomment = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (c == '[') {\n-\t\t\tbaselen = get_base_var(var);\n-\t\t\tif (baselen <= 0)\n+\t\t\tif (!isalpha(c))\n+\t\t\t\tbreak;\n+\t\t\tvar[baselen] = tolower(c);\n+\t\t\tif (get_value(fn, data, var, baselen+1) < 0)\n \t\t\t\tbreak;\n-\t\t\tvar[baselen++] = '.';\n-\t\t\tvar[baselen] = 0;\n-\t\t\tcontinue;\n \t\t}\n-\t\tif (!isalpha(c))\n-\t\t\tbreak;\n-\t\tvar[baselen] = tolower(c);\n-\t\tif (get_value(fn, data, var, baselen+1) < 0)\n-\t\t\tbreak;\n+\t\twarning(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\t\tcomment = 1;\n \t}\n-\tdie(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\treturn -1;\n }\n \n static int parse_unit_factor(const char *end, unsigned long *val)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 080117c..0599d9f 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -44,7 +44,8 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n test_expect_success 'do not crash on special long config line' '\n \tsetup &&\n \tgit config section.key \"$LONG_VALUE\" &&\n-\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+\tcheck section.key \"warning: bad config file line 2 in .git/config\n+warning: bad config file line 2 in .git/config\"\n '\n \n test_done\n-- \n1.6.4.2.395.ge3d52\n"},{"id":"122343","messageId":"7vab1cfr6s.fsf@alter.siamese.dyndns.org","threadId":"20595","inReplyTo":"f29b5893b7022f53d380504fe201303acd9ee3da.1251922441.git.git@drmicha.warpmail.net","subject":"Re: [PATCHv2] git-config: Parse config files leniently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-03T07:00:43Z","receivedAt":"2009-09-03T07:00:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n> Currently, git config dies as soon as there is a parsing error. This is\n> especially unfortunate in case a user tries to correct config mistakes\n> using git config -e.\n>\n> Instead, issue a warning only and treat the rest of the line as a\n> comment (ignore it). This benefits not only git config -e users but\n> also everyone else.\n\nThis changes the behaviour enough to break t3200-branch.sh, test #52.\n\nThe test stuffs an invalid (but not syntactically incorrect) value used by\n\"git branch\" in the configuration and tries to make sure that \"git branch\"\ndiagnoses the breakage, but it does not fail anymore with your patch.\n\nThere are probably other breakages as well (e.g. t5304-prune.sh, test #5)\nbut if you trace \"git branch\" under the debugger in the trash directory\nleft after running t3200 with -i, it should be pretty obvious that your\npatch is utterly bogus.  get_value() can return negative result after\ndiagnosing a semantic problem with the value, and that is different from a\nsyntax error that you would try to recover and continue, pretending you\ncan ignore the remainder of the line as if it is a comment.\n\nWhy was I CC'ed, if the patch wasn't even self tested?\n"},{"id":"122344","messageId":"4A9F733D.5050205@drmicha.warpmail.net","threadId":"20595","inReplyTo":"7vab1cfr6s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] git-config: Parse config files leniently","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-09-03T07:41:49Z","receivedAt":"2009-09-03T07:41:49Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 03.09.2009 09:00:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>> Currently, git config dies as soon as there is a parsing error. This is\n>> especially unfortunate in case a user tries to correct config mistakes\n>> using git config -e.\n>>\n>> Instead, issue a warning only and treat the rest of the line as a\n>> comment (ignore it). This benefits not only git config -e users but\n>> also everyone else.\n> \n> This changes the behaviour enough to break t3200-branch.sh, test #52.\n> \n> The test stuffs an invalid (but not syntactically incorrect) value used by\n> \"git branch\" in the configuration and tries to make sure that \"git branch\"\n> diagnoses the breakage, but it does not fail anymore with your patch.\n> \n> There are probably other breakages as well (e.g. t5304-prune.sh, test #5)\n> but if you trace \"git branch\" under the debugger in the trash directory\n> left after running t3200 with -i, it should be pretty obvious that your\n> patch is utterly bogus.  get_value() can return negative result after\n> diagnosing a semantic problem with the value, and that is different from a\n> syntax error that you would try to recover and continue, pretending you\n> can ignore the remainder of the line as if it is a comment.\n> \n> Why was I CC'ed, if the patch wasn't even self tested?\n\nBecause\n- not CC'ing you would have meant culling you from the existing CC,\n- we've discussed v1 of this patch before,\n- I asked in this patch (v2) whether to go for an alternative.\n\nSince \"git config -e\" for broken config is not my itch at all, but the\nreporter's, I'll stop my efforts after this response.\n\nMichael\n"},{"id":"122373","messageId":"7viqfz4mu3.fsf@alter.siamese.dyndns.org","threadId":"20595","inReplyTo":"4A9F733D.5050205@drmicha.warpmail.net","subject":"Re: [PATCHv2] git-config: Parse config files leniently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-03T23:42:28Z","receivedAt":"2009-09-03T23:42:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n>> Why was I CC'ed, if the patch wasn't even self tested?\n>\n> Because\n> - not CC'ing you would have meant culling you from the existing CC,\n> - we've discussed v1 of this patch before,\n> - I asked in this patch (v2) whether to go for an alternative.\n\nOh, so you did not mean this was for inclusion but as another round\nof RFC.  I misread your intent.  Sorry about that.\n\nThe eralier analysis of the cause of the breakage indicated that the\nimplementation in this patch was flawed.  What it essentially did was to\nre-define all die() to warn() in the codepaths around configuration\nvariable handling [*1*].\n\nHowever, it does not mean that the idea of \"ignoring syntax errors while\nkeeping other errors still noticed for all commands, not limited to config\nnor not limited to 'config -e'\" is necessarily flawed.\n\nFor example, the test I noticed the breakage with was stuffing an invalid\nvalue to branch.autosetuprebase and wanted to see \"git branch\" fail.\n\nObviously we do want to fail a \"git branch newbranch origin/master\" when\nthe value given to branch.autosetuprebase is misspelled, to avoid creating\nbogus settings the user did not intend to.  We do care about semantic\nerrors (i.e. this variable can take only one of these values, but the\nvalue given in the file is bogus) in such a case.  But if you are running\n\"git branch\" only to view, but not to create, there is no reason for us to\ncare about the branch.autosetuprebase variable [*2*].\n\nThis observation suggests that it may make sense to make the error\nhandling _even looser_ than what you intended to do in your patch (which I\nassume was \"we ignore syntax errors and try to recover, pretending that we\nsaw a comment until the end of line, but we still keep the validation of\nvalues assigned to variables that the existing commands rely on\").\n\nIdeally, the rule would be \"we care about the values of variables we are\ngoing to use, but allow misspelled values in variables that are not used\nby us---we have no business complaining about them.\"  Unfortunately that\nis much harder to arrange with the current code structure, but under that\nrule, \"config -e\" would care only about \"core.editor\" and nothing else, so\nas long as that variable can be sanely read, it should be able to start.\n\n\n[Footnotes]\n\n*1* The additional test that came with the patch only checked for the\npositive case (i.e. \"does the system with this patch treats errors looser\nthan before?\"), but failed to check the negative case (i.e. \"does the\nchange too much and stop catching errors that should be caught?\"), which\nunfortunately is a common mistake that easily lets bugs go unnoticed.\n\n*2* Worse yet, the parsing of branch.autosetuprebase is part of the\ndefault_config and commands that do not have anything to do with new\nbranch creation will fail with the current setup.\n"},{"id":"122392","messageId":"4AA0BE30.1030408@drmicha.warpmail.net","threadId":"20595","inReplyTo":"7viqfz4mu3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv2] git-config: Parse config files leniently","fromName":"Michael J Gruber","fromEmail":"git@drmicha.warpmail.net","sentAt":"2009-09-04T07:13:52Z","receivedAt":"2009-09-04T07:13:52Z","isPatch":false,"sender":{"key":"git@grubix.eu","avatar":"https://avatars.githubusercontent.com/u/233215?v=4"},"body":"Junio C Hamano venit, vidit, dixit 04.09.2009 01:42:\n> Michael J Gruber <git@drmicha.warpmail.net> writes:\n> \n>>> Why was I CC'ed, if the patch wasn't even self tested?\n>>\n>> Because\n>> - not CC'ing you would have meant culling you from the existing CC,\n>> - we've discussed v1 of this patch before,\n>> - I asked in this patch (v2) whether to go for an alternative.\n> \n> Oh, so you did not mean this was for inclusion but as another round\n> of RFC.  I misread your intent.  Sorry about that.\n\nOK. I'll try to distinguish better between RFC and RFI in the future.\nAnd, yes, usually I run *all* tests before sending patches. I had even\ngrepped all tests for \"config\" in order not to miss any, but failed to\nnote how the patch affects all other commands as well.\n\n> \n> The eralier analysis of the cause of the breakage indicated that the\n> implementation in this patch was flawed.  What it essentially did was to\n> re-define all die() to warn() in the codepaths around configuration\n> variable handling [*1*].\n> \n> However, it does not mean that the idea of \"ignoring syntax errors while\n> keeping other errors still noticed for all commands, not limited to config\n> nor not limited to 'config -e'\" is necessarily flawed.\n> \n> For example, the test I noticed the breakage with was stuffing an invalid\n> value to branch.autosetuprebase and wanted to see \"git branch\" fail.\n> \n> Obviously we do want to fail a \"git branch newbranch origin/master\" when\n> the value given to branch.autosetuprebase is misspelled, to avoid creating\n> bogus settings the user did not intend to.  We do care about semantic\n> errors (i.e. this variable can take only one of these values, but the\n> value given in the file is bogus) in such a case.  But if you are running\n> \"git branch\" only to view, but not to create, there is no reason for us to\n> care about the branch.autosetuprebase variable [*2*].\n> \n> This observation suggests that it may make sense to make the error\n> handling _even looser_ than what you intended to do in your patch (which I\n> assume was \"we ignore syntax errors and try to recover, pretending that we\n> saw a comment until the end of line, but we still keep the validation of\n> values assigned to variables that the existing commands rely on\").\n> \n> Ideally, the rule would be \"we care about the values of variables we are\n> going to use, but allow misspelled values in variables that are not used\n> by us---we have no business complaining about them.\"  Unfortunately that\n> is much harder to arrange with the current code structure, but under that\n> rule, \"config -e\" would care only about \"core.editor\" and nothing else, so\n> as long as that variable can be sanely read, it should be able to start.\n> \n> \n> [Footnotes]\n> \n> *1* The additional test that came with the patch only checked for the\n> positive case (i.e. \"does the system with this patch treats errors looser\n> than before?\"), but failed to check the negative case (i.e. \"does the\n> change too much and stop catching errors that should be caught?\"), which\n> unfortunately is a common mistake that easily lets bugs go unnoticed.\n> \n> *2* Worse yet, the parsing of branch.autosetuprebase is part of the\n> default_config and commands that do not have anything to do with new\n> branch creation will fail with the current setup.\n\nThanks for the thorough explanations. Especially *2* makes me think that\nquite some restructuring would be necessary in order to \"do it right\".\nThat would be above my head (and time constraints).\n\nGiven that, I think the practical options are\n\na) make \"git config\" parse leniently (both -e and others)\nb) leave as is and document how to recover from bad config\nc) launch the editor on a tmp copy, check and refuse or loop on fail...\n\nI still think of \"git config -e\" as a shortcut only (meaning it doesn't\nwarrant large specific code efforts), and it's problematic because the\nuser base is split in two sets:\n\n- Those who know their way around .git/config and editors.\n- Those who should stick with the get and set modes of \"git config\".\n\n\"git config -e\" helps users from the 2nd group shoot themselves in their\nfeet badly enough that they can recover only with insight from the first\ngroup...\n\nMichael\n"},{"id":"122399","messageId":"7v8wgvyuf1.fsf@alter.siamese.dyndns.org","threadId":"20595","inReplyTo":"4AA0BE30.1030408@drmicha.warpmail.net","subject":"Re: [PATCHv2] git-config: Parse config files leniently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-04T08:40:34Z","receivedAt":"2009-09-04T08:40:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael J Gruber <git@drmicha.warpmail.net> writes:\n\n>> *2* Worse yet, the parsing of branch.autosetuprebase is part of the\n>> default_config and commands that do not have anything to do with new\n>> branch creation will fail with the current setup.\n>\n> Thanks for the thorough explanations. Especially *2* makes me think that\n> quite some restructuring would be necessary in order to \"do it right\".\n\nWe need to distinguish at least the \"syntax\" and \"semantics\" errors, i.e.\nnot necessarily \"do it right\", but \"how your patch should have looked\nlike\".\n\nI think a slightly hacky but practical workaround then becomes possible.\nWe can have a config callback function to parse only core.editor (and\nperhaps some other very minimum set of variables needed to launch the\neditor on the right config file) extremely loosely, i.e. not even calling\ngit_config_string() to complain about error-nonbool.  You can use the\ncallback _only_ from the ACTION_EDIT codepath in builtin-config.c (which\ncurrently uses git_default_config and errors out when some uninteresting\nconfiguration variables have semantic errors).\n\nThat would get rid of the issue that the configuration mechanism triggers\nsemantic errors for unrelated variables, and would give us a usable\nrecovery editor, no?\n"}]}