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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 11:39:41PM +0100, Thomas Gummerer wrote:
>
> > -	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. :)

Good catch.  I'll fix this in the re-roll.

> >  	/* 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.

Thanks, will change that.

> 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

--
Thomas