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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 07, 2025 at 11:14:05AM +0200, Patrick Steinhardt wrote:
>
>> > +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).

"name_" is available.  In fact I think it is a very common pattern
in this codebase to name an incoming parameter with trailing "_",
and assign it to a local variable with the right name and with the
right type at the top of the function.

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

That agrees with my understanding.  Thanks for carefully checking.