{"thread":{"id":"32615","subject":"[PATCH] archive-tar: fix sanity check in config parsing","startedAt":"2013-01-13T17:42:01Z","lastAt":"2013-01-23T20:51:23Z","messageCount":31,"participants":["René Scharfe","Jeff King","Joachim Schmitz","Jonathan Nieder","Junio C Hamano","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"206704","messageId":"50F2F1E9.1040700@lsrfire.ath.cx","threadId":"32615","inReplyTo":null,"subject":"[PATCH] archive-tar: fix sanity check in config parsing","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2013-01-13T17:42:01Z","receivedAt":"2013-01-13T17:42:01Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"git archive supports passing generated tar archives through filter\ncommands like gzip. Additional filters can be set up using the\nconfiguration variables tar..command and tar..remote.\n\nWhen parsing these config variable names, we currently check that\nthe second dot is found nine characters into the name, disallowing\nfilter names with a length of five characters. Additionally,\ngit archive crashes when the second dot is omitted:\n\n\t$ ./git -c tar.foo=bar archive HEAD >/dev/null\n\tfatal: Data too large to fit into virtual memory space.\n\nInstead we should check if the second dot exists at all, or if\nwe only found the first one.\n\nSigned-off-by: Rene Scharfe \n---\n archive-tar.c | 2 +-\n t/t5000-tar-tree.sh | 3 ++-\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex d1cce46..093d10e 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -335,7 +335,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n \tif (prefixcmp(var, \"tar.\"))\n \t\treturn 0;\n \tdot = strrchr(var, '.');\n-\tif (dot == var + 9)\n+\tif (dot == var + 3)\n \t\treturn 0;\n \n \tname = var + 4;\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex e7c240f..3fbd366 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -212,7 +212,8 @@ test_expect_success 'git-archive --prefix=olde-' '\n test_expect_success 'setup tar filters' '\n \tgit config tar.tar.foo.command \"tr ab ba\" &&\n \tgit config tar.bar.command \"tr ab ba\" &&\n-\tgit config tar.bar.remote true\n+\tgit config tar.bar.remote true &&\n+\tgit config tar.invalid baz\n '\n \n test_expect_success 'archive --list mentions user filter' '\n-- \n1.8.0\n"},{"id":"206710","messageId":"20130113200044.GA3979@sigill.intra.peff.net","threadId":"32615","inReplyTo":"50F2F1E9.1040700@lsrfire.ath.cx","subject":"Re: [PATCH] archive-tar: fix sanity check in config parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-13T20:00:49Z","receivedAt":"2013-01-13T20:00:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:\n\n> When parsing these config variable names, we currently check that\n> the second dot is found nine characters into the name, disallowing\n> filter names with a length of five characters. Additionally,\n> git archive crashes when the second dot is omitted:\n> \n> \t$ ./git -c tar.foo=bar archive HEAD >/dev/null\n> \tfatal: Data too large to fit into virtual memory space.\n> \n> Instead we should check if the second dot exists at all, or if\n> we only found the first one.\n\nEek. Thanks for finding it. Your fix is obviously correct.\n\n> --- a/archive-tar.c\n> +++ b/archive-tar.c\n> @@ -335,7 +335,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n> \tif (prefixcmp(var, \"tar.\"))\n> \t\treturn 0;\n> \tdot = strrchr(var, '.');\n> -\tif (dot == var + 9)\n> +\tif (dot == var + 3)\n> \t\treturn 0;\n\nFor the curious, the original version of the patch[1] read:\n\n+ if (prefixcmp(var, \"tarfilter.\"))\n+ return 0;\n+ dot = strrchr(var, '.');\n+ if (dot == var + 9)\n+ return 0;\n\nand when I shortened the config section to \"tar\" in a re-roll of the\nseries, I missed the corresponding change to the offset.\n\n-Peff\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/175785/focus=175858\n"},{"id":"206783","messageId":"kd0evl$ac0$1@ger.gmane.org","threadId":"32615","inReplyTo":"20130113200044.GA3979@sigill.intra.peff.net","subject":"Re: [PATCH] archive-tar: fix sanity check in config parsing","fromName":"Joachim Schmitz","fromEmail":"jojo@schmitz-digital.de","sentAt":"2013-01-14T08:17:57Z","receivedAt":"2013-01-14T08:17:57Z","isPatch":true,"sender":{"key":"jojo@schmitz-digital.de","avatar":"https://avatars.githubusercontent.com/u/1786669?v=4"},"body":"Jeff King wrote:\n> On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:\n>\n>> When parsing these config variable names, we currently check that\n>> the second dot is found nine characters into the name, disallowing\n>> filter names with a length of five characters. Additionally,\n>> git archive crashes when the second dot is omitted:\n>>\n>> $ ./git -c tar.foo=bar archive HEAD >/dev/null\n>> fatal: Data too large to fit into virtual memory space.\n>>\n>> Instead we should check if the second dot exists at all, or if\n>> we only found the first one.\n>\n> Eek. Thanks for finding it. Your fix is obviously correct.\n>\n>> --- a/archive-tar.c\n>> +++ b/archive-tar.c\n>> @@ -335,7 +335,7 @@ static int tar_filter_config(const char *var,\n>> const char *value, void *data) if (prefixcmp(var, \"tar.\"))\n>> return 0;\n>> dot = strrchr(var, '.');\n>> - if (dot == var + 9)\n>> + if (dot == var + 3)\n>> return 0;\n>\n> For the curious, the original version of the patch[1] read:\n>\n> + if (prefixcmp(var, \"tarfilter.\"))\n> + return 0;\n> + dot = strrchr(var, '.');\n> + if (dot == var + 9)\n> + return 0;\n>\n> and when I shortened the config section to \"tar\" in a re-roll of the\n> series, I missed the corresponding change to the offset.\n\nWouldn't it then be better ti use strlen(\"tar\") rather than a 3? Or at least \na comment?\n\nBye, Jojo \n"},{"id":"206795","messageId":"20130114124424.GA14129@sigill.intra.peff.net","threadId":"32615","inReplyTo":"kd0evl$ac0$1@ger.gmane.org","subject":"Re: [PATCH] archive-tar: fix sanity check in config parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T12:44:24Z","receivedAt":"2013-01-14T12:44:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 09:17:57AM +0100, Joachim Schmitz wrote:\n\n> >For the curious, the original version of the patch[1] read:\n> >\n> >+ if (prefixcmp(var, \"tarfilter.\"))\n> >+ return 0;\n> >+ dot = strrchr(var, '.');\n> >+ if (dot == var + 9)\n> >+ return 0;\n> >\n> >and when I shortened the config section to \"tar\" in a re-roll of the\n> >series, I missed the corresponding change to the offset.\n> \n> Wouldn't it then be better ti use strlen(\"tar\") rather than a 3? Or\n> at least a comment?\n\nThen you are relying on the two strings being the same, rather than the\nstring and the length being the same. If you wanted to DRY it up, it\nwould look like:\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex d1cce46..a7c0690 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -332,15 +332,17 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n \tconst char *type;\n \tint namelen;\n \n-\tif (prefixcmp(var, \"tar.\"))\n+#define SECTION \"tar\"\n+\tif (prefixcmp(var, SECTION \".\"))\n \t\treturn 0;\n \tdot = strrchr(var, '.');\n-\tif (dot == var + 9)\n+\tif (dot == var + strlen(SECTION))\n \t\treturn 0;\n \n-\tname = var + 4;\n+\tname = var + strlen(SECTION) + 1;\n \tnamelen = dot - name;\n \ttype = dot + 1;\n+#undef SECTION\n \n \tar = find_tar_filter(name, namelen);\n \tif (!ar) {\n\n\n(of course there are other variants where you do not use a macro, but\nthen you need to manually check for the \".\" after the prefixcmp call).\nI dunno. It is technically more robust in that the offsets are computed,\nbut I think it is a little harder to read. Of course, I wrote the\noriginal so I am probably not a good judge.\n\nWe could also potentially encapsulate it in a function. I think the diff\ncode has a very similar block.\n\n-Peff\n"},{"id":"206799","messageId":"20130114145845.GA16497@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114124424.GA14129@sigill.intra.peff.net","subject":"Re: [PATCH] archive-tar: fix sanity check in config parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T14:58:45Z","receivedAt":"2013-01-14T14:58:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 04:44:24AM -0800, Jeff King wrote:\n\n> > Wouldn't it then be better ti use strlen(\"tar\") rather than a 3? Or\n> > at least a comment?\n> [...]\n> We could also potentially encapsulate it in a function. I think the diff\n> code has a very similar block.\n\nHere's a series that does that, with a few other cleanups on top. The\ndiffstat actually ends up a few lines longer, but that is mostly because\nof comments and function declarations. More importantly, though, the\ncall-sites are much easier to read.\n\nHaving written this series, though, I can't help but wonder if the world\nwould be a better place if config_fn_t looked more like:\n\n typedef int (*config_fn_t)(const char *full_var,\n const char *section,\n const char *subsection,\n const char *key,\n const char *value,\n void *data);\n\nIt's just as easy for the config parser to do this ahead of time, and by\nhanding off real C-strings (instead of ending up with a ptr/len pair for\nthe subsection), it makes the lives of the callbacks much easier (e.g.,\nthe final patch below contorts a bit to use string_list with the\nsubsection).\n\nI can look into that, but here is the less invasive cleanup:\n\n [1/6]: config: add helper function for parsing key names\n [2/6]: archive-tar: use match_config_key when parsing config\n [3/6]: convert some config callbacks to match_config_key\n [4/6]: userdiff: drop parse_driver function\n [5/6]: submodule: use match_config_key when parsing config\n [6/6]: submodule: simplify memory handling in config parsing\n\n-Peff\n"},{"id":"206800","messageId":"20130114150012.GA16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 1/6] config: add helper function for parsing key names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:00:13Z","receivedAt":"2013-01-14T15:00:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config callback functions get keys of the general form:\n\n section.subsection.key\n\n(where the subsection may be contain arbitrary data, or may\nbe missing). For matching keys without subsections, it is\nsimple enough to call \"strcmp\". Matching keys with\nsubsections is a little more complicated, and each callback\ndoes it in an ad-hoc way, usually involving error-prone\npointer arithmetic.\n\nLet's provide a helper that keeps the pointer arithmetic all\nin one place.\n\nSigned-off-by: Jeff King \n---\nNo users yet; they come in future patches.\n\n cache.h | 15 +++++++++++++++\n config.c | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..14003b8 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n+/*\n+ * Match and parse a config key of the form:\n+ *\n+ * section.(subsection.)?key\n+ *\n+ * (i.e., what gets handed to a config_fn_t). The caller provides the section;\n+ * we return -1 if it does not match, 0 otherwise. The subsection and key\n+ * out-parameters are filled by the function (and subsection is NULL if it is\n+ * missing).\n+ */\n+extern int match_config_key(const char *var,\n+\t\t const char *section,\n+\t\t const char **subsection, int *subsection_len,\n+\t\t const char **key);\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config.c b/config.c\nindex 7b444b6..18d9c0e 100644\n--- a/config.c\n+++ b/config.c\n@@ -1667,3 +1667,36 @@ int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\n }\n+\n+int match_config_key(const char *var,\n+\t\t const char *section,\n+\t\t const char **subsection, int *subsection_len,\n+\t\t const char **key)\n+{\n+\tint section_len = strlen(section);\n+\tconst char *dot;\n+\n+\t/* Does it start with \"section.\" ? */\n+\tif (prefixcmp(var, section) || var[section_len] != '.')\n+\t\treturn -1;\n+\n+\t/*\n+\t * Find the key; we don't know yet if we have a subsection, but we must\n+\t * parse backwards from the end, since the subsection may have dots in\n+\t * it, too.\n+\t */\n+\tdot = strrchr(var, '.');\n+\t*key = dot + 1;\n+\n+\t/* Did we have a subsection at all? */\n+\tif (dot == var + section_len) {\n+\t\t*subsection = NULL;\n+\t\t*subsection_len = 0;\n+\t}\n+\telse {\n+\t\t*subsection = var + section_len + 1;\n+\t\t*subsection_len = dot - *subsection;\n+\t}\n+\n+\treturn 0;\n+}\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206801","messageId":"20130114150256.GB16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 2/6] archive-tar: use match_config_key when parsing config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:02:56Z","receivedAt":"2013-01-14T15:02:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is fewer lines of code, but more importantly, fixes a\nbogus pointer offset. We are looking for \"tar.\" in the\nsection, but later assume that the dot we found is at offset\n9, not 3. This is a holdover from an earlier iteration of\n767cf45 which called the section \"tarfilter\".\n\nAs a result, we could erroneously reject some filters with\ndots in their name, as well as read uninitialized memory.\n\nReported by (and test by) René Scharfe.\n\nSigned-off-by: Jeff King \n---\nThis obviously replaces René's patch. It might make more sense to just\nput his patch onto maint, and build this on top; then this patch can get\nsquashed into the next one (which just updates the uninteresting\ncallsites).\n\n archive-tar.c | 10 +---------\n t/t5000-tar-tree.sh | 3 ++-\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex 0ba3f25..68dbe59 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -325,20 +325,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n static int tar_filter_config(const char *var, const char *value, void *data)\n {\n \tstruct archiver *ar;\n-\tconst char *dot;\n \tconst char *name;\n \tconst char *type;\n \tint namelen;\n \n-\tif (prefixcmp(var, \"tar.\"))\n+\tif (match_config_key(var, \"tar\", &name, &namelen, &type) < 0 || !name)\n \t\treturn 0;\n-\tdot = strrchr(var, '.');\n-\tif (dot == var + 9)\n-\t\treturn 0;\n-\n-\tname = var + 4;\n-\tnamelen = dot - name;\n-\ttype = dot + 1;\n \n \tar = find_tar_filter(name, namelen);\n \tif (!ar) {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex ecf00ed..517ae04 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -283,7 +283,8 @@ test_expect_success 'setup tar filters' '\n test_expect_success 'setup tar filters' '\n \tgit config tar.tar.foo.command \"tr ab ba\" &&\n \tgit config tar.bar.command \"tr ab ba\" &&\n-\tgit config tar.bar.remote true\n+\tgit config tar.bar.remote true &&\n+\tgit config tar.invalid baz\n '\n \n test_expect_success 'archive --list mentions user filter' '\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206802","messageId":"20130114150322.GC16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 3/6] convert some config callbacks to match_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:03:22Z","receivedAt":"2013-01-14T15:03:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is easier to read and avoids magic offset constants\nwhich need to be in sync with the section-name we provide.\n\nSigned-off-by: Jeff King \n---\n convert.c | 6 +-----\n ll-merge.c | 6 +-----\n userdiff.c | 13 +++----------\n 3 files changed, 5 insertions(+), 20 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 6602155..e3ecb30 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \t * External conversion drivers are configured using\n \t * \"filter..variable\".\n \t */\n-\tif (prefixcmp(var, \"filter.\") || (ep = strrchr(var, '.')) == var + 6)\n+\tif (match_config_key(var, \"filter\", &name, &namelen, &ep) < 0 || !name)\n \t\treturn 0;\n-\tname = var + 7;\n-\tnamelen = ep - name;\n \tfor (drv = user_convert; drv; drv = drv->next)\n \t\tif (!strncmp(drv->name, name, namelen) && !drv->name[namelen])\n \t\t\tbreak;\n@@ -479,8 +477,6 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \t\tuser_convert_tail = &(drv->next);\n \t}\n \n-\tep++;\n-\n \t/*\n \t * filter..smudge and filter..clean specifies\n \t * the command line:\ndiff --git a/ll-merge.c b/ll-merge.c\nindex acea33b..d4c4ff6 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -236,15 +236,13 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t * especially, we do not want to look at variables such as\n \t * \"merge.summary\", \"merge.tool\", and \"merge.verbosity\".\n \t */\n-\tif (prefixcmp(var, \"merge.\") || (ep = strrchr(var, '.')) == var + 5)\n+\tif (match_config_key(var, \"merge\", &name, &namelen, &ep) < 0 || !name)\n \t\treturn 0;\n \n \t/*\n \t * Find existing one as we might be processing merge..var2\n \t * after seeing merge..var1.\n \t */\n-\tname = var + 6;\n-\tnamelen = ep - name;\n \tfor (fn = ll_user_merge; fn; fn = fn->next)\n \t\tif (!strncmp(fn->name, name, namelen) && !fn->name[namelen])\n \t\t\tbreak;\n@@ -256,8 +254,6 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t\tll_user_merge_tail = &(fn->next);\n \t}\n \n-\tep++;\n-\n \tif (!strcmp(\"name\", ep)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\ndiff --git a/userdiff.c b/userdiff.c\nindex ed958ef..1a6a0fa 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,\n \t\tconst char *value, const char *type)\n {\n \tstruct userdiff_driver *drv;\n-\tconst char *dot;\n-\tconst char *name;\n+\tconst char *name, *key;\n \tint namelen;\n \n-\tif (prefixcmp(var, \"diff.\"))\n-\t\treturn NULL;\n-\tdot = strrchr(var, '.');\n-\tif (dot == var + 4)\n-\t\treturn NULL;\n-\tif (strcmp(type, dot+1))\n+\tif (match_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n+\t strcmp(type, key))\n \t\treturn NULL;\n \n-\tname = var + 5;\n-\tnamelen = dot - name;\n \tdrv = userdiff_find_by_namelen(name, namelen);\n \tif (!drv) {\n \t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206803","messageId":"20130114150414.GD16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 4/6] userdiff: drop parse_driver function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:04:14Z","receivedAt":"2013-01-14T15:04:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse userdiff config, we generally assume that\n\n diff.name.key\n\nwill affect the \"key\" value of the \"name\" driver. However,\nwithout checking the key, we conflict with the ancient\n\"diff.color.*\" namespace. The current code is careful not to\neven create a driver struct if we do not see a key that is\nknown by the diff-driver code.\n\nHowever, this carefulness is unnecessary; the default driver\nwith no keys set behaves exactly the same as having no\ndriver at all. We can simply set up the driver struct as\nsoon as we see we have a config key that looks like a\ndriver. This makes the code a bit more readable.\n\nSigned-off-by: Jeff King \n---\nThis is not strictly related to the series, but I noticed it as a\ncleanup while doing the previous patch.\n\n userdiff.c | 50 +++++++++++++++++++++-----------------------------\n 1 file changed, 21 insertions(+), 29 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex 1a6a0fa..c6cdec4 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -184,28 +184,6 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n \treturn NULL;\n }\n \n-static struct userdiff_driver *parse_driver(const char *var,\n-\t\tconst char *value, const char *type)\n-{\n-\tstruct userdiff_driver *drv;\n-\tconst char *name, *key;\n-\tint namelen;\n-\n-\tif (match_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n-\t strcmp(type, key))\n-\t\treturn NULL;\n-\n-\tdrv = userdiff_find_by_namelen(name, namelen);\n-\tif (!drv) {\n-\t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n-\t\tdrv = &drivers[ndrivers++];\n-\t\tmemset(drv, 0, sizeof(*drv));\n-\t\tdrv->name = xmemdupz(name, namelen);\n-\t\tdrv->binary = -1;\n-\t}\n-\treturn drv;\n-}\n-\n static int parse_funcname(struct userdiff_funcname *f, const char *k,\n \t\tconst char *v, int cflags)\n {\n@@ -233,20 +211,34 @@ int userdiff_config(const char *k, const char *v)\n int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n+\tconst char *name, *type;\n+\tint namelen;\n+\n+\tif (match_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n+\t\treturn 0;\n+\n+\tdrv = userdiff_find_by_namelen(name, namelen);\n+\tif (!drv) {\n+\t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n+\t\tdrv = &drivers[ndrivers++];\n+\t\tmemset(drv, 0, sizeof(*drv));\n+\t\tdrv->name = xmemdupz(name, namelen);\n+\t\tdrv->binary = -1;\n+\t}\n \n-\tif ((drv = parse_driver(k, v, \"funcname\")))\n+\tif (!strcmp(type, \"funcname\"))\n \t\treturn parse_funcname(&drv->funcname, k, v, 0);\n-\tif ((drv = parse_driver(k, v, \"xfuncname\")))\n+\tif (!strcmp(type, \"xfuncname\"))\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n-\tif ((drv = parse_driver(k, v, \"binary\")))\n+\tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n-\tif ((drv = parse_driver(k, v, \"command\")))\n+\tif (!strcmp(type, \"command\"))\n \t\treturn git_config_string(&drv->external, k, v);\n-\tif ((drv = parse_driver(k, v, \"textconv\")))\n+\tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n-\tif ((drv = parse_driver(k, v, \"cachetextconv\")))\n+\tif (!strcmp(type, \"cachetextconv\"))\n \t\treturn parse_bool(&drv->textconv_want_cache, k, v);\n-\tif ((drv = parse_driver(k, v, \"wordregex\")))\n+\tif (!strcmp(type, \"wordregex\"))\n \t\treturn git_config_string(&drv->word_regex, k, v);\n \n \treturn 0;\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206804","messageId":"20130114150438.GE16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 5/6] submodule: use match_config_key when parsing config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:04:38Z","receivedAt":"2013-01-14T15:04:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This makes the code a lot simpler to read by dropping a\nwhole bunch of constant offsets.\n\nAs a bonus, it means we also feed the whole config variable\nname to our error functions:\n\n [before]\n $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout\n fatal: bad foo.fetchrecursesubmodules argument: bogus\n\n [after]\n fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus\n\nSigned-off-by: Jeff King \n---\n submodule.c | 20 +++++++++++---------\n 1 file changed, 11 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2f55436..4361207 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -126,15 +126,17 @@ int parse_submodule_config_option(const char *var, const char *value)\n \n int parse_submodule_config_option(const char *var, const char *value)\n {\n-\tint len;\n \tstruct string_list_item *config;\n \tstruct strbuf submodname = STRBUF_INIT;\n+\tconst char *name, *key;\n+\tint namelen;\n \n-\tvar += 10;\t\t/* Skip \"submodule.\" */\n+\tif (match_config_key(var, \"submodule\", &name, &namelen, &key) < 0 ||\n+\t !name)\n+\t\treturn 0;\n \n-\tlen = strlen(var);\n-\tif ((len > 5) && !strcmp(var + len - 5, \".path\")) {\n-\t\tstrbuf_add(&submodname, var, len - 5);\n+\tif (!strcmp(key, \"path\")) {\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n \t\tif (config)\n \t\t\tfree(config->util);\n@@ -142,22 +144,22 @@ int parse_submodule_config_option(const char *var, const char *value)\n \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n \t\tconfig->util = strbuf_detach(&submodname, NULL);\n \t\tstrbuf_release(&submodname);\n-\t} else if ((len > 23) && !strcmp(var + len - 23, \".fetchrecursesubmodules\")) {\n-\t\tstrbuf_add(&submodname, var, len - 23);\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n \t\tif (!config)\n \t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n \t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n \t\tstrbuf_release(&submodname);\n-\t} else if ((len > 7) && !strcmp(var + len - 7, \".ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n \t\t\treturn 0;\n \t\t}\n \n-\t\tstrbuf_add(&submodname, var, len - 7);\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n \t\tif (config)\n \t\t\tfree(config->util);\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206805","messageId":"20130114150741.GF16828@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114145845.GA16497@sigill.intra.peff.net","subject":"[PATCH 6/6] submodule: simplify memory handling in config parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T15:07:41Z","receivedAt":"2013-01-14T15:07:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a strbuf for the name of the submodule, even though\nwe only ever add one buffer (which we know the length of) to\nit. Let's just use xmemdupz instead, which is slightly more\nefficient and makes it easier to follow what is going on.\n\nUnfortunately, we still end up having to deal with some\nmemory ownership issues in some code branches, as we have to\nallocate the string in order to do a string list lookup, and\nthen only sometimes want to hand ownership of that string\nover to the string_list. Still, making that explicit in the\ncode (as opposed to sometimes detaching the strbuf, and then\nalways releasing it) makes it a little more obvious what is\ngoing on.\n\nSigned-off-by: Jeff King \n---\nI'm undecided on this one. I think the result is easier to follow, but\nothers might find the original easier. This would be a lot more\nreadable if the config parser gave us a broken-out representation with\nreal C strings. Then we could pass the subsection straight to the\nstring_list functions for lookup, and using the \"dup\" flag of string\nlist, avoid even having to deal with memory duplication at all.\n\n submodule.c | 30 ++++++++++++++----------------\n 1 file changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4361207..23a8490 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)\n int parse_submodule_config_option(const char *var, const char *value)\n {\n \tstruct string_list_item *config;\n-\tstruct strbuf submodname = STRBUF_INIT;\n \tconst char *name, *key;\n \tint namelen;\n \n@@ -136,37 +135,36 @@ int parse_submodule_config_option(const char *var, const char *value)\n \t\treturn 0;\n \n \tif (!strcmp(key, \"path\")) {\n-\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n \t\tif (config)\n \t\t\tfree(config->util);\n \t\telse\n \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n-\t\tconfig->util = strbuf_detach(&submodname, NULL);\n-\t\tstrbuf_release(&submodname);\n+\t\tconfig->util = xmemdupz(name, namelen);\n \t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n-\t\tstrbuf_add(&submodname, name, namelen);\n-\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n+\t\tchar *name_cstr = xmemdupz(name, namelen);\n+\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);\n \t\tif (!config)\n-\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n+\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);\n+\t\telse\n+\t\t\tfree(name_cstr);\n \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n-\t\tstrbuf_release(&submodname);\n \t} else if (!strcmp(key, \"ignore\")) {\n+\t\tchar *name_cstr;\n+\n \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n \t\t\treturn 0;\n \t\t}\n \n-\t\tstrbuf_add(&submodname, name, namelen);\n-\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n-\t\tif (config)\n+\t\tname_cstr = xmemdupz(name, namelen);\n+\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);\n+\t\tif (config) {\n \t\t\tfree(config->util);\n-\t\telse\n-\t\t\tconfig = string_list_append(&config_ignore_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n-\t\tstrbuf_release(&submodname);\n+\t\t\tfree(name_cstr);\n+\t\t} else\n+\t\t\tconfig = string_list_append(&config_ignore_for_name, name_cstr);\n \t\tconfig->util = xstrdup(value);\n \t\treturn 0;\n \t}\n-- \n1.8.1.rc1.10.g7d71f7b\n"},{"id":"206809","messageId":"20130114165527.GB3121@elie.Belkin","threadId":"32615","inReplyTo":"20130114150322.GC16828@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] convert some config callbacks to match_config_key","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-14T16:55:28Z","receivedAt":"2013-01-14T16:55:28Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> --- a/convert.c\n> +++ b/convert.c\n> @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n> \t * External conversion drivers are configured using\n> \t * \"filter..variable\".\n> \t */\n> -\tif (prefixcmp(var, \"filter.\") || (ep = strrchr(var, '.')) == var + 6)\n> +\tif (match_config_key(var, \"filter\", &name, &namelen, &ep) < 0 || !name)\n> \t\treturn 0;\n\nHm, I actually find the preimage more readable here.\n\nI like the idea of having a function to do this, though. Here are a\ncouple of ideas for making the meaning obvious again for people like\nme:\n\nRename match_config_key() to something like parse_config_key()?\nmatch_ makes it sound like its main purpose is to match it against a\npattern, but really it is more about decomposing into constituent\nparts.\n\nRename ep to something like 'key' or 'filtertype'? Without the\nexplicit string processing, it is not obvious what ep is the end of.\n\n[...]\n> --- a/userdiff.c\n> +++ b/userdiff.c\n> @@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,\n> \t\tconst char *value, const char *type)\n> {\n> \tstruct userdiff_driver *drv;\n> -\tconst char *dot;\n> -\tconst char *name;\n> +\tconst char *name, *key;\n> \tint namelen;\n> \n> -\tif (prefixcmp(var, \"diff.\"))\n> -\t\treturn NULL;\n> -\tdot = strrchr(var, '.');\n> -\tif (dot == var + 4)\n> -\t\treturn NULL;\n> -\tif (strcmp(type, dot+1))\n> +\tif (match_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n> +\t strcmp(type, key))\n> \t\treturn NULL;\n> \n> -\tname = var + 5;\n> -\tnamelen = dot - name;\n> \tdrv = userdiff_find_by_namelen(name, namelen);\n\nWhat happens in the !name case? (Honest question --- I haven't checked.)\n\nGenerally I like the cleanup. Thanks for tasteful patch.\n\nJonathan\n"},{"id":"206810","messageId":"20130114170610.GB22098@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114165527.GB3121@elie.Belkin","subject":"Re: [PATCH 3/6] convert some config callbacks to match_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T17:06:10Z","receivedAt":"2013-01-14T17:06:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 08:55:28AM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > --- a/convert.c\n> > +++ b/convert.c\n> > @@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n> > \t * External conversion drivers are configured using\n> > \t * \"filter..variable\".\n> > \t */\n> > -\tif (prefixcmp(var, \"filter.\") || (ep = strrchr(var, '.')) == var + 6)\n> > +\tif (match_config_key(var, \"filter\", &name, &namelen, &ep) < 0 || !name)\n> > \t\treturn 0;\n> \n> Hm, I actually find the preimage more readable here.\n\nThe big thing to me is getting rid of the manual \"6\" there, which is bad\nfor maintainability (it must match the length of \"filter\", and there is\nno compile-time verification).\n\n> Rename match_config_key() to something like parse_config_key()?\n> match_ makes it sound like its main purpose is to match it against a\n> pattern, but really it is more about decomposing into constituent\n> parts.\n\nThere is already a git_config_parse_key, but it does something else. I\nwanted to avoid confusion there. And I was trying to indicate that this\nwas not just about parsing, but also about matching the section.\nBasically I was trying to encapsulate the current technique and have as\nlittle code change as possible. But maybe:\n\n struct config_key k;\n parse_config_key(&k, var);\n if (strcmp(k.section, \"filter\") || k.subsection))\n return 0;\n\nwould be a better start (or having git_config do the first two lines\nitself before triggering the callback).\n\n> Rename ep to something like 'key' or 'filtertype'? Without the\n> explicit string processing, it is not obvious what ep is the end of.\n\nAh, so that is what \"ep\" stands for. I was thinking it is a terrible\nvariable name, but I was trying to keep the patch minimal.\n\n> > -\tif (prefixcmp(var, \"diff.\"))\n> > -\t\treturn NULL;\n> > -\tdot = strrchr(var, '.');\n> > -\tif (dot == var + 4)\n> > -\t\treturn NULL;\n> > -\tif (strcmp(type, dot+1))\n> > +\tif (match_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n> > +\t strcmp(type, key))\n> > \t\treturn NULL;\n> > \n> > -\tname = var + 5;\n> > -\tnamelen = dot - name;\n> > \tdrv = userdiff_find_by_namelen(name, namelen);\n> \n> What happens in the !name case? (Honest question --- I haven't checked.)\n\nSegfault, I expect. Thanks for catching.\n\nI actually wrote this correctly once, coupled with patch 4, but screwed\nit up when teasing it apart into two patches. It should be:\n\n if (match_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n !name ||\n strcmp(type, key))\n return NULL;\n\nPatch 4 replaces this with correct code (as it moves it into\nuserdiff_config).\n\n-Peff\n"},{"id":"206818","messageId":"20130114180550.GA12961@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130114170610.GB22098@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] convert some config callbacks to match_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-14T18:05:51Z","receivedAt":"2013-01-14T18:05:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 09:06:10AM -0800, Jeff King wrote:\n\n> struct config_key k;\n> parse_config_key(&k, var);\n> if (strcmp(k.section, \"filter\") || k.subsection))\n> return 0;\n> \n> would be a better start (or having git_config do the first two lines\n> itself before triggering the callback).\n\nHere's what that looks like, along with the cleanups in submodule.c that\nare made possible by it.\n\nI \"cheat\" a little and use a static buffer when parsing the config key,\nso that the caller does not have to deal with freeing it. It makes using\nthe parser literally as simple as the lines above, but it does mean it\nisn't re-entrant (and worse, it has to be invoked from a config\ncallback, since the static buffer is tied to the config file stack).\n\nNone of that is a problem for the use here, but it is not a\ngenerally-callable function. For that reason, it might make more sense\nto have the config parser just provide the config_key, and not have a\npublic function at all. The downside to that is that we have to update\nthe function signature of all of the config callbacks.\n\n---\n cache.h | 7 ++++++\n config.c | 35 ++++++++++++++++++++++++++++++\n submodule.c | 41 ++++++++++++++---------------------\n 3 files changed, 58 insertions(+), 25 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..df756e6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1119,6 +1119,13 @@ extern int update_server_info(int);\n #define CONFIG_INVALID_PATTERN 6\n #define CONFIG_GENERIC_ERROR 7\n \n+struct config_key {\n+\tconst char *section;\n+\tconst char *subsection;\n+\tconst char *key;\n+};\n+void config_key_parse(struct config_key *, const char *);\n+\n typedef int (*config_fn_t)(const char *, const char *, void *);\n extern int git_default_config(const char *, const char *, void *);\n extern int git_config_from_file(config_fn_t fn, const char *, void *);\ndiff --git a/config.c b/config.c\nindex 7b444b6..7b8df3e 100644\n--- a/config.c\n+++ b/config.c\n@@ -18,6 +18,7 @@ typedef struct config_file {\n \tint eof;\n \tstruct strbuf value;\n \tstruct strbuf var;\n+\tstruct strbuf key_parse_buf;\n } config_file;\n \n static config_file *cf;\n@@ -899,6 +900,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\ttop.eof = 0;\n \t\tstrbuf_init(&top.value, 1024);\n \t\tstrbuf_init(&top.var, 1024);\n+\t\tstrbuf_init(&top.key_parse_buf, 1024);\n \t\tcf = ⊤\n \n \t\tret = git_parse_file(fn, data);\n@@ -906,6 +908,7 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)\n \t\t/* pop config-file parsing state stack */\n \t\tstrbuf_release(&top.value);\n \t\tstrbuf_release(&top.var);\n+\t\tstrbuf_release(&top.key_parse_buf);\n \t\tcf = top.prev;\n \n \t\tfclose(f);\n@@ -1667,3 +1670,35 @@ int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\n }\n+\n+void config_key_parse(struct config_key *key, const char *var)\n+{\n+\t/*\n+\t * We want to use a static buffer so the caller does not have to worry\n+\t * about memory ownership. But since config parsing can happen\n+\t * recursively, we must use storage from the stack of config files.\n+\t */\n+\tstruct strbuf *sb = &cf->key_parse_buf;\n+\tchar *dot;\n+\tchar *rdot;\n+\n+\tstrbuf_reset(sb);\n+\tstrbuf_addstr(sb, var);\n+\n+\tdot = strchr(sb->buf, '.');\n+\trdot = strrchr(sb->buf, '.');\n+\t/* Should never happen because our keys come from git_parse_file. */\n+\tif (!dot)\n+\t\tdie(\"BUG: config_key_parse was fed a bogus key\");\n+\tkey->section = sb->buf;\n+\t*dot = '\\0';\n+\tkey->key = rdot + 1;\n+\n+\tif (rdot == dot)\n+\t\tkey->subsection = NULL;\n+\telse {\n+\t\t*rdot = '\\0';\n+\t\tkey->subsection = dot + 1;\n+\t}\n+\n+}\ndiff --git a/submodule.c b/submodule.c\nindex 2f55436..4894718 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -11,9 +11,9 @@\n #include \"sha1-array.h\"\n #include \"argv-array.h\"\n \n-static struct string_list config_name_for_path;\n-static struct string_list config_fetch_recurse_submodules_for_name;\n-static struct string_list config_ignore_for_name;\n+static struct string_list config_name_for_path = STRING_LIST_INIT_DUP;\n+static struct string_list config_fetch_recurse_submodules_for_name = STRING_LIST_INIT_DUP;\n+static struct string_list config_ignore_for_name = STRING_LIST_INIT_DUP;\n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static struct string_list changed_submodule_paths;\n static int initialized_fetch_ref_tips;\n@@ -126,47 +126,38 @@ int parse_submodule_config_option(const char *var, const char *value)\n \n int parse_submodule_config_option(const char *var, const char *value)\n {\n-\tint len;\n+\tstruct config_key key;\n \tstruct string_list_item *config;\n-\tstruct strbuf submodname = STRBUF_INIT;\n \n-\tvar += 10;\t\t/* Skip \"submodule.\" */\n+\tconfig_key_parse(&key, var);\n+\tif (strcmp(key.section, \"submodule\") || !key.subsection)\n+\t\treturn 0;\n \n-\tlen = strlen(var);\n-\tif ((len > 5) && !strcmp(var + len - 5, \".path\")) {\n-\t\tstrbuf_add(&submodname, var, len - 5);\n+\tif (!strcmp(key.key, \"path\")) {\n \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n \t\tif (config)\n \t\t\tfree(config->util);\n \t\telse\n-\t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n-\t\tconfig->util = strbuf_detach(&submodname, NULL);\n-\t\tstrbuf_release(&submodname);\n-\t} else if ((len > 23) && !strcmp(var + len - 23, \".fetchrecursesubmodules\")) {\n-\t\tstrbuf_add(&submodname, var, len - 23);\n-\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n+\t\t\tconfig = string_list_append(&config_name_for_path, value);\n+\t\tconfig->util = xstrdup(key.subsection);\n+\t} else if (!strcmp(key.key, \"fetchrecursesubmodules\")) {\n+\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, key.subsection);\n \t\tif (!config)\n-\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n+\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name, key.subsection);\n \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n-\t\tstrbuf_release(&submodname);\n-\t} else if ((len > 7) && !strcmp(var + len - 7, \".ignore\")) {\n+\t} else if (!strcmp(key.key, \"ignore\")) {\n \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n \t\t\treturn 0;\n \t\t}\n \n-\t\tstrbuf_add(&submodname, var, len - 7);\n-\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n+\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, key.subsection);\n \t\tif (config)\n \t\t\tfree(config->util);\n \t\telse\n-\t\t\tconfig = string_list_append(&config_ignore_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n-\t\tstrbuf_release(&submodname);\n+\t\t\tconfig = string_list_append(&config_ignore_for_name, key.subsection);\n \t\tconfig->util = xstrdup(value);\n-\t\treturn 0;\n \t}\n \treturn 0;\n }\n"},{"id":"206819","messageId":"7v8v7veixc.fsf@alter.siamese.dyndns.org","threadId":"32615","inReplyTo":"20130114150012.GA16828@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] config: add helper function for parsing key names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-14T18:08:47Z","receivedAt":"2013-01-14T18:08:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The config callback functions get keys of the general form:\n>\n> section.subsection.key\n>\n> (where the subsection may be contain arbitrary data, or may\n> be missing). For matching keys without subsections, it is\n> simple enough to call \"strcmp\". Matching keys with\n> subsections is a little more complicated, and each callback\n> does it in an ad-hoc way, usually involving error-prone\n> pointer arithmetic.\n>\n> Let's provide a helper that keeps the pointer arithmetic all\n> in one place.\n>\n> Signed-off-by: Jeff King \n> ---\n> No users yet; they come in future patches.\n>\n> cache.h | 15 +++++++++++++++\n> config.c | 33 +++++++++++++++++++++++++++++++++\n> 2 files changed, 48 insertions(+)\n>\n> diff --git a/cache.h b/cache.h\n> index c257953..14003b8 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);\n> #define CONFIG_INCLUDE_INIT { 0 }\n> extern int git_config_include(const char *name, const char *value, void *data);\n> \n> +/*\n> + * Match and parse a config key of the form:\n> + *\n> + * section.(subsection.)?key\n> + *\n> + * (i.e., what gets handed to a config_fn_t). The caller provides the section;\n> + * we return -1 if it does not match, 0 otherwise. The subsection and key\n> + * out-parameters are filled by the function (and subsection is NULL if it is\n> + * missing).\n> + */\n> +extern int match_config_key(const char *var,\n> +\t\t const char *section,\n> +\t\t const char **subsection, int *subsection_len,\n> +\t\t const char **key);\n> +\n\nI agree with Jonathan about the naming s/match/parse/.\n\nAfter looking at the callers in your later patches, I think the\ncounted interface to subsection is probably fine. The caller can\ncheck !subsection to see if it is a two- or three- level name, and\n\n if (parse_config_key(var, \"submodule\", &name, &namelen, &key) < 0 ||\n\t!name)\n\treturn 0;\n\nis very easy to follow (that is the result of your 5th step).\n"},{"id":"206943","messageId":"20130115160422.GC21815@sigill.intra.peff.net","threadId":"32615","inReplyTo":"7v8v7veixc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/6] config: add helper function for parsing key names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-15T16:04:22Z","receivedAt":"2013-01-15T16:04:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 14, 2013 at 10:08:47AM -0800, Junio C Hamano wrote:\n\n> > +extern int match_config_key(const char *var,\n> > +\t\t const char *section,\n> > +\t\t const char **subsection, int *subsection_len,\n> > +\t\t const char **key);\n> > +\n> \n> I agree with Jonathan about the naming s/match/parse/.\n\nI see this is marked for re-roll in WC. I'm happy to re-roll it with the\nsuggestions from Jonathan, but before I do, did you have any comment on\nthe \"struct config_key\" alternative I sent as a follow-up?\n\nYou said:\n\n> After looking at the callers in your later patches, I think the\n> counted interface to subsection is probably fine. The caller can\n> check !subsection to see if it is a two- or three- level name, and\n> \n> if (parse_config_key(var, \"submodule\", &name, &namelen, &key) < 0 ||\n> \t!name)\n> \treturn 0;\n> \n> is very easy to follow (that is the result of your 5th step).\n\nbut I wasn't sure if that was \"it is not worth the trouble of the other\none\" or \"I did not yet read the other one\".\n\n-Peff\n"},{"id":"206951","messageId":"7vehhm4bof.fsf@alter.siamese.dyndns.org","threadId":"32615","inReplyTo":"20130115160422.GC21815@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] config: add helper function for parsing key names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-15T17:07:44Z","receivedAt":"2013-01-15T17:07:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... did you have any comment on\n> the \"struct config_key\" alternative I sent as a follow-up?\n\nI did read it but I cannot say I did so very carefully. My gut\nreaction was that the \"take the variable name and section name,\nreturn the subsection name pointer and length, if there is any, and\nthe key\" made it readable enough. The proposed interface to make\nand lend a copy to the caller does make it more readble, but I do\nnot know if that is worth doing. Neutral-to-slightly-in-favor, I\nwould say.\n"},{"id":"207224","messageId":"7va9s6qkkz.fsf@alter.siamese.dyndns.org","threadId":"32615","inReplyTo":"7vehhm4bof.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/6] config: add helper function for parsing key names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-18T20:53:32Z","receivedAt":"2013-01-18T20:53:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> ... did you have any comment on\n>> the \"struct config_key\" alternative I sent as a follow-up?\n>\n> I did read it but I cannot say I did so very carefully. My gut\n> reaction was that the \"take the variable name and section name,\n> return the subsection name pointer and length, if there is any, and\n> the key\" made it readable enough. The proposed interface to make\n> and lend a copy to the caller does make it more readble, but I do\n> not know if that is worth doing. Neutral-to-slightly-in-favor, I\n> would say.\n\nNow I re-read that \"struct config_key\" thing, I would have to say\nthat the idea of giving split and NUL-terminated strings to the\ncallers is good, but the \"cheat\" looks somewhat brittle for all the\nreasons that come from using a static buffer (which you already\nmentioned). As I do not offhand think of a better alternative, I'd\nsay we leave it for another day.\n"},{"id":"207555","messageId":"20130123062132.GA2038@sigill.intra.peff.net","threadId":"32615","inReplyTo":"7va9s6qkkz.fsf@alter.siamese.dyndns.org","subject":"[PATCHv2 0/8] config key-parsing cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:21:32Z","receivedAt":"2013-01-23T06:21:32Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 18, 2013 at 12:53:32PM -0800, Junio C Hamano wrote:\n\n> >> ... did you have any comment on\n> >> the \"struct config_key\" alternative I sent as a follow-up?\n> >\n> > I did read it but I cannot say I did so very carefully. My gut\n> > reaction was that the \"take the variable name and section name,\n> > return the subsection name pointer and length, if there is any, and\n> > the key\" made it readable enough. The proposed interface to make\n> > and lend a copy to the caller does make it more readble, but I do\n> > not know if that is worth doing. Neutral-to-slightly-in-favor, I\n> > would say.\n> \n> Now I re-read that \"struct config_key\" thing, I would have to say\n> that the idea of giving split and NUL-terminated strings to the\n> callers is good, but the \"cheat\" looks somewhat brittle for all the\n> reasons that come from using a static buffer (which you already\n> mentioned). As I do not offhand think of a better alternative, I'd\n> say we leave it for another day.\n\nOK. I had the feeling if the config parser provided it to the caller\nthat more sites could take advantage of it (without adding too many\nlines to call the parsing function). But looking again, there aren't\nthat many sites that would benefit. E.g., git_daemon_config in daemon.c\ncould use it to avoid using a constant offset. But the current code\nthere is not hard to read, and saving a few characters there is not\nworth the complexity.\n\nSo I've re-rolled the original version, taking into account the comments\nfrom you and Jonathan. I also clarified a few of the commit messages,\nand modified two more sites to use the new function.\n\n [1/8]: config: add helper function for parsing key names\n\nSame as before, but now called parse_config_key.\n\n [2/8]: archive-tar: use parse_config_key when parsing config\n\nSame (rebased for new name, of course).\n\n [3/8]: convert some config callbacks to parse_config_key\n\nTweaked confusing \"ep\" variable name. Fixed missing \"!name\" check in\nuserdiff code (which gets removed in the next patch anyway).\n\n [4/8]: userdiff: drop parse_driver function\n [5/8]: submodule: use parse_config_key when parsing config\n [6/8]: submodule: simplify memory handling in config parsing\n\nSame.\n\n [7/8]: help: use parse_config_key for man config\n [8/8]: reflog: use parse_config_key in config callback\n\nTwo new callsites. I split these out because unlike the ones in 3/8,\nthey do not benefit from a reduction in lines of code. However, I think\nthe results are still more readable. You can judge for yourself; drop\nthem if you disagree. Or feel free to squash them into 3/8 if that makes\nmore sense.\n\n-Peff\n"},{"id":"207556","messageId":"20130123062305.GA5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 1/8] config: add helper function for parsing key names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:23:05Z","receivedAt":"2013-01-23T06:23:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The config callback functions get keys of the general form:\n\n section.subsection.key\n\n(where the subsection may be contain arbitrary data, or may\nbe missing). For matching keys without subsections, it is\nsimple enough to call \"strcmp\". Matching keys with\nsubsections is a little more complicated, and each callback\ndoes it in an ad-hoc way, usually involving error-prone\npointer arithmetic.\n\nLet's provide a helper that keeps the pointer arithmetic all\nin one place.\n\nSigned-off-by: Jeff King \n---\n cache.h | 15 +++++++++++++++\n config.c | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 48 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex c257953..b19305b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1164,6 +1164,21 @@ extern int git_config_include(const char *name, const char *value, void *data);\n #define CONFIG_INCLUDE_INIT { 0 }\n extern int git_config_include(const char *name, const char *value, void *data);\n \n+/*\n+ * Match and parse a config key of the form:\n+ *\n+ * section.(subsection.)?key\n+ *\n+ * (i.e., what gets handed to a config_fn_t). The caller provides the section;\n+ * we return -1 if it does not match, 0 otherwise. The subsection and key\n+ * out-parameters are filled by the function (and subsection is NULL if it is\n+ * missing).\n+ */\n+extern int parse_config_key(const char *var,\n+\t\t\t const char *section,\n+\t\t\t const char **subsection, int *subsection_len,\n+\t\t\t const char **key);\n+\n extern int committer_ident_sufficiently_given(void);\n extern int author_ident_sufficiently_given(void);\n \ndiff --git a/config.c b/config.c\nindex 7b444b6..11bd4d8 100644\n--- a/config.c\n+++ b/config.c\n@@ -1667,3 +1667,36 @@ int config_error_nonbool(const char *var)\n {\n \treturn error(\"Missing value for '%s'\", var);\n }\n+\n+int parse_config_key(const char *var,\n+\t\t const char *section,\n+\t\t const char **subsection, int *subsection_len,\n+\t\t const char **key)\n+{\n+\tint section_len = strlen(section);\n+\tconst char *dot;\n+\n+\t/* Does it start with \"section.\" ? */\n+\tif (prefixcmp(var, section) || var[section_len] != '.')\n+\t\treturn -1;\n+\n+\t/*\n+\t * Find the key; we don't know yet if we have a subsection, but we must\n+\t * parse backwards from the end, since the subsection may have dots in\n+\t * it, too.\n+\t */\n+\tdot = strrchr(var, '.');\n+\t*key = dot + 1;\n+\n+\t/* Did we have a subsection at all? */\n+\tif (dot == var + section_len) {\n+\t\t*subsection = NULL;\n+\t\t*subsection_len = 0;\n+\t}\n+\telse {\n+\t\t*subsection = var + section_len + 1;\n+\t\t*subsection_len = dot - *subsection;\n+\t}\n+\n+\treturn 0;\n+}\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207557","messageId":"20130123062327.GB5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 2/8] archive-tar: use parse_config_key when parsing config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:23:27Z","receivedAt":"2013-01-23T06:23:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is fewer lines of code, but more importantly, fixes a\nbogus pointer offset. We are looking for \"tar.\" in the\nsection, but later assume that the dot we found is at offset\n9, not 3. This is a holdover from an earlier iteration of\n767cf45 which called the section \"tarfilter\".\n\nAs a result, we could erroneously reject some filters with\ndots in their name, as well as read uninitialized memory.\n\nReported by (and test by) René Scharfe.\n\nSigned-off-by: Jeff King \n---\n archive-tar.c | 10 +---------\n t/t5000-tar-tree.sh | 3 ++-\n 2 files changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/archive-tar.c b/archive-tar.c\nindex d1cce46..719b629 100644\n--- a/archive-tar.c\n+++ b/archive-tar.c\n@@ -327,20 +327,12 @@ static int tar_filter_config(const char *var, const char *value, void *data)\n static int tar_filter_config(const char *var, const char *value, void *data)\n {\n \tstruct archiver *ar;\n-\tconst char *dot;\n \tconst char *name;\n \tconst char *type;\n \tint namelen;\n \n-\tif (prefixcmp(var, \"tar.\"))\n+\tif (parse_config_key(var, \"tar\", &name, &namelen, &type) < 0 || !name)\n \t\treturn 0;\n-\tdot = strrchr(var, '.');\n-\tif (dot == var + 9)\n-\t\treturn 0;\n-\n-\tname = var + 4;\n-\tnamelen = dot - name;\n-\ttype = dot + 1;\n \n \tar = find_tar_filter(name, namelen);\n \tif (!ar) {\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex e7c240f..3fbd366 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -212,7 +212,8 @@ test_expect_success 'setup tar filters' '\n test_expect_success 'setup tar filters' '\n \tgit config tar.tar.foo.command \"tr ab ba\" &&\n \tgit config tar.bar.command \"tr ab ba\" &&\n-\tgit config tar.bar.remote true\n+\tgit config tar.bar.remote true &&\n+\tgit config tar.invalid baz\n '\n \n test_expect_success 'archive --list mentions user filter' '\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207558","messageId":"20130123062422.GC5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 3/8] convert some config callbacks to parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:24:23Z","receivedAt":"2013-01-23T06:24:23Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These callers can drop some inline pointer arithmetic and\nmagic offset constants, making them more readable and less\nerror-prone (those constants had to match the lengths of\nstrings, but there is no automatic verification of that\nfact).\n\nThe \"ep\" pointer (presumably for \"end pointer\"), which\npoints to the final key segment of the config variable, is\ngiven the more standard name \"key\" to describe its function\nrather than its derivation.\n\nSigned-off-by: Jeff King \n---\n convert.c | 14 +++++---------\n ll-merge.c | 14 +++++---------\n userdiff.c | 13 +++----------\n 3 files changed, 13 insertions(+), 28 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 6602155..3520252 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -457,7 +457,7 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \n static int read_convert_config(const char *var, const char *value, void *cb)\n {\n-\tconst char *ep, *name;\n+\tconst char *key, *name;\n \tint namelen;\n \tstruct convert_driver *drv;\n \n@@ -465,10 +465,8 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \t * External conversion drivers are configured using\n \t * \"filter..variable\".\n \t */\n-\tif (prefixcmp(var, \"filter.\") || (ep = strrchr(var, '.')) == var + 6)\n+\tif (parse_config_key(var, \"filter\", &name, &namelen, &key) < 0 || !name)\n \t\treturn 0;\n-\tname = var + 7;\n-\tnamelen = ep - name;\n \tfor (drv = user_convert; drv; drv = drv->next)\n \t\tif (!strncmp(drv->name, name, namelen) && !drv->name[namelen])\n \t\t\tbreak;\n@@ -479,8 +477,6 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \t\tuser_convert_tail = &(drv->next);\n \t}\n \n-\tep++;\n-\n \t/*\n \t * filter..smudge and filter..clean specifies\n \t * the command line:\n@@ -490,13 +486,13 @@ static int read_convert_config(const char *var, const char *value, void *cb)\n \t * The command-line will not be interpolated in any way.\n \t */\n \n-\tif (!strcmp(\"smudge\", ep))\n+\tif (!strcmp(\"smudge\", key))\n \t\treturn git_config_string(&drv->smudge, var, value);\n \n-\tif (!strcmp(\"clean\", ep))\n+\tif (!strcmp(\"clean\", key))\n \t\treturn git_config_string(&drv->clean, var, value);\n \n-\tif (!strcmp(\"required\", ep)) {\n+\tif (!strcmp(\"required\", key)) {\n \t\tdrv->required = git_config_bool(var, value);\n \t\treturn 0;\n \t}\ndiff --git a/ll-merge.c b/ll-merge.c\nindex acea33b..fb61ea6 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -222,7 +222,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n static int read_merge_config(const char *var, const char *value, void *cb)\n {\n \tstruct ll_merge_driver *fn;\n-\tconst char *ep, *name;\n+\tconst char *key, *name;\n \tint namelen;\n \n \tif (!strcmp(var, \"merge.default\")) {\n@@ -236,15 +236,13 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t * especially, we do not want to look at variables such as\n \t * \"merge.summary\", \"merge.tool\", and \"merge.verbosity\".\n \t */\n-\tif (prefixcmp(var, \"merge.\") || (ep = strrchr(var, '.')) == var + 5)\n+\tif (parse_config_key(var, \"merge\", &name, &namelen, &key) < 0 || !name)\n \t\treturn 0;\n \n \t/*\n \t * Find existing one as we might be processing merge..var2\n \t * after seeing merge..var1.\n \t */\n-\tname = var + 6;\n-\tnamelen = ep - name;\n \tfor (fn = ll_user_merge; fn; fn = fn->next)\n \t\tif (!strncmp(fn->name, name, namelen) && !fn->name[namelen])\n \t\t\tbreak;\n@@ -256,16 +254,14 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t\tll_user_merge_tail = &(fn->next);\n \t}\n \n-\tep++;\n-\n-\tif (!strcmp(\"name\", ep)) {\n+\tif (!strcmp(\"name\", key)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n \t\tfn->description = xstrdup(value);\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(\"driver\", ep)) {\n+\tif (!strcmp(\"driver\", key)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n \t\t/*\n@@ -289,7 +285,7 @@ static int read_merge_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tif (!strcmp(\"recursive\", ep)) {\n+\tif (!strcmp(\"recursive\", key)) {\n \t\tif (!value)\n \t\t\treturn error(\"%s: lacks value\", var);\n \t\tfn->recursive = xstrdup(value);\ndiff --git a/userdiff.c b/userdiff.c\nindex ed958ef..a4ea1e9 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -188,20 +188,13 @@ static struct userdiff_driver *parse_driver(const char *var,\n \t\tconst char *value, const char *type)\n {\n \tstruct userdiff_driver *drv;\n-\tconst char *dot;\n-\tconst char *name;\n+\tconst char *name, *key;\n \tint namelen;\n \n-\tif (prefixcmp(var, \"diff.\"))\n-\t\treturn NULL;\n-\tdot = strrchr(var, '.');\n-\tif (dot == var + 4)\n-\t\treturn NULL;\n-\tif (strcmp(type, dot+1))\n+\tif (parse_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n+\t !name || strcmp(type, key))\n \t\treturn NULL;\n \n-\tname = var + 5;\n-\tnamelen = dot - name;\n \tdrv = userdiff_find_by_namelen(name, namelen);\n \tif (!drv) {\n \t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207559","messageId":"20130123062507.GD5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 4/8] userdiff: drop parse_driver function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:25:07Z","receivedAt":"2013-01-23T06:25:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse userdiff config, we generally assume that\n\n diff.name.key\n\nwill affect the \"key\" value of the \"name\" driver. However,\nwithout confirming that the key is a valid userdiff key, we\nmay accidentally conflict with the ancient \"diff.color.*\"\nnamespace. The current code is careful not to even create a\ndriver struct if we do not see a key that is known by the\ndiff-driver code.\n\nHowever, this carefulness is unnecessary; the default driver\nwith no keys set behaves exactly the same as having no\ndriver at all. We can simply set up the driver struct as\nsoon as we see we have a config key that looks like a\ndriver. This makes the code a bit more readable.\n\nSigned-off-by: Jeff King \n---\n userdiff.c | 50 +++++++++++++++++++++-----------------------------\n 1 file changed, 21 insertions(+), 29 deletions(-)\n\ndiff --git a/userdiff.c b/userdiff.c\nindex a4ea1e9..ea43a03 100644\n--- a/userdiff.c\n+++ b/userdiff.c\n@@ -184,28 +184,6 @@ static struct userdiff_driver *userdiff_find_by_namelen(const char *k, int len)\n \treturn NULL;\n }\n \n-static struct userdiff_driver *parse_driver(const char *var,\n-\t\tconst char *value, const char *type)\n-{\n-\tstruct userdiff_driver *drv;\n-\tconst char *name, *key;\n-\tint namelen;\n-\n-\tif (parse_config_key(var, \"diff\", &name, &namelen, &key) < 0 ||\n-\t !name || strcmp(type, key))\n-\t\treturn NULL;\n-\n-\tdrv = userdiff_find_by_namelen(name, namelen);\n-\tif (!drv) {\n-\t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n-\t\tdrv = &drivers[ndrivers++];\n-\t\tmemset(drv, 0, sizeof(*drv));\n-\t\tdrv->name = xmemdupz(name, namelen);\n-\t\tdrv->binary = -1;\n-\t}\n-\treturn drv;\n-}\n-\n static int parse_funcname(struct userdiff_funcname *f, const char *k,\n \t\tconst char *v, int cflags)\n {\n@@ -233,20 +211,34 @@ int userdiff_config(const char *k, const char *v)\n int userdiff_config(const char *k, const char *v)\n {\n \tstruct userdiff_driver *drv;\n+\tconst char *name, *type;\n+\tint namelen;\n+\n+\tif (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n+\t\treturn 0;\n+\n+\tdrv = userdiff_find_by_namelen(name, namelen);\n+\tif (!drv) {\n+\t\tALLOC_GROW(drivers, ndrivers+1, drivers_alloc);\n+\t\tdrv = &drivers[ndrivers++];\n+\t\tmemset(drv, 0, sizeof(*drv));\n+\t\tdrv->name = xmemdupz(name, namelen);\n+\t\tdrv->binary = -1;\n+\t}\n \n-\tif ((drv = parse_driver(k, v, \"funcname\")))\n+\tif (!strcmp(type, \"funcname\"))\n \t\treturn parse_funcname(&drv->funcname, k, v, 0);\n-\tif ((drv = parse_driver(k, v, \"xfuncname\")))\n+\tif (!strcmp(type, \"xfuncname\"))\n \t\treturn parse_funcname(&drv->funcname, k, v, REG_EXTENDED);\n-\tif ((drv = parse_driver(k, v, \"binary\")))\n+\tif (!strcmp(type, \"binary\"))\n \t\treturn parse_tristate(&drv->binary, k, v);\n-\tif ((drv = parse_driver(k, v, \"command\")))\n+\tif (!strcmp(type, \"command\"))\n \t\treturn git_config_string(&drv->external, k, v);\n-\tif ((drv = parse_driver(k, v, \"textconv\")))\n+\tif (!strcmp(type, \"textconv\"))\n \t\treturn git_config_string(&drv->textconv, k, v);\n-\tif ((drv = parse_driver(k, v, \"cachetextconv\")))\n+\tif (!strcmp(type, \"cachetextconv\"))\n \t\treturn parse_bool(&drv->textconv_want_cache, k, v);\n-\tif ((drv = parse_driver(k, v, \"wordregex\")))\n+\tif (!strcmp(type, \"wordregex\"))\n \t\treturn git_config_string(&drv->word_regex, k, v);\n \n \treturn 0;\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207560","messageId":"20130123062522.GE5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 5/8] submodule: use parse_config_key when parsing config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:25:22Z","receivedAt":"2013-01-23T06:25:22Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This makes the code a lot simpler to read by dropping a\nwhole bunch of constant offsets.\n\nAs a bonus, it means we also feed the whole config variable\nname to our error functions:\n\n [before]\n $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout\n fatal: bad foo.fetchrecursesubmodules argument: bogus\n\n [after]\n $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout\n fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus\n\nSigned-off-by: Jeff King \n---\n submodule.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2f55436..25413de 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -126,15 +126,16 @@ int parse_submodule_config_option(const char *var, const char *value)\n \n int parse_submodule_config_option(const char *var, const char *value)\n {\n-\tint len;\n \tstruct string_list_item *config;\n \tstruct strbuf submodname = STRBUF_INIT;\n+\tconst char *name, *key;\n+\tint namelen;\n \n-\tvar += 10;\t\t/* Skip \"submodule.\" */\n+\tif (parse_config_key(var, \"submodule\", &name, &namelen, &key) < 0 || !name)\n+\t\treturn 0;\n \n-\tlen = strlen(var);\n-\tif ((len > 5) && !strcmp(var + len - 5, \".path\")) {\n-\t\tstrbuf_add(&submodname, var, len - 5);\n+\tif (!strcmp(key, \"path\")) {\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n \t\tif (config)\n \t\t\tfree(config->util);\n@@ -142,22 +143,22 @@ int parse_submodule_config_option(const char *var, const char *value)\n \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n \t\tconfig->util = strbuf_detach(&submodname, NULL);\n \t\tstrbuf_release(&submodname);\n-\t} else if ((len > 23) && !strcmp(var + len - 23, \".fetchrecursesubmodules\")) {\n-\t\tstrbuf_add(&submodname, var, len - 23);\n+\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n \t\tif (!config)\n \t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n \t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n \t\tstrbuf_release(&submodname);\n-\t} else if ((len > 7) && !strcmp(var + len - 7, \".ignore\")) {\n+\t} else if (!strcmp(key, \"ignore\")) {\n \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n \t\t\treturn 0;\n \t\t}\n \n-\t\tstrbuf_add(&submodname, var, len - 7);\n+\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n \t\tif (config)\n \t\t\tfree(config->util);\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207561","messageId":"20130123062642.GF5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 6/8] submodule: simplify memory handling in config parsing","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:26:42Z","receivedAt":"2013-01-23T06:26:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We keep a strbuf for the name of the submodule, even though\nwe only ever add one string to it. Let's just use xmemdupz\ninstead, which is slightly more efficient and makes it\neasier to follow what is going on.\n\nUnfortunately, we still end up having to deal with some\nmemory ownership issues in some code branches, as we have to\nallocate the string in order to do a string list lookup, and\nthen only sometimes want to hand ownership of that string\nover to the string_list. Still, making that explicit in the\ncode (as opposed to sometimes detaching the strbuf, and then\nalways releasing it) makes it a little more obvious what is\ngoing on.\n\nSigned-off-by: Jeff King \n---\n submodule.c | 30 ++++++++++++++----------------\n 1 file changed, 14 insertions(+), 16 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 25413de..9ba1496 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)\n int parse_submodule_config_option(const char *var, const char *value)\n {\n \tstruct string_list_item *config;\n-\tstruct strbuf submodname = STRBUF_INIT;\n \tconst char *name, *key;\n \tint namelen;\n \n@@ -135,37 +134,36 @@ int parse_submodule_config_option(const char *var, const char *value)\n \t\treturn 0;\n \n \tif (!strcmp(key, \"path\")) {\n-\t\tstrbuf_add(&submodname, name, namelen);\n \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n \t\tif (config)\n \t\t\tfree(config->util);\n \t\telse\n \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n-\t\tconfig->util = strbuf_detach(&submodname, NULL);\n-\t\tstrbuf_release(&submodname);\n+\t\tconfig->util = xmemdupz(name, namelen);\n \t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n-\t\tstrbuf_add(&submodname, name, namelen);\n-\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n+\t\tchar *name_cstr = xmemdupz(name, namelen);\n+\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);\n \t\tif (!config)\n-\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n+\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);\n+\t\telse\n+\t\t\tfree(name_cstr);\n \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n-\t\tstrbuf_release(&submodname);\n \t} else if (!strcmp(key, \"ignore\")) {\n+\t\tchar *name_cstr;\n+\n \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n \t\t\treturn 0;\n \t\t}\n \n-\t\tstrbuf_add(&submodname, name, namelen);\n-\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n-\t\tif (config)\n+\t\tname_cstr = xmemdupz(name, namelen);\n+\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);\n+\t\tif (config) {\n \t\t\tfree(config->util);\n-\t\telse\n-\t\t\tconfig = string_list_append(&config_ignore_for_name,\n-\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n-\t\tstrbuf_release(&submodname);\n+\t\t\tfree(name_cstr);\n+\t\t} else\n+\t\t\tconfig = string_list_append(&config_ignore_for_name, name_cstr);\n \t\tconfig->util = xstrdup(value);\n \t\treturn 0;\n \t}\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207562","messageId":"20130123062709.GG5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 7/8] help: use parse_config_key for man config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:27:09Z","receivedAt":"2013-01-23T06:27:09Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The resulting code ends up about the same length, but it is\na little more self-explanatory. It now explicitly documents\nand checks the pre-condition that the incoming var starts\nwith \"man.\", and drops the magic offset \"4\".\n\nSigned-off-by: Jeff King \n---\n builtin/help.c | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/help.c b/builtin/help.c\nindex bd86253..04cb77d 100644\n--- a/builtin/help.c\n+++ b/builtin/help.c\n@@ -237,21 +237,21 @@ static int add_man_viewer_info(const char *var, const char *value)\n \n static int add_man_viewer_info(const char *var, const char *value)\n {\n-\tconst char *name = var + 4;\n-\tconst char *subkey = strrchr(name, '.');\n+\tconst char *name, *subkey;\n+\tint namelen;\n \n-\tif (!subkey)\n+\tif (parse_config_key(var, \"man\", &name, &namelen, &subkey) < 0 || !name)\n \t\treturn 0;\n \n-\tif (!strcmp(subkey, \".path\")) {\n+\tif (!strcmp(subkey, \"path\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\treturn add_man_viewer_path(name, subkey - name, value);\n+\t\treturn add_man_viewer_path(name, namelen, value);\n \t}\n-\tif (!strcmp(subkey, \".cmd\")) {\n+\tif (!strcmp(subkey, \"cmd\")) {\n \t\tif (!value)\n \t\t\treturn config_error_nonbool(var);\n-\t\treturn add_man_viewer_cmd(name, subkey - name, value);\n+\t\treturn add_man_viewer_cmd(name, namelen, value);\n \t}\n \n \treturn 0;\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207563","messageId":"20130123062737.GH5036@sigill.intra.peff.net","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"[PATCHv2 8/8] reflog: use parse_config_key in config callback","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-01-23T06:27:37Z","receivedAt":"2013-01-23T06:27:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This doesn't save any lines, but does keep us from doing\nerror-prone pointer arithmetic with constants.\n\nSigned-off-by: Jeff King \n---\n builtin/reflog.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex b3c9e27..1fedf66 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -510,26 +510,27 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)\n \n static int reflog_expire_config(const char *var, const char *value, void *cb)\n {\n-\tconst char *lastdot = strrchr(var, '.');\n+\tconst char *pattern, *key;\n+\tint pattern_len;\n \tunsigned long expire;\n \tint slot;\n \tstruct reflog_expire_cfg *ent;\n \n-\tif (!lastdot || prefixcmp(var, \"gc.\"))\n+\tif (parse_config_key(var, \"gc\", &pattern, &pattern_len, &key) < 0)\n \t\treturn git_default_config(var, value, cb);\n \n-\tif (!strcmp(lastdot, \".reflogexpire\")) {\n+\tif (!strcmp(key, \"reflogexpire\")) {\n \t\tslot = EXPIRE_TOTAL;\n \t\tif (parse_expire_cfg_value(var, value, &expire))\n \t\t\treturn -1;\n-\t} else if (!strcmp(lastdot, \".reflogexpireunreachable\")) {\n+\t} else if (!strcmp(key, \"reflogexpireunreachable\")) {\n \t\tslot = EXPIRE_UNREACH;\n \t\tif (parse_expire_cfg_value(var, value, &expire))\n \t\t\treturn -1;\n \t} else\n \t\treturn git_default_config(var, value, cb);\n \n-\tif (lastdot == var + 2) {\n+\tif (!pattern) {\n \t\tswitch (slot) {\n \t\tcase EXPIRE_TOTAL:\n \t\t\tdefault_reflog_expire = expire;\n@@ -541,7 +542,7 @@ static int reflog_expire_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \n-\tent = find_cfg_ent(var + 3, lastdot - (var+3));\n+\tent = find_cfg_ent(pattern, pattern_len);\n \tif (!ent)\n \t\treturn -1;\n \tswitch (slot) {\n-- \n1.8.0.2.15.g815dc66\n"},{"id":"207568","messageId":"7vpq0widma.fsf@alter.siamese.dyndns.org","threadId":"32615","inReplyTo":"20130123062737.GH5036@sigill.intra.peff.net","subject":"Re: [PATCHv2 8/8] reflog: use parse_config_key in config callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-23T07:04:45Z","receivedAt":"2013-01-23T07:04:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> This doesn't save any lines, but does keep us from doing\n> error-prone pointer arithmetic with constants.\n\nYeah, this and 7/8 shows that the true value of the new parse\nfunction is not line number reduction but clarity of the calling\ncode. There is really no point making everybody implement \"last_dot\nis there, so the subsection name must be between the section name\nand that last_dot\" like the original before this patch.\n\nThanks. Will read it over again and then apply.\n"},{"id":"207569","messageId":"20130123072716.GB3361@elie.Belkin","threadId":"32615","inReplyTo":"20130123062132.GA2038@sigill.intra.peff.net","subject":"Re: [PATCHv2 0/8] config key-parsing cleanups","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-23T07:27:16Z","receivedAt":"2013-01-23T07:27:16Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> So I've re-rolled the original version\n\nLovely. :)\nReviewed-by: Jonathan Nieder \n"},{"id":"207634","messageId":"51004BDF.6040606@web.de","threadId":"32615","inReplyTo":"20130123062522.GE5036@sigill.intra.peff.net","subject":"Re: [PATCHv2 5/8] submodule: use parse_config_key when parsing config","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-23T20:45:19Z","receivedAt":"2013-01-23T20:45:19Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 23.01.2013 07:25, schrieb Jeff King:\n> This makes the code a lot simpler to read by dropping a\n> whole bunch of constant offsets.\n> \n> As a bonus, it means we also feed the whole config variable\n> name to our error functions:\n> \n> [before]\n> $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout\n> fatal: bad foo.fetchrecursesubmodules argument: bogus\n> \n> [after]\n> $ git -c submodule.foo.fetchrecursesubmodules=bogus checkout\n> fatal: bad submodule.foo.fetchrecursesubmodules argument: bogus\n\nThanks, that makes lots of sense!\n\nAcked-by: Jens Lehmann \n\n> Signed-off-by: Jeff King \n> ---\n> submodule.c | 19 ++++++++++---------\n> 1 file changed, 10 insertions(+), 9 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 2f55436..25413de 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -126,15 +126,16 @@ int parse_submodule_config_option(const char *var, const char *value)\n> \n> int parse_submodule_config_option(const char *var, const char *value)\n> {\n> -\tint len;\n> \tstruct string_list_item *config;\n> \tstruct strbuf submodname = STRBUF_INIT;\n> +\tconst char *name, *key;\n> +\tint namelen;\n> \n> -\tvar += 10;\t\t/* Skip \"submodule.\" */\n> +\tif (parse_config_key(var, \"submodule\", &name, &namelen, &key) < 0 || !name)\n> +\t\treturn 0;\n> \n> -\tlen = strlen(var);\n> -\tif ((len > 5) && !strcmp(var + len - 5, \".path\")) {\n> -\t\tstrbuf_add(&submodname, var, len - 5);\n> +\tif (!strcmp(key, \"path\")) {\n> +\t\tstrbuf_add(&submodname, name, namelen);\n> \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n> \t\tif (config)\n> \t\t\tfree(config->util);\n> @@ -142,22 +143,22 @@ int parse_submodule_config_option(const char *var, const char *value)\n> \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n> \t\tconfig->util = strbuf_detach(&submodname, NULL);\n> \t\tstrbuf_release(&submodname);\n> -\t} else if ((len > 23) && !strcmp(var + len - 23, \".fetchrecursesubmodules\")) {\n> -\t\tstrbuf_add(&submodname, var, len - 23);\n> +\t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n> +\t\tstrbuf_add(&submodname, name, namelen);\n> \t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n> \t\tif (!config)\n> \t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n> \t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n> \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n> \t\tstrbuf_release(&submodname);\n> -\t} else if ((len > 7) && !strcmp(var + len - 7, \".ignore\")) {\n> +\t} else if (!strcmp(key, \"ignore\")) {\n> \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n> \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n> \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n> \t\t\treturn 0;\n> \t\t}\n> \n> -\t\tstrbuf_add(&submodname, var, len - 7);\n> +\t\tstrbuf_add(&submodname, name, namelen);\n> \t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n> \t\tif (config)\n> \t\t\tfree(config->util);\n> \n"},{"id":"207635","messageId":"51004D4B.5090409@web.de","threadId":"32615","inReplyTo":"20130123062642.GF5036@sigill.intra.peff.net","subject":"Re: [PATCHv2 6/8] submodule: simplify memory handling in config parsing","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2013-01-23T20:51:23Z","receivedAt":"2013-01-23T20:51:23Z","isPatch":false,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Am 23.01.2013 07:26, schrieb Jeff King:\n> We keep a strbuf for the name of the submodule, even though\n> we only ever add one string to it. Let's just use xmemdupz\n> instead, which is slightly more efficient and makes it\n> easier to follow what is going on.\n> \n> Unfortunately, we still end up having to deal with some\n> memory ownership issues in some code branches, as we have to\n> allocate the string in order to do a string list lookup, and\n> then only sometimes want to hand ownership of that string\n> over to the string_list. Still, making that explicit in the\n> code (as opposed to sometimes detaching the strbuf, and then\n> always releasing it) makes it a little more obvious what is\n> going on.\n\nThanks, this helps until I some day find the time to refactor\nthat code into a more digestible shape ;-)\n\nAcked-by: Jens Lehmann \n\n> Signed-off-by: Jeff King \n> ---\n> submodule.c | 30 ++++++++++++++----------------\n> 1 file changed, 14 insertions(+), 16 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index 25413de..9ba1496 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -127,7 +127,6 @@ int parse_submodule_config_option(const char *var, const char *value)\n> int parse_submodule_config_option(const char *var, const char *value)\n> {\n> \tstruct string_list_item *config;\n> -\tstruct strbuf submodname = STRBUF_INIT;\n> \tconst char *name, *key;\n> \tint namelen;\n> \n> @@ -135,37 +134,36 @@ int parse_submodule_config_option(const char *var, const char *value)\n> \t\treturn 0;\n> \n> \tif (!strcmp(key, \"path\")) {\n> -\t\tstrbuf_add(&submodname, name, namelen);\n> \t\tconfig = unsorted_string_list_lookup(&config_name_for_path, value);\n> \t\tif (config)\n> \t\t\tfree(config->util);\n> \t\telse\n> \t\t\tconfig = string_list_append(&config_name_for_path, xstrdup(value));\n> -\t\tconfig->util = strbuf_detach(&submodname, NULL);\n> -\t\tstrbuf_release(&submodname);\n> +\t\tconfig->util = xmemdupz(name, namelen);\n> \t} else if (!strcmp(key, \"fetchrecursesubmodules\")) {\n> -\t\tstrbuf_add(&submodname, name, namelen);\n> -\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, submodname.buf);\n> +\t\tchar *name_cstr = xmemdupz(name, namelen);\n> +\t\tconfig = unsorted_string_list_lookup(&config_fetch_recurse_submodules_for_name, name_cstr);\n> \t\tif (!config)\n> -\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name,\n> -\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n> +\t\t\tconfig = string_list_append(&config_fetch_recurse_submodules_for_name, name_cstr);\n> +\t\telse\n> +\t\t\tfree(name_cstr);\n> \t\tconfig->util = (void *)(intptr_t)parse_fetch_recurse_submodules_arg(var, value);\n> -\t\tstrbuf_release(&submodname);\n> \t} else if (!strcmp(key, \"ignore\")) {\n> +\t\tchar *name_cstr;\n> +\n> \t\tif (strcmp(value, \"untracked\") && strcmp(value, \"dirty\") &&\n> \t\t strcmp(value, \"all\") && strcmp(value, \"none\")) {\n> \t\t\twarning(\"Invalid parameter \\\"%s\\\" for config option \\\"submodule.%s.ignore\\\"\", value, var);\n> \t\t\treturn 0;\n> \t\t}\n> \n> -\t\tstrbuf_add(&submodname, name, namelen);\n> -\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, submodname.buf);\n> -\t\tif (config)\n> +\t\tname_cstr = xmemdupz(name, namelen);\n> +\t\tconfig = unsorted_string_list_lookup(&config_ignore_for_name, name_cstr);\n> +\t\tif (config) {\n> \t\t\tfree(config->util);\n> -\t\telse\n> -\t\t\tconfig = string_list_append(&config_ignore_for_name,\n> -\t\t\t\t\t\t strbuf_detach(&submodname, NULL));\n> -\t\tstrbuf_release(&submodname);\n> +\t\t\tfree(name_cstr);\n> +\t\t} else\n> +\t\t\tconfig = string_list_append(&config_ignore_for_name, name_cstr);\n> \t\tconfig->util = xstrdup(value);\n> \t\treturn 0;\n> \t}\n> \n"}]}