patchRe: [PATCH] refs: parse_hide_refs_config to use parse_config_key

> parse_config_key was introduced in 1b86bbb0ade (config: add helper
> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed
> in this patch was introduced at daebaa7813 (upload/receive-pack: allow
> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,
> so presumably the code replaced in this patch was only introduced due
> to not wanting to wait on the proper helper function being available.
> 
> Make the condition easier to read by using parse_config_key.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> 
>   When investigating the state of the art for parsing config options, I saw
>   opportunity for a small drive-by patch in an area that I did not look at for 
>   a long time.
>   
>   The authors of the two mentioned commits are Jeff and Junio, so maybe you
>   remember another reason for this NEEDSWORK here?

> diff --git a/refs.c b/refs.c
> index cd36b64ed9..c9e5f13630 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;
>  
>  int parse_hide_refs_config(const char *var, const char *value, const char *section)
>  {
> +	const char *subsection, *key;
> +	int subsection_len;
>  	if (!strcmp("transfer.hiderefs", var) ||
> -	    /* NEEDSWORK: use parse_config_key() once both are merged */
> -	    (starts_with(var, section) && var[strlen(section)] == '.' &&
> -	     !strcmp(var + strlen(section), ".hiderefs"))) {
> +	    (!parse_config_key(var, section, &subsection, &subsection_len, &key)
> +	    && !strcmp(key, "hiderefs"))) {

> parse_config_key was introduced in 1b86bbb0ade (config: add helper
> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed
> in this patch was introduced at daebaa7813 (upload/receive-pack: allow
> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,
> so presumably the code replaced in this patch was only introduced due
> to not wanting to wait on the proper helper function being available.
> 
> Make the condition easier to read by using parse_config_key.
> [...]
>  	if (!strcmp("transfer.hiderefs", var) ||
> -	    /* NEEDSWORK: use parse_config_key() once both are merged */
> -	    (starts_with(var, section) && var[strlen(section)] == '.' &&
> -	     !strcmp(var + strlen(section), ".hiderefs"))) {
> +	    (!parse_config_key(var, section, &subsection, &subsection_len, &key)
> +	    && !subsection && !strcmp(key, "hiderefs"))) {

> parse_config_key was introduced in 1b86bbb0ade (config: add helper
> function for parsing key names, 2013-01-22), the NEEDSWORK that is removed
> in this patch was introduced at daebaa7813 (upload/receive-pack: allow
> hiding ref hierarchies, 2013-01-18), which is only a couple days apart,
> so presumably the code replaced in this patch was only introduced due
> to not wanting to wait on the proper helper function being available.
>
> Make the condition easier to read by using parse_config_key.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  refs.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index cd36b64ed9..21bc8c9101 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1034,10 +1034,11 @@ static struct string_list *hide_refs;
>  
>  int parse_hide_refs_config(const char *var, const char *value, const char *section)
>  {
> +	const char *subsection, *key;
> +	int subsection_len;
>  	if (!strcmp("transfer.hiderefs", var) ||
> -	    /* NEEDSWORK: use parse_config_key() once both are merged */
> -	    (starts_with(var, section) && var[strlen(section)] == '.' &&
> -	     !strcmp(var + strlen(section), ".hiderefs"))) {
> +	    (!parse_config_key(var, section, &subsection, &subsection_len, &key)
> +	    && !subsection && !strcmp(key, "hiderefs"))) {
>  		char *ref;
>  		int len;

> This will start parsing "receive.foobar.hiderefs", which we don't want.
> I think you need:
> 
>   !parse_config_key(var, section, &subsection, &subsection_len, &key) &&
>   !subsection &&
>   !strcmp(key, "hiderefs")
> 
> Perhaps passing NULL for the subsection variable should cause
> parse_config_key to return failure when there is a non-empty subsection.
> 
> -Peff
> 
> PS Outside of parse_config_key, this code would be nicer if it used
>    skip_prefix() instead of starts_with(). Since it's going away, I
>    don't think it matters, but I note that parse_config_key could
>    probably benefit from the same.

> The parse_config_key() function was introduced to make it
> easier to match "section.subsection.key" variables. It also
> handles the simpler "section.key", and the caller is
> responsible for distinguishing the two from its
> out-parameters.
>
> Most callers who _only_ want "section.key" would just use a
> strcmp(var, "section.key"), since there is no parsing
> required. However, they may still use parse_config_key() if
> their "section" variable isn't a constant (an example of
> this is in parse_hide_refs_config).

> Jeff King <peff@peff.net> writes:
>
>> The parse_config_key() function was introduced to make it
>> easier to match "section.subsection.key" variables. It also
>> handles the simpler "section.key", and the caller is
>> responsible for distinguishing the two from its
>> out-parameters.
>>
>> Most callers who _only_ want "section.key" would just use a
>> strcmp(var, "section.key"), since there is no parsing
>> required. However, they may still use parse_config_key() if
>> their "section" variable isn't a constant (an example of
>> this is in parse_hide_refs_config).
>
> Perhaps "only" at the end of the title?

> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
>
> 	if (parse_config_key(...) || !name)
> 		return ...;

> Jeff King <peff@peff.net> writes:
> 
> > The parse_config_key() function was introduced to make it
> > easier to match "section.subsection.key" variables. It also
> > handles the simpler "section.key", and the caller is
> > responsible for distinguishing the two from its
> > out-parameters.
> >
> > Most callers who _only_ want "section.key" would just use a
> > strcmp(var, "section.key"), since there is no parsing
> > required. However, they may still use parse_config_key() if
> > their "section" variable isn't a constant (an example of
> > this is in parse_hide_refs_config).
> 
> Perhaps "only" at the end of the title?

> After grepping for call sites of this function, I think we can
> simplify quite a few instances of:
> 
> 	if (parse_config_key(...) || !name)
> 		return ...;

> On Fri, Feb 24, 2017 at 03:39:40PM -0500, Jeff King wrote:
>
>> This will start parsing "receive.foobar.hiderefs", which we don't want.
>> I think you need:
>> 
>>   !parse_config_key(var, section, &subsection, &subsection_len, &key) &&
>>   !subsection &&
>>   !strcmp(key, "hiderefs")
>> 
>> Perhaps passing NULL for the subsection variable should cause
>> parse_config_key to return failure when there is a non-empty subsection.
>> 
>> -Peff
>> 
>> PS Outside of parse_config_key, this code would be nicer if it used
>>    skip_prefix() instead of starts_with(). Since it's going away, I
>>    don't think it matters, but I note that parse_config_key could
>>    probably benefit from the same.
>
> While I'm thinking about it, here are patches to do that. The third one
> I'd probably squash into yours (after ordering it to the end).
>
>   [1/3]: parse_config_key: use skip_prefix instead of starts_with
>   [2/3]: parse_config_key: allow matching single-level config
>   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection

> > While I'm thinking about it, here are patches to do that. The third one
> > I'd probably squash into yours (after ordering it to the end).
> >
> >   [1/3]: parse_config_key: use skip_prefix instead of starts_with
> >   [2/3]: parse_config_key: allow matching single-level config
> >   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
> 
> While you were doing that, I was grepping the call sites for
> parse_config_key() and made sure that all of them are OK when fed
> two level names.  Most of them follow this pattern:
> 
> 	if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
> 		return -1;
> 
> and ones that do not immediately check !name does either eventually
> do so or have separate codepaths for handlihng two- and three-level
> names.

> On Fri, Feb 24, 2017 at 01:18:48PM -0800, Junio C Hamano wrote:
>
>> > While I'm thinking about it, here are patches to do that. The third one
>> > I'd probably squash into yours (after ordering it to the end).
>> >
>> >   [1/3]: parse_config_key: use skip_prefix instead of starts_with
>> >   [2/3]: parse_config_key: allow matching single-level config
>> >   [3/3]: parse_hide_refs_config: tell parse_config_key we don't want a subsection
>> 
>> While you were doing that, I was grepping the call sites for
>> parse_config_key() and made sure that all of them are OK when fed
>> two level names.  Most of them follow this pattern:
>> 
>> 	if (parse_config_key(k, "diff", &name, &namelen, &type) || !name)
>> 		return -1;
>> 
>> and ones that do not immediately check !name does either eventually
>> do so or have separate codepaths for handlihng two- and three-level
>> names.
>
> Yeah, I did that, too. :)
>
> I don't think there are any other sites to convert. And I don't think we
> can make the "!name" case easier (because some call-sites do want to
> handle both types). And it's not like it gets much easier anyway (unlike
> the opposite case, you _do_ need to declare the extra variables.