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

  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

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

  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.

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.

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

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

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

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

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

This lets us avoid declaring some otherwise useless
variables.

Signed-off-by: Jeff King <redacted>
---
 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

On Fri, Feb 24, 2017 at 1:08 PM, Jeff King [off-list ref] 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