Re: [PATCH v5 1/5] hiderefs: add hide-refs hook to hide refs dynamically

[Junio C Hamano <gitster@pobox.com>]

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

> The first conditional to parse the configuration variable name var
> tries to handle both generic transfer.hideRefs and direction
> specific {receive,uploadpack}.hideRefs and "section" at this point
> has "transfer", "receive" or "uploadpack", doesn't it?

This part of my review comments was based on my misreading the
patch.  Sorry.  "section" comes from the caller and says either
"receive" or "uploadpack".

But because the updating of hide_refs_section is done outside that
"first conditional" that makes sure we are actually looking at a
"*.hiderefs" configuration variable, it is misleading.  The only
saving grace is that it is guarded by "hook"

> +	if (hook && hide_refs_section.len == 0)
> +		strbuf_addstr(&hide_refs_section, section);
> +

that is only set inside the body of the if statement of the first
conditional that ensures that we are reading *.hiderefs variable,
but it would make more sense to move it inside it.

Or even better would be to clean the function up with a preliminary
patch to return early when we are not looking at *.hiderefs variable,
perhaps like the attached, and then build on top.

 refs.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git i/refs.c w/refs.c
index c89d558892..8cf8c58ebd 100644
--- i/refs.c
+++ w/refs.c
@@ -1393,24 +1393,29 @@ static struct string_list *hide_refs;
 int parse_hide_refs_config(const char *var, const char *value, const char *section)
 {
 	const char *key;
-	if (!strcmp("transfer.hiderefs", var) ||
-	    (!parse_config_key(var, section, NULL, NULL, &key) &&
-	     !strcmp(key, "hiderefs"))) {
-		char *ref;
-		int len;
-
-		if (!value)
-			return config_error_nonbool(var);
-		ref = xstrdup(value);
-		len = strlen(ref);
-		while (len && ref[len - 1] == '/')
-			ref[--len] = '\0';
-		if (!hide_refs) {
-			CALLOC_ARRAY(hide_refs, 1);
-			hide_refs->strdup_strings = 1;
-		}
-		string_list_append(hide_refs, ref);
+	char *ref;
+	int len;
+
+	/*
+	 * "section" is either "receive" or "uploadpack"; are we looking
+	 * at transfer.hiderefs or $section.hiderefs?
+	 */
+	if (strcmp("transfer.hiderefs", var) &&
+	    !(!parse_config_key(var, section, NULL, NULL, &key) &&
+	      !strcmp(key, "hiderefs")))
+		return 0; /* neither */
+	if (!value)
+		return config_error_nonbool(var);
+	ref = xstrdup(value);
+	len = strlen(ref);
+	while (len && ref[len - 1] == '/')
+		ref[--len] = '\0';
+	if (!hide_refs) {
+		CALLOC_ARRAY(hide_refs, 1);
+		hide_refs->strdup_strings = 1;
 	}
+	string_list_append(hide_refs, ref);
+
 	return 0;
 }