Jeff King <peff@xxxxxxxx> 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.
