[PATCH] refs: parse_hide_refs_config to use parse_config_key

[PATCH] refs: parse_hide_refs_config to use parse_config_key

[Stefan Beller]

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?
  
  Thanks,
  Stefan
  
 refs.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

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"))) {
 		char *ref;
 		int len;
 
-- 
2.12.0.rc1.16.ge4278d41a0.dirty

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

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

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.

[PATCH] refs: parse_hide_refs_config to use parse_config_key

[Stefan Beller]

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;
 
-- 
2.12.0.rc1.16.ge4278d41a0.dirty

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

Yeah, this one looks fine.

-Peff

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Stefan Beller <sbeller@google.com> 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 <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;

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

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

[PATCH 1/3] parse_config_key: use skip_prefix instead of starts_with

[Jeff King]

This saves us having to repeatedly add in "section_len" (and
also avoids walking over the first part of the string
multiple times for a strlen() and strrchr()).

Signed-off-by: Jeff King <peff@peff.net>
---
 config.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/config.c b/config.c
index c6b874a7b..1b08a75a7 100644
--- a/config.c
+++ b/config.c
@@ -2536,11 +2536,10 @@ int parse_config_key(const char *var,
 		     const char **subsection, int *subsection_len,
 		     const char **key)
 {
-	int section_len = strlen(section);
 	const char *dot;
 
 	/* Does it start with "section." ? */
-	if (!starts_with(var, section) || var[section_len] != '.')
+	if (!skip_prefix(var, section, &var) || *var != '.')
 		return -1;
 
 	/*
@@ -2552,12 +2551,12 @@ int parse_config_key(const char *var,
 	*key = dot + 1;
 
 	/* Did we have a subsection at all? */
-	if (dot == var + section_len) {
+	if (dot == var) {
 		*subsection = NULL;
 		*subsection_len = 0;
 	}
 	else {
-		*subsection = var + section_len + 1;
+		*subsection = var + 1;
 		*subsection_len = dot - *subsection;
 	}
 
-- 
2.12.0.616.g5f622f3b1

[PATCH 2/3] parse_config_key: allow matching single-level config

[Jeff King]

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

Using the parse_config_key is a bit clunky, though:

  const char *subsection;
  int subsection_len;
  const char *key;

  if (!parse_config_key(var, section, &subsection, &subsection_len, &key) &&
      !subsection) {
	  /* matched! */
  }

Instead, let's treat a NULL subsection as an indication that
the caller does not expect one. That lets us write:

  const char *key;

  if (!parse_config_key(var, section, NULL, NULL, &key)) {
	  /* matched! */
  }

Existing callers should be unaffected, as passing a NULL
subsection would currently segfault.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h  | 5 ++++-
 config.c | 8 ++++++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 61fc86e6d..647a78f3f 100644
--- a/cache.h
+++ b/cache.h
@@ -1819,8 +1819,11 @@ extern int git_config_include(const char *name, const char *value, void *data);
  *
  * (i.e., what gets handed to a config_fn_t). The caller provides the section;
  * we return -1 if it does not match, 0 otherwise. The subsection and key
- * out-parameters are filled by the function (and subsection is NULL if it is
+ * out-parameters are filled by the function (and *subsection is NULL if it is
  * missing).
+ *
+ * If the subsection pointer-to-pointer passed in is NULL, returns 0 only if
+ * there is no subsection at all.
  */
 extern int parse_config_key(const char *var,
 			    const char *section,
diff --git a/config.c b/config.c
index 1b08a75a7..13c8b21ea 100644
--- a/config.c
+++ b/config.c
@@ -2552,10 +2552,14 @@ int parse_config_key(const char *var,
 
 	/* Did we have a subsection at all? */
 	if (dot == var) {
-		*subsection = NULL;
-		*subsection_len = 0;
+		if (subsection) {
+			*subsection = NULL;
+			*subsection_len = 0;
+		}
 	}
 	else {
+		if (!subsection)
+			return -1;
 		*subsection = var + 1;
 		*subsection_len = dot - *subsection;
 	}
-- 
2.12.0.616.g5f622f3b1

[PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection

[Jeff King]

This lets us avoid declaring some otherwise useless
variables.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 21bc8c910..b9188908b 100644
--- a/refs.c
+++ b/refs.c
@@ -1034,11 +1034,10 @@ 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;
+	const char *key;
 	if (!strcmp("transfer.hiderefs", var) ||
-	    (!parse_config_key(var, section, &subsection, &subsection_len, &key)
-	    && !subsection && !strcmp(key, "hiderefs"))) {
+	    (!parse_config_key(var, section, NULL, NULL, &key) &&
+	     !strcmp(key, "hiderefs"))) {
 		char *ref;
 		int len;
 
-- 
2.12.0.616.g5f622f3b1

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Jeff King <peff@peff.net> 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, &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 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.

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, &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.

-Peff

Re: [PATCH 2/3] parse_config_key: allow matching single-level config

[Junio C Hamano]

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

Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key

[Junio C Hamano]

Jeff King <peff@peff.net> 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, &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.

Yeah, because the rejection of !name as an error is not the only
reason these call sites have &name and &namelen---they 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 2/3] parse_config_key: allow matching single-level config

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

which still stands.  My initial reaction was "eh, I didn't know it
was an error to call the function for two-level names".

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

This is false.  Sorry for the noise.

Re: [PATCH 2/3] parse_config_key: allow matching single-level config

[Jeff King]

On Fri, Feb 24, 2017 at 01:20:48PM -0800, Junio C Hamano wrote:

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

Yeah, that would be an improvement.

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

I think you figured this out from your other response, but no, those are
the opposite case (it tricked me at first, too).

-Peff

Re: [PATCH 3/3] parse_hide_refs_config: tell parse_config_key we don't want a subsection

[Stefan Beller]

On Fri, Feb 24, 2017 at 1:08 PM, Jeff King <peff@peff.net> wrote:
> +           (!parse_config_key(var, section, NULL, NULL, &key) &&
> +            !strcmp(key, "hiderefs"))) {

Heh, see how fast my code gets replaced with even better code? ;)
All three patches look good,

Thanks,
Stefan