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

From: Jeff King <peff@peff.net>
Date: Jul 7, 2025, 20:28 UTC
Message-ID: <20250707202801.GA3115893@coredump.intra.peff.net>
In-Reply-To: <aGuP3Q5xykmRNp0m@pks.im>

On Mon, Jul 07, 2025 at 11:14:05AM +0200, Patrick Steinhardt wrote:

Show 6 quoted lines

> > +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`.

Yeah, it's for "void". This is a pattern used elsewhere for callbacks (usually as "vdata", but here we didn't need a container struct since there's only one item). I think "payload" is not a term we usually use, but maybe just "data" would be the usual thing (we only need "vdata" when we're assigning to the non-void data type).

IMHO we should probably avoid the underscore pattern. It's OK here, but it runs close to violating the reserved names rules (a global variable variable _name is bad, and _Name anywhere is bad).

Show 20 quoted lines

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

AFAICT "remote add" allows anything that parses as a refspec, which implies that refs/remotes/<name>/ passes check_refname_format(). And we don't allow backslashes there:

  $ git remote add foo/bar url
  [no output, $? is 0]
  $ git remote add 'bar\foo' url
  fatal: 'bar\foo' is not a valid remote name

I don't think this is platform dependent. It's coming from the refname_disposition table, so we're not calling is_dir_sep(). Only '/' is marked in that table as end-of-component, and "\\" is forbidden.

So I don't think we need to worry about backslashes here.

-Peff