{"thread":{"id":"45216","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","startedAt":"2017-02-24T20:39:48Z","lastAt":"2017-02-24T23:28:13Z","messageCount":16,"participants":["Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312555","messageId":"20170224203940.hbmfsouw5k67l3h3@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224203335.3762-1-sbeller@google.com","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T20:39:41Z","receivedAt":"2017-02-24T20:39:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 12:33:35PM -0800, Stefan Beller wrote:\n\n> parse_config_key was introduced in 1b86bbb0ade (config: add helper\n> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed\n> in this patch was introduced at daebaa7813 (upload/receive-pack: allow\n> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,\n> so presumably the code replaced in this patch was only introduced due\n> to not wanting to wait on the proper helper function being available.\n> \n> Make the condition easier to read by using parse_config_key.\n> \n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n> \n>   When investigating the state of the art for parsing config options, I saw\n>   opportunity for a small drive-by patch in an area that I did not look at for \n>   a long time.\n>   \n>   The authors of the two mentioned commits are Jeff and Junio, so maybe you\n>   remember another reason for this NEEDSWORK here?\n\nNo, I think the reasoning you gave in the commit message is exactly\nwhat happened.\n\n> diff --git a/refs.c b/refs.c\n> index cd36b64ed9..c9e5f13630 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;\n>  \n>  int parse_hide_refs_config(const char *var, const char *value, const char *section)\n>  {\n> +\tconst char *subsection, *key;\n> +\tint subsection_len;\n>  \tif (!strcmp(\"transfer.hiderefs\", var) ||\n> -\t    /* NEEDSWORK: use parse_config_key() once both are merged */\n> -\t    (starts_with(var, section) && var[strlen(section)] == '.' &&\n> -\t     !strcmp(var + strlen(section), \".hiderefs\"))) {\n> +\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n> +\t    && !strcmp(key, \"hiderefs\"))) {\n\nThis will start parsing \"receive.foobar.hiderefs\", which we don't want.\nI think you need:\n\n  !parse_config_key(var, section, &subsection, &subsection_len, &key) &&\n  !subsection &&\n  !strcmp(key, \"hiderefs\")\n\nPerhaps passing NULL for the subsection variable should cause\nparse_config_key to return failure when there is a non-empty subsection.\n\n-Peff\n\nPS Outside of parse_config_key, this code would be nicer if it used\n   skip_prefix() instead of starts_with(). Since it's going away, I\n   don't think it matters, but I note that parse_config_key could\n   probably benefit from the same.\n"},{"id":"312556","messageId":"20170224203335.3762-1-sbeller@google.com","threadId":"45216","inReplyTo":null,"subject":"[PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-24T20:33:35Z","receivedAt":"2017-02-24T20:43:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"parse_config_key was introduced in 1b86bbb0ade (config: add helper\nfunction for parsing key names, 2013-01-22), the NEEDSWORK that is removed\nin this patch was introduced at daebaa7813 (upload/receive-pack: allow\nhiding ref hierarchies, 2013-01-18), which is only a couple days apart,\nso presumably the code replaced in this patch was only introduced due\nto not wanting to wait on the proper helper function being available.\n\nMake the condition easier to read by using parse_config_key.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\n  When investigating the state of the art for parsing config options, I saw\n  opportunity for a small drive-by patch in an area that I did not look at for \n  a long time.\n  \n  The authors of the two mentioned commits are Jeff and Junio, so maybe you\n  remember another reason for this NEEDSWORK here?\n  \n  Thanks,\n  Stefan\n  \n refs.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cd36b64ed9..c9e5f13630 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;\n \n int parse_hide_refs_config(const char *var, const char *value, const char *section)\n {\n+\tconst char *subsection, *key;\n+\tint subsection_len;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t    /* NEEDSWORK: use parse_config_key() once both are merged */\n-\t    (starts_with(var, section) && var[strlen(section)] == '.' &&\n-\t     !strcmp(var + strlen(section), \".hiderefs\"))) {\n+\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n+\t    && !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n \n-- \n2.12.0.rc1.16.ge4278d41a0.dirty\n\n"},{"id":"312557","messageId":"20170224204335.10652-1-sbeller@google.com","threadId":"45216","inReplyTo":"20170224203940.hbmfsouw5k67l3h3@sigill.intra.peff.net","subject":"[PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-24T20:43:35Z","receivedAt":"2017-02-24T20:43:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"parse_config_key was introduced in 1b86bbb0ade (config: add helper\nfunction for parsing key names, 2013-01-22), the NEEDSWORK that is removed\nin this patch was introduced at daebaa7813 (upload/receive-pack: allow\nhiding ref hierarchies, 2013-01-18), which is only a couple days apart,\nso presumably the code replaced in this patch was only introduced due\nto not wanting to wait on the proper helper function being available.\n\nMake the condition easier to read by using parse_config_key.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cd36b64ed9..21bc8c9101 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;\n \n int parse_hide_refs_config(const char *var, const char *value, const char *section)\n {\n+\tconst char *subsection, *key;\n+\tint subsection_len;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t    /* NEEDSWORK: use parse_config_key() once both are merged */\n-\t    (starts_with(var, section) && var[strlen(section)] == '.' &&\n-\t     !strcmp(var + strlen(section), \".hiderefs\"))) {\n+\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n+\t    && !subsection && !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n \n-- \n2.12.0.rc1.16.ge4278d41a0.dirty\n\n"},{"id":"312558","messageId":"20170224204725.6dq3zmazugzv2y6o@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224204335.10652-1-sbeller@google.com","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T20:47:25Z","receivedAt":"2017-02-24T20:47:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 12:43:35PM -0800, Stefan Beller wrote:\n\n> parse_config_key was introduced in 1b86bbb0ade (config: add helper\n> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed\n> in this patch was introduced at daebaa7813 (upload/receive-pack: allow\n> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,\n> so presumably the code replaced in this patch was only introduced due\n> to not wanting to wait on the proper helper function being available.\n> \n> Make the condition easier to read by using parse_config_key.\n> [...]\n>  \tif (!strcmp(\"transfer.hiderefs\", var) ||\n> -\t    /* NEEDSWORK: use parse_config_key() once both are merged */\n> -\t    (starts_with(var, section) && var[strlen(section)] == '.' &&\n> -\t     !strcmp(var + strlen(section), \".hiderefs\"))) {\n> +\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n> +\t    && !subsection && !strcmp(key, \"hiderefs\"))) {\n\nYeah, this one looks fine.\n\n-Peff\n"},{"id":"312559","messageId":"xmqqd1e71fkl.fsf@gitster.mtv.corp.google.com","threadId":"45216","inReplyTo":"20170224204335.10652-1-sbeller@google.com","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T20:47:38Z","receivedAt":"2017-02-24T20:47:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> parse_config_key was introduced in 1b86bbb0ade (config: add helper\n> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed\n> in this patch was introduced at daebaa7813 (upload/receive-pack: allow\n> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,\n> so presumably the code replaced in this patch was only introduced due\n> to not wanting to wait on the proper helper function being available.\n>\n> Make the condition easier to read by using parse_config_key.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  refs.c | 7 ++++---\n>  1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index cd36b64ed9..21bc8c9101 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;\n>  \n>  int parse_hide_refs_config(const char *var, const char *value, const char *section)\n>  {\n> +\tconst char *subsection, *key;\n> +\tint subsection_len;\n>  \tif (!strcmp(\"transfer.hiderefs\", var) ||\n> -\t    /* NEEDSWORK: use parse_config_key() once both are merged */\n> -\t    (starts_with(var, section) && var[strlen(section)] == '.' &&\n> -\t     !strcmp(var + strlen(section), \".hiderefs\"))) {\n> +\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n> +\t    && !subsection && !strcmp(key, \"hiderefs\"))) {\n>  \t\tchar *ref;\n>  \t\tint len;\n\nThanks for noticing.  \"once both are merged\" is a cryptic comment to\nleave when somebody knows which two topics make \"both\" ;-)\n"},{"id":"312561","messageId":"20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224203940.hbmfsouw5k67l3h3@sigill.intra.peff.net","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:06:44Z","receivedAt":"2017-02-24T21:13:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 03:39:40PM -0500, Jeff King wrote:\n\n> This will start parsing \"receive.foobar.hiderefs\", which we don't want.\n> I think you need:\n> \n>   !parse_config_key(var, section, &subsection, &subsection_len, &key) &&\n>   !subsection &&\n>   !strcmp(key, \"hiderefs\")\n> \n> Perhaps passing NULL for the subsection variable should cause\n> parse_config_key to return failure when there is a non-empty subsection.\n> \n> -Peff\n> \n> PS Outside of parse_config_key, this code would be nicer if it used\n>    skip_prefix() instead of starts_with(). Since it's going away, I\n>    don't think it matters, but I note that parse_config_key could\n>    probably benefit from the same.\n\nWhile I'm thinking about it, here are patches to do that. The third one\nI'd probably squash into yours (after ordering it to the end).\n\n  [1/3]: parse_config_key: use skip_prefix instead of starts_with\n  [2/3]: parse_config_key: allow matching single-level config\n  [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection\n\n cache.h  |  5 ++++-\n config.c | 15 +++++++++------\n refs.c   |  7 +++----\n 3 files changed, 16 insertions(+), 11 deletions(-)\n\n"},{"id":"312562","messageId":"20170224210719.zyf5bd6dtwhi5s3f@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net","subject":"[PATCH 1/3] parse_config_key: use skip_prefix instead of starts_with","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:07:19Z","receivedAt":"2017-02-24T21:14:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This saves us having to repeatedly add in \"section_len\" (and\nalso avoids walking over the first part of the string\nmultiple times for a strlen() and strrchr()).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n config.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex c6b874a7b..1b08a75a7 100644\n--- a/config.c\n+++ b/config.c\n@@ -2536,11 +2536,10 @@ int parse_config_key(const char *var,\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 (!starts_with(var, section) || var[section_len] != '.')\n+\tif (!skip_prefix(var, section, &var) || *var != '.')\n \t\treturn -1;\n \n \t/*\n@@ -2552,12 +2551,12 @@ int parse_config_key(const char *var,\n \t*key = dot + 1;\n \n \t/* Did we have a subsection at all? */\n-\tif (dot == var + section_len) {\n+\tif (dot == var) {\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 = var + 1;\n \t\t*subsection_len = dot - *subsection;\n \t}\n \n-- \n2.12.0.616.g5f622f3b1\n\n"},{"id":"312563","messageId":"20170224210802.rpr5vdpqhsp3pt5v@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net","subject":"[PATCH 2/3] parse_config_key: allow matching single-level config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:08:02Z","receivedAt":"2017-02-24T21:14:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The parse_config_key() function was introduced to make it\neasier to match \"section.subsection.key\" variables. It also\nhandles the simpler \"section.key\", and the caller is\nresponsible for distinguishing the two from its\nout-parameters.\n\nMost callers who _only_ want \"section.key\" would just use a\nstrcmp(var, \"section.key\"), since there is no parsing\nrequired. However, they may still use parse_config_key() if\ntheir \"section\" variable isn't a constant (an example of\nthis is in parse_hide_refs_config).\n\nUsing the parse_config_key is a bit clunky, though:\n\n  const char *subsection;\n  int subsection_len;\n  const char *key;\n\n  if (!parse_config_key(var, section, &subsection, &subsection_len, &key) &&\n      !subsection) {\n\t  /* matched! */\n  }\n\nInstead, let's treat a NULL subsection as an indication that\nthe caller does not expect one. That lets us write:\n\n  const char *key;\n\n  if (!parse_config_key(var, section, NULL, NULL, &key)) {\n\t  /* matched! */\n  }\n\nExisting callers should be unaffected, as passing a NULL\nsubsection would currently segfault.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h  | 5 ++++-\n config.c | 8 ++++++--\n 2 files changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 61fc86e6d..647a78f3f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1819,8 +1819,11 @@ extern int git_config_include(const char *name, const char *value, void *data);\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+ * out-parameters are filled by the function (and *subsection is NULL if it is\n  * missing).\n+ *\n+ * If the subsection pointer-to-pointer passed in is NULL, returns 0 only if\n+ * there is no subsection at all.\n  */\n extern int parse_config_key(const char *var,\n \t\t\t    const char *section,\ndiff --git a/config.c b/config.c\nindex 1b08a75a7..13c8b21ea 100644\n--- a/config.c\n+++ b/config.c\n@@ -2552,10 +2552,14 @@ int parse_config_key(const char *var,\n \n \t/* Did we have a subsection at all? */\n \tif (dot == var) {\n-\t\t*subsection = NULL;\n-\t\t*subsection_len = 0;\n+\t\tif (subsection) {\n+\t\t\t*subsection = NULL;\n+\t\t\t*subsection_len = 0;\n+\t\t}\n \t}\n \telse {\n+\t\tif (!subsection)\n+\t\t\treturn -1;\n \t\t*subsection = var + 1;\n \t\t*subsection_len = dot - *subsection;\n \t}\n-- \n2.12.0.616.g5f622f3b1\n\n"},{"id":"312564","messageId":"20170224210816.ftcfa4jy4wltv2b7@sigill.intra.peff.net","threadId":"45216","inReplyTo":"20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net","subject":"[PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:08:16Z","receivedAt":"2017-02-24T21:15:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This lets us avoid declaring some otherwise useless\nvariables.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 21bc8c910..b9188908b 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1034,11 +1034,10 @@ static struct string_list *hide_refs;\n \n int parse_hide_refs_config(const char *var, const char *value, const char *section)\n {\n-\tconst char *subsection, *key;\n-\tint subsection_len;\n+\tconst char *key;\n \tif (!strcmp(\"transfer.hiderefs\", var) ||\n-\t    (!parse_config_key(var, section, &subsection, &subsection_len, &key)\n-\t    && !subsection && !strcmp(key, \"hiderefs\"))) {\n+\t    (!parse_config_key(var, section, NULL, NULL, &key) &&\n+\t     !strcmp(key, \"hiderefs\"))) {\n \t\tchar *ref;\n \t\tint len;\n \n-- \n2.12.0.616.g5f622f3b1\n"},{"id":"312565","messageId":"xmqq4lzj1e4n.fsf@gitster.mtv.corp.google.com","threadId":"45216","inReplyTo":"20170224210643.max6z2ykm3gbg7lw@sigill.intra.peff.net","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T21:18:48Z","receivedAt":"2017-02-24T21:18:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 24, 2017 at 03:39:40PM -0500, Jeff King wrote:\n>\n>> This will start parsing \"receive.foobar.hiderefs\", which we don't want.\n>> I think you need:\n>> \n>>   !parse_config_key(var, section, &subsection, &subsection_len, &key) &&\n>>   !subsection &&\n>>   !strcmp(key, \"hiderefs\")\n>> \n>> Perhaps passing NULL for the subsection variable should cause\n>> parse_config_key to return failure when there is a non-empty subsection.\n>> \n>> -Peff\n>> \n>> PS Outside of parse_config_key, this code would be nicer if it used\n>>    skip_prefix() instead of starts_with(). Since it's going away, I\n>>    don't think it matters, but I note that parse_config_key could\n>>    probably benefit from the same.\n>\n> While I'm thinking about it, here are patches to do that. The third one\n> I'd probably squash into yours (after ordering it to the end).\n>\n>   [1/3]: parse_config_key: use skip_prefix instead of starts_with\n>   [2/3]: parse_config_key: allow matching single-level config\n>   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection\n\nWhile you were doing that, I was grepping the call sites for\nparse_config_key() and made sure that all of them are OK when fed\ntwo level names.  Most of them follow this pattern:\n\n\tif (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n\t\treturn -1;\n\nand ones that do not immediately check !name does either eventually\ndo so or have separate codepaths for handlihng two- and three-level\nnames.\n"},{"id":"312566","messageId":"20170224212025.hcxusnrxijzb5qox@sigill.intra.peff.net","threadId":"45216","inReplyTo":"xmqq4lzj1e4n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:20:25Z","receivedAt":"2017-02-24T21:20:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:\n\n> > While I'm thinking about it, here are patches to do that. The third one\n> > I'd probably squash into yours (after ordering it to the end).\n> >\n> >   [1/3]: parse_config_key: use skip_prefix instead of starts_with\n> >   [2/3]: parse_config_key: allow matching single-level config\n> >   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection\n> \n> While you were doing that, I was grepping the call sites for\n> parse_config_key() and made sure that all of them are OK when fed\n> two level names.  Most of them follow this pattern:\n> \n> \tif (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n> \t\treturn -1;\n> \n> and ones that do not immediately check !name does either eventually\n> do so or have separate codepaths for handlihng two- and three-level\n> names.\n\nYeah, I did that, too. :)\n\nI don't think there are any other sites to convert. And I don't think we\ncan make the \"!name\" case easier (because some call-sites do want to\nhandle both types). And it's not like it gets much easier anyway (unlike\nthe opposite case, you _do_ need to declare the extra variables.\n\n-Peff\n"},{"id":"312567","messageId":"xmqqzihbz3nz.fsf@gitster.mtv.corp.google.com","threadId":"45216","inReplyTo":"20170224210802.rpr5vdpqhsp3pt5v@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] parse_config_key: allow matching single-level config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T21:20:48Z","receivedAt":"2017-02-24T21:20:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> The parse_config_key() function was introduced to make it\n> easier to match \"section.subsection.key\" variables. It also\n> handles the simpler \"section.key\", and the caller is\n> responsible for distinguishing the two from its\n> out-parameters.\n>\n> Most callers who _only_ want \"section.key\" would just use a\n> strcmp(var, \"section.key\"), since there is no parsing\n> required. However, they may still use parse_config_key() if\n> their \"section\" variable isn't a constant (an example of\n> this is in parse_hide_refs_config).\n\nPerhaps \"only\" at the end of the title?\n\nAfter grepping for call sites of this function, I think we can\nsimplify quite a few instances of:\n\n\tif (parse_config_key(...) || !name)\n\t\treturn ...;\n\n"},{"id":"312568","messageId":"xmqqvarzz3hl.fsf@gitster.mtv.corp.google.com","threadId":"45216","inReplyTo":"20170224212025.hcxusnrxijzb5qox@sigill.intra.peff.net","subject":"Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T21:24:38Z","receivedAt":"2017-02-24T21:25:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:\n>\n>> > While I'm thinking about it, here are patches to do that. The third one\n>> > I'd probably squash into yours (after ordering it to the end).\n>> >\n>> >   [1/3]: parse_config_key: use skip_prefix instead of starts_with\n>> >   [2/3]: parse_config_key: allow matching single-level config\n>> >   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection\n>> \n>> While you were doing that, I was grepping the call sites for\n>> parse_config_key() and made sure that all of them are OK when fed\n>> two level names.  Most of them follow this pattern:\n>> \n>> \tif (parse_config_key(k, \"diff\", &name, &namelen, &type) || !name)\n>> \t\treturn -1;\n>> \n>> and ones that do not immediately check !name does either eventually\n>> do so or have separate codepaths for handlihng two- and three-level\n>> names.\n>\n> Yeah, I did that, too. :)\n>\n> I don't think there are any other sites to convert. And I don't think we\n> can make the \"!name\" case easier (because some call-sites do want to\n> handle both types). And it's not like it gets much easier anyway (unlike\n> the opposite case, you _do_ need to declare the extra variables.\n\nYeah, because the rejection of !name as an error is not the only\nreason these call sites have &name and &namelen---they want to use\nthat subsection name after that if() statement rejects malformed\ninput, so we cannot really lose them.\n\nThanks.  All three looked good.\n"},{"id":"312569","messageId":"xmqqr32nz3fn.fsf@gitster.mtv.corp.google.com","threadId":"45216","inReplyTo":"xmqqzihbz3nz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] parse_config_key: allow matching single-level config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-02-24T21:25:48Z","receivedAt":"2017-02-24T21:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> The parse_config_key() function was introduced to make it\n>> easier to match \"section.subsection.key\" variables. It also\n>> handles the simpler \"section.key\", and the caller is\n>> responsible for distinguishing the two from its\n>> out-parameters.\n>>\n>> Most callers who _only_ want \"section.key\" would just use a\n>> strcmp(var, \"section.key\"), since there is no parsing\n>> required. However, they may still use parse_config_key() if\n>> their \"section\" variable isn't a constant (an example of\n>> this is in parse_hide_refs_config).\n>\n> Perhaps \"only\" at the end of the title?\n\nwhich still stands.  My initial reaction was \"eh, I didn't know it\nwas an error to call the function for two-level names\".\n\n> After grepping for call sites of this function, I think we can\n> simplify quite a few instances of:\n>\n> \tif (parse_config_key(...) || !name)\n> \t\treturn ...;\n\nThis is false.  Sorry for the noise.\n"},{"id":"312570","messageId":"20170224212618.qwegffvga4p4n4o7@sigill.intra.peff.net","threadId":"45216","inReplyTo":"xmqqzihbz3nz.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/3] parse_config_key: allow matching single-level config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-24T21:26:18Z","receivedAt":"2017-02-24T21:28:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 24, 2017 at 01:20:48PM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > The parse_config_key() function was introduced to make it\n> > easier to match \"section.subsection.key\" variables. It also\n> > handles the simpler \"section.key\", and the caller is\n> > responsible for distinguishing the two from its\n> > out-parameters.\n> >\n> > Most callers who _only_ want \"section.key\" would just use a\n> > strcmp(var, \"section.key\"), since there is no parsing\n> > required. However, they may still use parse_config_key() if\n> > their \"section\" variable isn't a constant (an example of\n> > this is in parse_hide_refs_config).\n> \n> Perhaps \"only\" at the end of the title?\n\nYeah, that would be an improvement.\n\n> After grepping for call sites of this function, I think we can\n> simplify quite a few instances of:\n> \n> \tif (parse_config_key(...) || !name)\n> \t\treturn ...;\n\nI think you figured this out from your other response, but no, those are\nthe opposite case (it tricked me at first, too).\n\n-Peff\n"},{"id":"312580","messageId":"CAGZ79kbR4iHnFmhpk0qZurN2sdMnk3kukMAfZ_KitZHx3X-wbA@mail.gmail.com","threadId":"45216","inReplyTo":"20170224210816.ftcfa4jy4wltv2b7@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-02-24T23:28:07Z","receivedAt":"2017-02-24T23:28:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Feb 24, 2017 at 1:08 PM, Jeff King <peff@peff.net> wrote:\n> +           (!parse_config_key(var, section, NULL, NULL, &key) &&\n> +            !strcmp(key, \"hiderefs\"))) {\n\nHeh, see how fast my code gets replaced with even better code? ;)\nAll three patches look good,\n\nThanks,\nStefan\n"}]}