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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html