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