Re: [PATCH] remote: detect collisions in remote names

From: Patrick Steinhardt <ps@pks.im>
Date: Jul 7, 2025, 09:14 UTC
Message-ID: <aGuP3Q5xykmRNp0m@pks.im>
In-Reply-To: <20250705185842.GA2496172@coredump.intra.peff.net>

On Sat, Jul 05, 2025 at 02:58:42PM -0400, Jeff King wrote:

Show 12 quoted lines

> On Sat, Jul 05, 2025 at 12:57:50PM -0400, Jeff King wrote:
> 
> > So I dunno. It feels like a configuration error in most cases, but not
> > all. I'd probably say that people touching the config manually should be
> > allowed to do what they want, but maybe "git remote" should be a bit
> > more careful about names being proper subsets of existing remotes (it
> > should already prevent the exact-match above, I'd think, because the ref
> > namespace it uses will always match the configuration name).
> 
> So I'm not entirely convinced we should do anything here. The answer
> might just be "if it hurts, don't do it". But if we wanted any
> protections in the "git remote" porcelain, they might look like this:

I think having these protections is sensible. And I also agree with you that we shouldn't keep people from doing weird things by manipulating the config directly.

Show 9 quoted lines

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 0d6755bcb7..b18730ddb2 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -157,6 +157,21 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)
>  	return 0;
>  }
>  
> +static int check_remote_collision(struct remote *remote, void *vname)

Tiniest nit: I was a bit puzzled what the `v` in `vname` stands for, and it took a while until I noticed that it probably stands for `void`. If you end up rerolling, I'd suggest to either call this `payload` or `_name`.

Show 14 quoted lines

> +{
> +	const char *name = vname;
> +	const char *p;
> +
> +	if (skip_prefix(name, remote->name, &p) && *p == '/')
> +		die(_("remote name '%s' is a subset of existing remote '%s'"),
> +		    name, remote->name);
> +	if (skip_prefix(remote->name, name, &p) && *p == '/')
> +		die(_("remote name '%s' is a superset of existing remote '%s'"),
> +		    name, remote->name);
> +
> +	return 0;
> +}
> +

Hm. Do we have to care about '\' on Windows, as well? This made me rediscover the following function:

    static int valid_remote_nick(const char *name)
    {
    	if (!name[0] || is_dot_or_dotdot(name))
    		return 0;
    
    	/* remote nicknames cannot contain slashes */
    	while (*name)
    		if (is_dir_sep(*name++))
    			return 0;
    	return 1;
    }

Which... puzzled me a bit at first, as it seems to indicate that a remote with a path separator is invalid. But as it turns out we only use this function if remotes are configured via ".git/remotes" or ".git/branches". Looks like we eventually lost this limitation, probably when config-based remotes were introduced.

Show 9 quoted lines

> @@ -208,6 +223,8 @@ static int add(int argc, const char **argv, const char *prefix,
>  	if (!valid_remote_name(name))
>  		die(_("'%s' is not a valid remote name"), name);
>  
> +	for_each_remote(check_remote_collision, (void *)name);
> +
>  	strbuf_addf(&buf, "remote.%s.url", name);
>  	git_config_set(buf.buf, url);
>

Nice and simple.

Patrick