On Sat, Jul 05, 2025 at 02:58:42PM -0400, Jeff King wrote:
> 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.
> 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`.
> +{
> + 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.
> @@ -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
