{"thread":{"id":"27595","subject":"[RFC/PATCH] config.c: Make git_config() work correctly when called recursively","startedAt":"2011-06-09T17:45:28Z","lastAt":"2011-06-16T19:55:57Z","messageCount":5,"participants":["Ramsay Jones","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"169798","messageId":"4DF106B8.2080902@ramsay1.demon.co.uk","threadId":"27595","inReplyTo":null,"subject":"[RFC/PATCH] config.c: Make git_config() work correctly when called recursively","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-09T17:45:28Z","receivedAt":"2011-06-09T17:45:28Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nOn Cygwin, this fixes a test failure in t3301-notes.sh (test 98,\n\"git notes copy --for-rewrite (disabled)\").\n\nThe test failure is caused by a recursive call to git_config() which\nhas the effect of skipping to the end-of-file while processing the\n\"notes.rewriteref\" config variable. Thus, any config variables that\nappear after \"notes.rewriteref\" are simply ignored by git_config().\n\nThe recursive call to git_config() is due to the \"schizophrenic stat\"\nfunctions on cygwin, and is arrived at as follows:\n\n  cmd_notes() : builtin/notes.c:1057\n    =>copy() @1084: builtin/notes.c:605\n      =>notes_copy_from_stdin() @630: builtin/notes.c:418\n        =>init_copy_notes_for_rewrite() @426: builtin/notes.c:359\n          =>git_config() @384: config.c:876\n\n  *cb=>notes_rewite_config() : builtin/notes.c:329\n    =>string_list_add_refs_by_glob() @348 : notes.c:936\n      =>for_each_glob_ref() @939: refs.c:815\n        =>for_each_glob_ref_in() @817: refs.c:785\n          =>for_each_ref() @809: refs.c:729\n            =>do_for_each_ref() @731: refs.c:658\n              =>get_loose_refs() @663: refs.c:359\n                =>get_ref_dir() @368: refs.c:258\n                  =>stat() @299: compat/cygwin.h:8\n\n  stat macro => indirect call: *cygwin_stat_fn : compat/cygwin.c:141\n    =>cygwin_stat_stub() : compat/cygwin.c:131\n      =>init_stat() @133: compat/cygwin.c:115\n        =>git_config() @118: config.c:876\n\n[The line numbers are from a somewhat recent version of git, but are\njust quoted as a guide; some of them have probably already changed]\n\nSigned-off-by: Ramsay Jones <ramsay@ramsay1.demon.co.uk>\n---\n\nWhile rebasing my 'cygwin-tests' branch recently, this commit conflicted\nwith e96c19c5 (config: support values longer than 1023 bytes, 10-04-2011),\ndue to the replacement of the value char array with a strbuf.\n\nI have not sent this patch to the list before, since I had planned to find\na different solution first, so this (updated patch) is more by way of an\nextended bug-report! Having said that, it works fine ...\n\n[I had also intended to check if a similar change for the key (currently\nlimited to MAXNAME) would be a good idea; my initial reaction was no since\nwe have control of the key names, with only a few exceptions eg. some keys\ncontain branch names ...]\n\nATB,\nRamsay Jones\n\n config.c |   80 ++++++++++++++++++++++++++++++++++++++-----------------------\n 1 files changed, 50 insertions(+), 30 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex e0b3b80..1fc063b 100644\n--- a/config.c\n+++ b/config.c\n@@ -12,10 +12,18 @@\n \n #define MAXNAME (256)\n \n-static FILE *config_file;\n-static const char *config_file_name;\n-static int config_linenr;\n-static int config_file_eof;\n+typedef struct config_file {\n+\tstruct config_file *prev;\n+\tFILE *f;\n+\tconst char *name;\n+\tint linenr;\n+\tint eof;\n+\tstruct strbuf value;\n+\tchar var[MAXNAME];\n+} config_file;\n+\n+static config_file *cf;\n+\n static int zlib_compression_seen;\n \n const char *config_exclusive_filename = NULL;\n@@ -99,7 +107,7 @@ static int get_next_char(void)\n \tFILE *f;\n \n \tc = '\\n';\n-\tif ((f = config_file) != NULL) {\n+\tif (cf && ((f = cf->f) != NULL)) {\n \t\tc = fgetc(f);\n \t\tif (c == '\\r') {\n \t\t\t/* DOS like systems */\n@@ -110,9 +118,9 @@ static int get_next_char(void)\n \t\t\t}\n \t\t}\n \t\tif (c == '\\n')\n-\t\t\tconfig_linenr++;\n+\t\t\tcf->linenr++;\n \t\tif (c == EOF) {\n-\t\t\tconfig_file_eof = 1;\n+\t\t\tcf->eof = 1;\n \t\t\tc = '\\n';\n \t\t}\n \t}\n@@ -121,21 +129,20 @@ static int get_next_char(void)\n \n static char *parse_value(void)\n {\n-\tstatic struct strbuf value = STRBUF_INIT;\n \tint quote = 0, comment = 0, space = 0;\n \n-\tstrbuf_reset(&value);\n+\tstrbuf_reset(&cf->value);\n \tfor (;;) {\n \t\tint c = get_next_char();\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\n \t\t\t\treturn NULL;\n-\t\t\treturn value.buf;\n+\t\t\treturn cf->value.buf;\n \t\t}\n \t\tif (comment)\n \t\t\tcontinue;\n \t\tif (isspace(c) && !quote) {\n-\t\t\tif (value.len)\n+\t\t\tif (cf->value.len)\n \t\t\t\tspace++;\n \t\t\tcontinue;\n \t\t}\n@@ -146,7 +153,7 @@ static char *parse_value(void)\n \t\t\t}\n \t\t}\n \t\tfor (; space; space--)\n-\t\t\tstrbuf_addch(&value, ' ');\n+\t\t\tstrbuf_addch(&cf->value, ' ');\n \t\tif (c == '\\\\') {\n \t\t\tc = get_next_char();\n \t\t\tswitch (c) {\n@@ -168,14 +175,14 @@ static char *parse_value(void)\n \t\t\tdefault:\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tstrbuf_addch(&value, c);\n+\t\t\tstrbuf_addch(&cf->value, c);\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '\"') {\n \t\t\tquote = 1-quote;\n \t\t\tcontinue;\n \t\t}\n-\t\tstrbuf_addch(&value, c);\n+\t\tstrbuf_addch(&cf->value, c);\n \t}\n }\n \n@@ -192,7 +199,7 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t/* Get the full name */\n \tfor (;;) {\n \t\tc = get_next_char();\n-\t\tif (config_file_eof)\n+\t\tif (cf->eof)\n \t\t\tbreak;\n \t\tif (!iskeychar(c))\n \t\t\tbreak;\n@@ -256,7 +263,7 @@ static int get_base_var(char *name)\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (config_file_eof)\n+\t\tif (cf->eof)\n \t\t\treturn -1;\n \t\tif (c == ']')\n \t\t\treturn baselen;\n@@ -274,7 +281,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n {\n \tint comment = 0;\n \tint baselen = 0;\n-\tstatic char var[MAXNAME];\n+\tchar *var = cf->var;\n \n \t/* U+FEFF Byte Order Mark in UTF8 */\n \tstatic const unsigned char *utf8_bom = (unsigned char *) \"\\xef\\xbb\\xbf\";\n@@ -298,7 +305,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\t\t}\n \t\t}\n \t\tif (c == '\\n') {\n-\t\t\tif (config_file_eof)\n+\t\t\tif (cf->eof)\n \t\t\t\treturn 0;\n \t\t\tcomment = 0;\n \t\t\tcontinue;\n@@ -323,7 +330,7 @@ static int git_parse_file(config_fn_t fn, void *data)\n \t\tif (get_value(fn, data, var, baselen+1) < 0)\n \t\t\tbreak;\n \t}\n-\tdie(\"bad config file line %d in %s\", config_linenr, config_file_name);\n+\tdie(\"bad config file line %d in %s\", cf->linenr, cf->name);\n }\n \n static int parse_unit_factor(const char *end, unsigned long *val)\n@@ -374,8 +381,8 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \n static void die_bad_config(const char *name)\n {\n-\tif (config_file_name)\n-\t\tdie(\"bad config value for '%s' in %s\", name, config_file_name);\n+\tif (cf && cf->name)\n+\t\tdie(\"bad config value for '%s' in %s\", name, cf->name);\n \tdie(\"bad config value for '%s'\", name);\n }\n \n@@ -795,13 +802,24 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \n \tret = -1;\n \tif (f) {\n-\t\tconfig_file = f;\n-\t\tconfig_file_name = filename;\n-\t\tconfig_linenr = 1;\n-\t\tconfig_file_eof = 0;\n+\t\tconfig_file top;\n+\n+\t\t/* push config-file parsing state stack */\n+\t\ttop.prev = cf;\n+\t\ttop.f = f;\n+\t\ttop.name = filename;\n+\t\ttop.linenr = 1;\n+\t\ttop.eof = 0;\n+\t\tstrbuf_init(&top.value, 1024);\n+\t\tcf = &top;\n+\n \t\tret = git_parse_file(fn, data);\n+\n+\t\t/* pop config-file parsing state stack */\n+\t\tstrbuf_release(&top.value);\n+\t\tcf = top.prev;\n+\n \t\tfclose(f);\n-\t\tconfig_file_name = NULL;\n \t}\n \treturn ret;\n }\n@@ -909,6 +927,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n {\n \tconst char *ep;\n \tsize_t section_len;\n+\tFILE *f = cf->f;\n \n \tswitch (store.state) {\n \tcase KEY_SEEN:\n@@ -920,7 +939,7 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t\t\treturn 1;\n \t\t\t}\n \n-\t\t\tstore.offset[store.seen] = ftell(config_file);\n+\t\t\tstore.offset[store.seen] = ftell(f);\n \t\t\tstore.seen++;\n \t\t}\n \t\tbreak;\n@@ -947,19 +966,19 @@ static int store_aux(const char *key, const char *value, void *cb)\n \t\t * Do not increment matches: this is no match, but we\n \t\t * just made sure we are in the desired section.\n \t\t */\n-\t\tstore.offset[store.seen] = ftell(config_file);\n+\t\tstore.offset[store.seen] = ftell(f);\n \t\t/* fallthru */\n \tcase SECTION_END_SEEN:\n \tcase START:\n \t\tif (matches(key, value)) {\n-\t\t\tstore.offset[store.seen] = ftell(config_file);\n+\t\t\tstore.offset[store.seen] = ftell(f);\n \t\t\tstore.state = KEY_SEEN;\n \t\t\tstore.seen++;\n \t\t} else {\n \t\t\tif (strrchr(key, '.') - key == store.baselen &&\n \t\t\t      !strncmp(key, store.key, store.baselen)) {\n \t\t\t\t\tstore.state = SECTION_SEEN;\n-\t\t\t\t\tstore.offset[store.seen] = ftell(config_file);\n+\t\t\t\t\tstore.offset[store.seen] = ftell(f);\n \t\t\t}\n \t\t}\n \t}\n@@ -1415,6 +1434,7 @@ int git_config_rename_section(const char *old_name, const char *new_name)\n \tstruct lock_file *lock = xcalloc(sizeof(struct lock_file), 1);\n \tint out_fd;\n \tchar buf[1024];\n+\tFILE *config_file;\n \n \tif (config_exclusive_filename)\n \t\tconfig_filename = xstrdup(config_exclusive_filename);\n-- \n1.7.5\n"},{"id":"169812","messageId":"20110609203958.GA4671@sigill.intra.peff.net","threadId":"27595","inReplyTo":"4DF106B8.2080902@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH] config.c: Make git_config() work correctly when called recursively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-09T20:39:58Z","receivedAt":"2011-06-09T20:39:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 09, 2011 at 06:45:28PM +0100, Ramsay Jones wrote:\n\n> The recursive call to git_config() is due to the \"schizophrenic stat\"\n> functions on cygwin, and is arrived at as follows:\n> \n>   cmd_notes() : builtin/notes.c:1057\n>     =>copy() @1084: builtin/notes.c:605\n>       =>notes_copy_from_stdin() @630: builtin/notes.c:418\n>         =>init_copy_notes_for_rewrite() @426: builtin/notes.c:359\n>           =>git_config() @384: config.c:876\n> \n>   *cb=>notes_rewite_config() : builtin/notes.c:329\n>     =>string_list_add_refs_by_glob() @348 : notes.c:936\n>       =>for_each_glob_ref() @939: refs.c:815\n>         =>for_each_glob_ref_in() @817: refs.c:785\n>           =>for_each_ref() @809: refs.c:729\n>             =>do_for_each_ref() @731: refs.c:658\n>               =>get_loose_refs() @663: refs.c:359\n>                 =>get_ref_dir() @368: refs.c:258\n>                   =>stat() @299: compat/cygwin.h:8\n> \n>   stat macro => indirect call: *cygwin_stat_fn : compat/cygwin.c:141\n>     =>cygwin_stat_stub() : compat/cygwin.c:131\n>       =>init_stat() @133: compat/cygwin.c:115\n>         =>git_config() @118: config.c:876\n\nWow, that's quite a call-chain.\n\n> I have not sent this patch to the list before, since I had planned to find\n> a different solution first, so this (updated patch) is more by way of an\n> extended bug-report! Having said that, it works fine ...\n\nI think it's actually a pretty sane approach. Your other option would be\nnot lazily configuring cygwin stat, which means putting an extra very\nearly stat call somewhere.\n\nBut look at how deep the call chain above is. And consider how the bug\nmanifested itself: silently ignoring some config. So I wouldn't be\nterribly surprised if there is another such recursion hiding somewhere,\nor if we manage to introduce one accidentally in the future.\n\nSo making git_config safe for recursion seems like a good solution for\nfuture maintainability.\n\n>  config.c |   80 ++++++++++++++++++++++++++++++++++++++-----------------------\n\nThe patch looks sane from my quick skim, though:\n\n> -static FILE *config_file;\n> -static const char *config_file_name;\n> -static int config_linenr;\n> -static int config_file_eof;\n> +typedef struct config_file {\n> +\tstruct config_file *prev;\n> +\tFILE *f;\n> +\tconst char *name;\n> +\tint linenr;\n> +\tint eof;\n> +\tstruct strbuf value;\n> +\tchar var[MAXNAME];\n> +} config_file;\n> +\n> +static config_file *cf;\n\nSince you've nicely encapsulated all of the global data in this struct,\nmaybe it is worth simply passing a struct pointer down the call-chain\ninstead of relying on a global pointer. Then you can let the language do\nits job and stop managing the stack yourself.\n\n-Peff\n"},{"id":"170076","messageId":"4DF7A627.2080600@ramsay1.demon.co.uk","threadId":"27595","inReplyTo":"20110609203958.GA4671@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] config.c: Make git_config() work correctly when called recursively","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-14T18:19:19Z","receivedAt":"2011-06-14T18:19:19Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"[Sorry for the late reply - I've been away from email for several days...]\n\nJeff King wrote:\n> On Thu, Jun 09, 2011 at 06:45:28PM +0100, Ramsay Jones wrote:\n> \n>> The recursive call to git_config() is due to the \"schizophrenic stat\"\n>> functions on cygwin, and is arrived at as follows:\n\n[...]\n\n> Wow, that's quite a call-chain.\n\n:-P\n\n>> I have not sent this patch to the list before, since I had planned to find\n>> a different solution first, so this (updated patch) is more by way of an\n>> extended bug-report! Having said that, it works fine ...\n> \n> I think it's actually a pretty sane approach. Your other option would be\n> not lazily configuring cygwin stat, which means putting an extra very\n> early stat call somewhere.\n> \n> But look at how deep the call chain above is. And consider how the bug\n> manifested itself: silently ignoring some config. So I wouldn't be\n> terribly surprised if there is another such recursion hiding somewhere,\n> or if we manage to introduce one accidentally in the future.\n> \n> So making git_config safe for recursion seems like a good solution for\n> future maintainability.\n\nIndeed, that is exactly why this was my first (and so far only) solution\nto this problem; it is simple, safe and resilient in the face of future\npatches! [I'm not going to commit to auditing all of the current calls to\ngit_config(), checking all subsequent call-chains for calls to [l]stat(),\nlet alone all future changes to git.]\n\nHowever, I anticipated some resistance to this approach, so I have intended\nto find another solution (for about 4 months now!), in order to present the\nlist with some alternatives. I would be quite happy not to bother ;-)\n\n[Actually, one of my preferred solutions is to revert commit adbc0b6 et. seq.,\nbut that is a whole different discussion!]\n\n> The patch looks sane from my quick skim, though:\n> \n>> -static FILE *config_file;\n>> -static const char *config_file_name;\n>> -static int config_linenr;\n>> -static int config_file_eof;\n>> +typedef struct config_file {\n>> +\tstruct config_file *prev;\n>> +\tFILE *f;\n>> +\tconst char *name;\n>> +\tint linenr;\n>> +\tint eof;\n>> +\tstruct strbuf value;\n>> +\tchar var[MAXNAME];\n>> +} config_file;\n>> +\n>> +static config_file *cf;\n> \n> Since you've nicely encapsulated all of the global data in this struct,\n> maybe it is worth simply passing a struct pointer down the call-chain\n> instead of relying on a global pointer. Then you can let the language do\n> its job and stop managing the stack yourself.\n\nThe first version of this patch did exactly that! However, that first patch\nfailed the test-suite. :(\n\nThe failure was caused by a change to the die_bad_config() function, which\nin the current patch looks like this:\n\n    @@ -374,8 +381,8 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n \n     static void die_bad_config(const char *name)\n     {\n    -\tif (config_file_name)\n    -\t\tdie(\"bad config value for '%s' in %s\", name, config_file_name);\n    +\tif (cf && cf->name)\n    +\t\tdie(\"bad config value for '%s' in %s\", name, cf->name);\n     \tdie(\"bad config value for '%s'\", name);\n     }\n\nIn order not to change the public interface (note that git_config_int() and\ngit_config_ulong() call die_bad_config()), I had to change this function so\nthat it only contains the final die() call. Thus, the error message no longer\nincluded the config filename. This caused the test called 'invalid unit' in\nt1300-repo-config.sh to fail.\n\nI could, of course, have simply changed the expect file so that it would pass\nthe test, but I wanted the change to be self-contained and to pass all existing\ntests (ie. the external interface/behaviour should *not* change).\n\nSo, I ended up with this (even simpler) patch.\n\nThank you for your comments on the patch.\n\nATB,\nRamsay Jones\n"},{"id":"170020","messageId":"20110614182726.GA451@sigill.intra.peff.net","threadId":"27595","inReplyTo":"4DF7A627.2080600@ramsay1.demon.co.uk","subject":"Re: [RFC/PATCH] config.c: Make git_config() work correctly when called recursively","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-06-14T18:27:26Z","receivedAt":"2011-06-14T18:27:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 14, 2011 at 07:19:19PM +0100, Ramsay Jones wrote:\n\n> > Since you've nicely encapsulated all of the global data in this struct,\n> > maybe it is worth simply passing a struct pointer down the call-chain\n> > instead of relying on a global pointer. Then you can let the language do\n> > its job and stop managing the stack yourself.\n> \n> The first version of this patch did exactly that! However, that first patch\n> failed the test-suite. :(\n> \n> The failure was caused by a change to the die_bad_config() function, which\n> in the current patch looks like this:\n> \n>     @@ -374,8 +381,8 @@ int git_parse_ulong(const char *value, unsigned long *ret)\n>  \n>      static void die_bad_config(const char *name)\n>      {\n>     -\tif (config_file_name)\n>     -\t\tdie(\"bad config value for '%s' in %s\", name, config_file_name);\n>     +\tif (cf && cf->name)\n>     +\t\tdie(\"bad config value for '%s' in %s\", name, cf->name);\n>      \tdie(\"bad config value for '%s'\", name);\n>      }\n\nAh, right. We want to keep it as globals, because we end up in a\ncallback which does not get the context passed to it.\n\n> In order not to change the public interface (note that git_config_int() and\n> git_config_ulong() call die_bad_config()), I had to change this function so\n> that it only contains the final die() call. Thus, the error message no longer\n> included the config filename. This caused the test called 'invalid unit' in\n> t1300-repo-config.sh to fail.\n> \n> I could, of course, have simply changed the expect file so that it would pass\n> the test, but I wanted the change to be self-contained and to pass all existing\n> tests (ie. the external interface/behaviour should *not* change).\n\nNo, you did the right thing here. The information on which config file\nwe're in is valuable, and taking away the globals is not worth the pain\nof making all of the callers and callbacks of git_config have to deal\nwith passing around a context struct.\n\nSo the patch you posted looks good to me.\n\n-Peff\n"},{"id":"170157","messageId":"4DFA5FCD.6080902@ramsay1.demon.co.uk","threadId":"27595","inReplyTo":"20110614182726.GA451@sigill.intra.peff.net","subject":"Re: [RFC/PATCH] config.c: Make git_config() work correctly when called recursively","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2011-06-16T19:55:57Z","receivedAt":"2011-06-16T19:55:57Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Jeff King wrote:\n> On Tue, Jun 14, 2011 at 07:19:19PM +0100, Ramsay Jones wrote:\n[...]\n>> I could, of course, have simply changed the expect file so that it would pass\n>> the test, but I wanted the change to be self-contained and to pass all existing\n>> tests (ie. the external interface/behaviour should *not* change).\n> \n> No, you did the right thing here. The information on which config file\n> we're in is valuable, and taking away the globals is not worth the pain\n> of making all of the callers and callbacks of git_config have to deal\n> with passing around a context struct.\n> \n> So the patch you posted looks good to me.\n\nThanks! (I will abandon the search for an alternate solution! ;-)\n\nI think the commit message needs to be re-worded before actually submitting\nthe patch (it's fine for an RFC, but the call-chain info has a limited shelf\nlife and should go).\n\nAlso, I wanted to re-submit two additional patches in that branch; last time\nthey didn't even make it to pu!  Hopefully they will take at least one step\nfurther this time.\n\nATB,\nRamsay Jones\n"}]}