Re: [PATCH] refs: parse_hide_refs_config to use parse_config_key
From: Jeff King <hidden>
Date: 2017-02-24 20:39:48
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.
quoted hunk ↗ jump to hunk
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.