Re: [PATCH v2 1/4] remote: use parse_config_key

From: Jeff King <peff@peff.net>
Date: Feb 15, 2016, 23:04 UTC
Message-ID: <20160215230443.GB30631@sigill.intra.peff.net>
In-Reply-To: <1455575984-24348-2-git-send-email-t.gummerer@gmail.com>

On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:

Show 9 quoted lines

> -	if (!starts_with(key,  "remote."))
> +	if (parse_config_key(key, "remote", &name, &namelen, &subkey) < 0)
>  		return 0;
> -	name = key + 7;
>  
>  	/* Handle remote.* variables */
> -	if (!strcmp(name, "pushdefault"))
> +	if (!strcmp(subkey, "pushdefault"))
>  		return git_config_string(&pushremote_name, key, value);

I think this needs to become:

  if (!name && !strcmp(subkey, "pushdefault"))

so that we do not match "remote.foo.pushdefault", which is nonsense. The original avoided it by conflating "name" and "subkey" at various points, and not parsing out the subkey until later. Making that more explicit is one of the things that I think is improved by your patch. :)

Show 12 quoted lines

>  	/* Handle remote.<name>.* variables */
> -	if (*name == '/') {
> +	if (*(name ? name : subkey) == '/') {
>  		warning("Config remote shorthand cannot begin with '/': %s",
> -			name);
> +			name ? name : subkey);
>  		return 0;
>  	}
> -	subkey = strrchr(name, '.');
> -	if (!subkey)
> +	if (!name)
>  		return 0;

I think you can bump the "if (!name)" check earlier. If it is empty, we know that it does not start with "/". And then you can avoid the extra NULL-checks.

The rest of the patch looks good to me. I hadn't realized initially that all of the subkey compares would become "foo" and not ".foo". That makes the diff noisier, but IMHO the result is much better.

-Peff