Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Jeff King]

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)
>   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.

-Peff

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Jeff King  writes:

> 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)
>>  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.

Yeah, because the rejection of !name as an error is not the only
reason these call sites have  and  want to use
that subsection name after that if() statement rejects malformed
input, so we cannot really lose them.

Thanks.  All three looked good.

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Jeff King  writes:

> 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, , _len, ) &&
>>   !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 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)
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.

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Jeff King]

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, , _len, ) &&
>   !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

 cache.h  |  5 -
 config.c | 15 +--
 refs.c   |  7 +++
 3 files changed, 16 insertions(+), 11 deletions(-)

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Stefan Beller  writes:

> 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 
> ---
>  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, , _len, )
> + && !subsection && !strcmp(key, "hiderefs"))) {
>   char *ref;
>   int len;

Thanks for noticing.  "once both are merged" is a cryptic comment to
leave when somebody knows which two topics make "both" ;-)

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Jeff King]

On Fri, Feb 24, 2017 at 12:43:35PM -0800, Stefan Beller wrote:

> 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, , _len, )
> + && !subsection && !strcmp(key, "hiderefs"))) {

Yeah, this one looks fine.

-Peff

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Jeff King]

On Fri, Feb 24, 2017 at 12:33:35PM -0800, Stefan Beller wrote:

> 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 
> ---
> 
>   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?

No, I think the reasoning you gave in the commit message is exactly
what happened.

> 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, , _len, )
> + && !strcmp(key, "hiderefs"))) {

This will start parsing "receive.foobar.hiderefs", which we don't want.
I think you need:

  !parse_config_key(var, section, , _len, ) &&
  !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.