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@xxxxxxxxxx>
> ---
>
> 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.