Allowing "/" in the name of a git remote is a strange choice

Allowing "/" in the name of a git remote is a strange choice

[Per Cederqvist]

Today I realized that git accepts "/" in a remote name.

This can lead to problems. I have a repository that contains a branch
called "master" and another called "chat/master". Just for fun, I
added a second remote in this repository and named it
"origin/chat".

Now, does "refs/remotes/origin/chat/master" refer to the branch
"chat/master" from "origin", or the branch "master" from
"origin/chat"? Git seems to think it refers to both:

> $ git fetch --all
> Fetching origin
> From $PRIVATE_URL
>  + 4e31956300f...30e26ebbb19 chat/master -> origin/chat/master  (forced update)
> Fetching origin/chat
> From  $PRIVATE_URL
>  + 30e26ebbb19...4e31956300f master     -> origin/chat/master  (forced update)

Every time I run "git fetch --all" git updates the origin/chat/master ref twice.

If it was up to me, I'd add a check to valid_remote_name() to ensure
the name doesn't contain any "/" character.  I doubt it is used often.

    /ceder

Re: Allowing "/" in the name of a git remote is a strange choice

[Junio C Hamano]

Per Cederqvist <ceder@lysator.liu.se> writes:

> Today I realized that git accepts "/" in a remote name.
>
> This can lead to problems. I have a repository that contains a branch
> called "master" and another called "chat/master". Just for fun, I
> added a second remote in this repository and named it
> "origin/chat".
>
> Now, does "refs/remotes/origin/chat/master" refer to the branch
> "chat/master" from "origin", or the branch "master" from
> "origin/chat"? Git seems to think it refers to both:

That would have been a fun experiment ;-)

> If it was up to me, I'd add a check to valid_remote_name() to ensure
> the name doesn't contain any "/" character.  I doubt it is used often.

If your remote-naming discipline is to always use two-levels
(e.g. origin/chat, origin/chien, origin/lapin but never origin or
origin/chat/blanc mixed in), then there is no confusion.

It becomes only confusing if you mix origin and origin/chat.  

So it is not like we can just forbid '/' retroactively and expect no
repercussions, especially given that I hear there are more than a
few thousands of existing Git users in the world.

Re: Allowing "/" in the name of a git remote is a strange choice

[Patrick Steinhardt]

On Thu, Jul 03, 2025 at 09:51:12PM -0700, Junio C Hamano wrote:
> Per Cederqvist <ceder@lysator.liu.se> writes:
> 
> > Today I realized that git accepts "/" in a remote name.
> >
> > This can lead to problems. I have a repository that contains a branch
> > called "master" and another called "chat/master". Just for fun, I
> > added a second remote in this repository and named it
> > "origin/chat".
> >
> > Now, does "refs/remotes/origin/chat/master" refer to the branch
> > "chat/master" from "origin", or the branch "master" from
> > "origin/chat"? Git seems to think it refers to both:
> 
> That would have been a fun experiment ;-)
> 
> > If it was up to me, I'd add a check to valid_remote_name() to ensure
> > the name doesn't contain any "/" character.  I doubt it is used often.
> 
> If your remote-naming discipline is to always use two-levels
> (e.g. origin/chat, origin/chien, origin/lapin but never origin or
> origin/chat/blanc mixed in), then there is no confusion.
> 
> It becomes only confusing if you mix origin and origin/chat.  
> 
> So it is not like we can just forbid '/' retroactively and expect no
> repercussions, especially given that I hear there are more than a
> few thousands of existing Git users in the world.

We cannot just blanket-disallow this now, true. But shouldn't Git be
able to detect this conflict, similar to how a user cannot have both
refs/heads/branch and refs/heads/branch/nested?

Patrick

Re: Allowing "/" in the name of a git remote is a strange choice

[Lidong Yan]

Patrick Steinhardt <ps@pks.im> writes:

> We cannot just blanket-disallow this now, true. But shouldn't Git be
> able to detect this conflict, similar to how a user cannot have both
> refs/heads/branch and refs/heads/branch/nested?

Perhaps we should forbid having two remotes where one is the directory base
(i.e., a prefix) of the other.

Re: Allowing "/" in the name of a git remote is a strange choice

[Lidong Yan]

Patrick Steinhardt <ps@pks.im> write:
> 
> We cannot just blanket-disallow this now, true. But shouldn't Git be
> able to detect this conflict, similar to how a user cannot have both
> refs/heads/branch and refs/heads/branch/nested?
> 

I also find `git fetch` works fine, but check out remote branch detect this
conflict

$ git branch --set-upstream-to=origin/chat/master chat/master

Gives

fatal: not tracking: ambiguous information for ref 'refs/remotes/origin/chat/master'
hint: There are multiple remotes whose fetch refspecs map to the remote
hint: tracking ref 'refs/remotes/origin/chat/master':
hint:   origin/chat
hint:   origin
hint: 
hint: This is typically a configuration error.
hint: 
hint: To support setting up tracking branches, ensure that
hint: different remotes' fetch refspecs map into different
hint: tracking namespaces

Re: Allowing "/" in the name of a git remote is a strange choice

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> So it is not like we can just forbid '/' retroactively and expect no
>> repercussions, especially given that I hear there are more than a
>> few thousands of existing Git users in the world.
>
> We cannot just blanket-disallow this now, true. But shouldn't Git be
> able to detect this conflict, similar to how a user cannot have both
> refs/heads/branch and refs/heads/branch/nested?

Yup.  Sorry but I should probably have not left it out, as that was
way too obvious an improved "solution", compared to "just forbid '/',
as I cannot imagine anybody using it".

Re: Allowing "/" in the name of a git remote is a strange choice

[Per Cederqvist]

On Fri, Jul 4, 2025 at 6:51 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Per Cederqvist <ceder@lysator.liu.se> writes:
>
> > Today I realized that git accepts "/" in a remote name.
> >
> > This can lead to problems. I have a repository that contains a branch
> > called "master" and another called "chat/master". Just for fun, I
> > added a second remote in this repository and named it
> > "origin/chat".
> >
> > Now, does "refs/remotes/origin/chat/master" refer to the branch
> > "chat/master" from "origin", or the branch "master" from
> > "origin/chat"? Git seems to think it refers to both:
>
> That would have been a fun experiment ;-)

It was. Luckily I figured this out while trying to deduce the allowed format
of a remote name by reading the source code, not while trying to understand
confusing behaviour from git.

> > If it was up to me, I'd add a check to valid_remote_name() to ensure
> > the name doesn't contain any "/" character.  I doubt it is used often.
>
> If your remote-naming discipline is to always use two-levels
> (e.g. origin/chat, origin/chien, origin/lapin but never origin or
> origin/chat/blanc mixed in), then there is no confusion.
>
> It becomes only confusing if you mix origin and origin/chat.
>
> So it is not like we can just forbid '/' retroactively and expect no
> repercussions, especially given that I hear there are more than a
> few thousands of existing Git users in the world.

I wonder how many use "/" in a remote name, though. My guess is
very few.

If you want to do anything about this, there are a few possible ways:

- forbid "/", but add a setting that allows it. Note that even if you forbid
  "/", existing clones will continue to work. It is only when you add a
  new remote that the name is checked.

- require that the number of "/" character in a remote is equal for all
   remotes in a particular clone

- deperecate "/" and start warning about it now, and forbid it after a
suitable period

    /ceder

Re: Allowing "/" in the name of a git remote is a strange choice

[Jeff King]

On Thu, Jul 03, 2025 at 09:33:20PM +0200, Per Cederqvist wrote:

> > $ git fetch --all
> > Fetching origin
> > From $PRIVATE_URL
> >  + 4e31956300f...30e26ebbb19 chat/master -> origin/chat/master  (forced update)
> > Fetching origin/chat
> > From  $PRIVATE_URL
> >  + 30e26ebbb19...4e31956300f master     -> origin/chat/master  (forced update)
> 
> Every time I run "git fetch --all" git updates the origin/chat/master ref twice.
> 
> If it was up to me, I'd add a check to valid_remote_name() to ensure
> the name doesn't contain any "/" character.  I doubt it is used often.

I think the "/" here is really just a special case of a more general
problem: overlapping fetch refspec destinations.

For example, try this:

  git init repo
  cd repo

  git init one
  git -C one commit --allow-empty -m foo

  git init two
  git -C two commit --allow-empty -m bar

  git config remote.one.url one
  git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
  git config remote.two.url two
  git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*

  git fetch --all

which gives similar output to what you showed above. Of course it's
easier to see here when the names are identical rather than one being a
prefix of the other. But it's fundamentally the same issue, and
forbidding "/" would not fix it.

We could perhaps detect these kinds of overlap, but I wonder:

  1. How expensive is it to do so, and when should we do it? Obviously
     for a handful of refs a quadratic approach is OK. But what if you
     had 10,000 remotes (this is not purely hypothetical; GitHub used to
     manage object migration in its fork networks with configured
     remotes, but hit enough performance issues to switch away from
     that). So I'd be hesitant to check this on every "git fetch".

  2. Is it something people actually want to do? It's certainly a
     _weird_ configuration, but I could imagine there being useful
     corner cases (e.g., one URL is an infrequently backup of the other,
     so you don't usually do "--all", or you set skipDefaultUpdate for
     one of them.

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

-Peff

[PATCH] remote: detect collisions in remote names

[Jeff King]

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:

-- >8 --
Subject: [PATCH] remote: detect collisions in remote names

When two remotes collide in the destinations of their fetch refspecs,
the results can be confusing. For example, in this silly example:

  git config remote.one.url [...]
  git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
  git config remote.two.url [...]
  git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
  git fetch --all

we may try to write to the same ref twice (once for each remote we're
fetching). There's also a more subtle version of this. If you have
remotes "outer/inner" and "outer", then the ref "inner/branch" on the
second remote will conflict with just "branch" on the former (they both
want to write to "refs/remotes/outer/inner/branch").

We probably don't want to forbid this kind of overlap completely. While
the results can be confusing, there are legitimate reasons to have
multiple refs write into the same namespace (e.g., if one is a "backup"
of the other that is rarely fetched from).

But it may be worth limiting the porcelain "git remote" command to avoid
this confusion. The example above cannot be done with "git remote",
because it always[1] matches the refspecs to the remote name, and you
can only have one instance of each remote name. But you can still
trigger the more subtle variant like this:

  git remote add outer [...]
  git remote add outer/inner [...]

So let's detect that kind of name collision (in both directions) and
forbid it. You can still do whatever you like by manipulating the config
directly, but this should prevent the most obvious foot-gun.

[1] Almost always. With the --mirror option, the resulting refspec will
    just write into "refs/*"; the remote name does not appear in the ref
    namespace at all.

    Our new "names must not overlap" rule is not necessary for that
    case, but it seems reasonable to enforce it consistently. We already
    require all remote names to be valid in the ref namespace, even
    though we won't ever use them in that context for --mirror remotes.

    Likewise, our new rule doesn't help with overlap here. Any two
    mirror remotes will always overlap (in fact, any mirror remote along
    with any other single one, since refs/remotes/ is a subset of the
    mirrored refs). I'm not sure this is worth worrying about, but if it
    is, we'd want an additional rule like "mirror remotes must be the
    only remote".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote.c  | 17 +++++++++++++++++
 t/t5505-remote.sh | 14 ++++++++++++++
 2 files changed, 31 insertions(+)

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)
+{
+	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;
+}
+
 static int add(int argc, const char **argv, const char *prefix,
 	       struct repository *repo UNUSED)
 {
@@ -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);
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index bef0250e89..2701eef85e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '
 	test_cmp expect actual
 '
 
+test_expect_success 'forbid adding subset of existing remote' '
+	test_when_finished "git remote rm outer" &&
+	git remote add outer url &&
+	test_must_fail git remote add outer/inner url 2>err &&
+	test_grep ".outer/inner. is a subset of existing remote .outer." err
+'
+
+test_expect_success 'forbid adding superset of existing remote' '
+	test_when_finished "git remote rm outer/inner" &&
+	git remote add outer/inner url &&
+	test_must_fail git remote add outer url 2>err &&
+	test_grep ".outer. is a superset of existing remote .outer/inner." err
+'
+
 test_done
-- 
2.50.0.438.g3b3bebd3e8

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

[Patrick Steinhardt]

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

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Mon, Jul 07, 2025 at 02:04:19PM -0700, Junio C Hamano wrote:

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

Yeah, that is legal and is a pattern we use (though I admit that I find
any underscores kind of ugly and easy to miss). I was curious how often
each pattern appeared:

  ["v" prefix: vdata, va, etc]
  $ git grep 'void \*v' '*.c' | wc -l
  51

  [leading underscore: _data, _a, etc]
  $ git grep 'void \*_' '*.c' | wc -l
  52

  [trailing underscore: mostly a_, b_ in comparators]
  $ git grep 'void \*[a-zA-Z0-9]_' '*.c'  | wc -l
  30

  [just calling it "data"]
  $ git grep 'void \*data' '*.c' | wc -l
  314

The last one is cheating a little because it catches function pointer
declarations, too, but grepping for "= data;" returns over a hundred
hits, too.

So that was mostly for fun, and I think any is OK. ;) But here is the
patch again with the void pointer just called "data".

Although I think we're all a bit lukewarm on the concept, I feel like it
won't hurt anything, isn't too much code, and disables a potential (if
somewhat rare) footgun. So probably worth doing?

-- >8 --
Subject: [PATCH] remote: detect collisions in remote names

When two remotes collide in the destinations of their fetch refspecs,
the results can be confusing. For example, in this silly example:

  git config remote.one.url [...]
  git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
  git config remote.two.url [...]
  git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
  git fetch --all

we may try to write to the same ref twice (once for each remote we're
fetching). There's also a more subtle version of this. If you have
remotes "outer/inner" and "outer", then the ref "inner/branch" on the
second remote will conflict with just "branch" on the former (they both
want to write to "refs/remotes/outer/inner/branch").

We probably don't want to forbid this kind of overlap completely. While
the results can be confusing, there are legitimate reasons to have
multiple refs write into the same namespace (e.g., if one is a "backup"
of the other that is rarely fetched from).

But it may be worth limiting the porcelain "git remote" command to avoid
this confusion. The example above cannot be done with "git remote",
because it always[1] matches the refspecs to the remote name, and you
can only have one instance of each remote name. But you can still
trigger the more subtle variant like this:

  git remote add outer [...]
  git remote add outer/inner [...]

So let's detect that kind of name collision (in both directions) and
forbid it. You can still do whatever you like by manipulating the config
directly, but this should prevent the most obvious foot-gun.

[1] Almost always. With the --mirror option, the resulting refspec will
    just write into "refs/*"; the remote name does not appear in the ref
    namespace at all.

    Our new "names must not overlap" rule is not necessary for that
    case, but it seems reasonable to enforce it consistently. We already
    require all remote names to be valid in the ref namespace, even
    though we won't ever use them in that context for --mirror remotes.

    Likewise, our new rule doesn't help with overlap here. Any two
    mirror remotes will always overlap (in fact, any mirror remote along
    with any other single one, since refs/remotes/ is a subset of the
    mirrored refs). I'm not sure this is worth worrying about, but if it
    is, we'd want an additional rule like "mirror remotes must be the
    only remote".

Signed-off-by: Jeff King <peff@peff.net>
---
Subject: [PATCH] remote: detect collisions in remote names

When two remotes collide in the destinations of their fetch refspecs,
the results can be confusing. For example, in this silly example:

  git config remote.one.url [...]
  git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
  git config remote.two.url [...]
  git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
  git fetch --all

we may try to write to the same ref twice (once for each remote we're
fetching). There's also a more subtle version of this. If you have
remotes "outer/inner" and "outer", then the ref "inner/branch" on the
second remote will conflict with just "branch" on the former (they both
want to write to "refs/remotes/outer/inner/branch").

We probably don't want to forbid this kind of overlap completely. While
the results can be confusing, there are legitimate reasons to have
multiple refs write into the same namespace (e.g., if one is a "backup"
of the other that is rarely fetched from).

But it may be worth limiting the porcelain "git remote" command to avoid
this confusion. The example above cannot be done with "git remote",
because it always[1] matches the refspecs to the remote name, and you
can only have one instance of each remote name. But you can still
trigger the more subtle variant like this:

  git remote add outer [...]
  git remote add outer/inner [...]

So let's detect that kind of name collision (in both directions) and
forbid it. You can still do whatever you like by manipulating the config
directly, but this should prevent the most obvious foot-gun.

[1] Almost always. With the --mirror option, the resulting refspec will
    just write into "refs/*"; the remote name does not appear in the ref
    namespace at all.

    Our new "names must not overlap" rule is not necessary for that
    case, but it seems reasonable to enforce it consistently. We already
    require all remote names to be valid in the ref namespace, even
    though we won't ever use them in that context for --mirror remotes.

    Likewise, our new rule doesn't help with overlap here. Any two
    mirror remotes will always overlap (in fact, any mirror remote along
    with any other single one, since refs/remotes/ is a subset of the
    mirrored refs). I'm not sure this is worth worrying about, but if it
    is, we'd want an additional rule like "mirror remotes must be the
    only remote".

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/remote.c  | 17 +++++++++++++++++
 t/t5505-remote.sh | 14 ++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/builtin/remote.c b/builtin/remote.c
index 0d6755bcb7..a770df669c 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 *data)
+{
+	const char *name = data;
+	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;
+}
+
 static int add(int argc, const char **argv, const char *prefix,
 	       struct repository *repo UNUSED)
 {
@@ -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);
 
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index bef0250e89..2701eef85e 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '
 	test_cmp expect actual
 '
 
+test_expect_success 'forbid adding subset of existing remote' '
+	test_when_finished "git remote rm outer" &&
+	git remote add outer url &&
+	test_must_fail git remote add outer/inner url 2>err &&
+	test_grep ".outer/inner. is a subset of existing remote .outer." err
+'
+
+test_expect_success 'forbid adding superset of existing remote' '
+	test_when_finished "git remote rm outer/inner" &&
+	git remote add outer/inner url &&
+	test_must_fail git remote add outer url 2>err &&
+	test_grep ".outer. is a superset of existing remote .outer/inner." err
+'
+
 test_done
-- 
2.50.1.488.g2a977559af

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

[Jeff King]

On Tue, Jul 08, 2025 at 06:59:47PM -0400, Jeff King wrote:

> Although I think we're all a bit lukewarm on the concept, I feel like it
> won't hurt anything, isn't too much code, and disables a potential (if
> somewhat rare) footgun. So probably worth doing?

> -- >8 --
> Subject: [PATCH] remote: detect collisions in remote names
> [...]
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Subject: [PATCH] remote: detect collisions in remote names

Sorry, I am apparently bad at using my editor.

I _think_ this will just apply correctly for you, since the duplicated
commit message is all after the "---".

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 07, 2025 at 02:04:19PM -0700, Junio C Hamano wrote:
>
>> > 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.
>
> Yeah, that is legal and is a pattern we use (though I admit that I find
> any underscores kind of ugly and easy to miss). I was curious how often
> each pattern appeared:
>
>   ["v" prefix: vdata, va, etc]
>   $ git grep 'void \*v' '*.c' | wc -l
>   51
>
>   [leading underscore: _data, _a, etc]
>   $ git grep 'void \*_' '*.c' | wc -l
>   52
>
>   [trailing underscore: mostly a_, b_ in comparators]
>   $ git grep 'void \*[a-zA-Z0-9]_' '*.c'  | wc -l
>   30

Only a single letter followed by an underscore, which may be
followed by more letters legal in names (like a_bcde)?

A more fair pattern may be something like

$ git grep 'void \*[A-Za-z_0-9]*_[^A-Za-z_0-9]' \*.c | wc -l
52

>   [just calling it "data"]
>   $ git grep 'void \*data' '*.c' | wc -l
>   314
>
> The last one is cheating a little because it catches function pointer
> declarations, too, but grepping for "= data;" returns over a hundred
> hits, too.

Also "void *cb_data" is fairly common, I think, as we have some
callback API functions.

$ git grep 'void \*cb_data' \*.c | wc -l
234

> So that was mostly for fun, and I think any is OK. ;) But here is the
> patch again with the void pointer just called "data".

Yeah, I think any would be fine.  I was a bit surprised that
v-something was so widely used, though, as I find that it makes the
least sense among all possibilities (and "data" is the distant
second, as it would become awkward when you have to have more than
one, like my_custom_cmp(void *left, void *right), if your rule says
that you must say "data").

> Although I think we're all a bit lukewarm on the concept, I feel like it
> won't hurt anything, isn't too much code, and disables a potential (if
> somewhat rare) footgun. So probably worth doing?

Even though it does not cover all cases, at least those coming from
"git remote" will be able to avoid surprises, so let me replace with
this version, wait for a few days for more inputs from others and
then mark it for 'next' if nobody sees any downsides.


> -- >8 --
> Subject: [PATCH] remote: detect collisions in remote names
>
> When two remotes collide in the destinations of their fetch refspecs,
> the results can be confusing. For example, in this silly example:
>
>   git config remote.one.url [...]
>   git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
>   git config remote.two.url [...]
>   git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
>   git fetch --all
>
> we may try to write to the same ref twice (once for each remote we're
> fetching). There's also a more subtle version of this. If you have
> remotes "outer/inner" and "outer", then the ref "inner/branch" on the
> second remote will conflict with just "branch" on the former (they both
> want to write to "refs/remotes/outer/inner/branch").
>
> We probably don't want to forbid this kind of overlap completely. While
> the results can be confusing, there are legitimate reasons to have
> multiple refs write into the same namespace (e.g., if one is a "backup"
> of the other that is rarely fetched from).
>
> But it may be worth limiting the porcelain "git remote" command to avoid
> this confusion. The example above cannot be done with "git remote",
> because it always[1] matches the refspecs to the remote name, and you
> can only have one instance of each remote name. But you can still
> trigger the more subtle variant like this:
>
>   git remote add outer [...]
>   git remote add outer/inner [...]
>
> So let's detect that kind of name collision (in both directions) and
> forbid it. You can still do whatever you like by manipulating the config
> directly, but this should prevent the most obvious foot-gun.
>
> [1] Almost always. With the --mirror option, the resulting refspec will
>     just write into "refs/*"; the remote name does not appear in the ref
>     namespace at all.
>
>     Our new "names must not overlap" rule is not necessary for that
>     case, but it seems reasonable to enforce it consistently. We already
>     require all remote names to be valid in the ref namespace, even
>     though we won't ever use them in that context for --mirror remotes.
>
>     Likewise, our new rule doesn't help with overlap here. Any two
>     mirror remotes will always overlap (in fact, any mirror remote along
>     with any other single one, since refs/remotes/ is a subset of the
>     mirrored refs). I'm not sure this is worth worrying about, but if it
>     is, we'd want an additional rule like "mirror remotes must be the
>     only remote".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Subject: [PATCH] remote: detect collisions in remote names
>
> When two remotes collide in the destinations of their fetch refspecs,
> the results can be confusing. For example, in this silly example:
>
>   git config remote.one.url [...]
>   git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
>   git config remote.two.url [...]
>   git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
>   git fetch --all
>
> we may try to write to the same ref twice (once for each remote we're
> fetching). There's also a more subtle version of this. If you have
> remotes "outer/inner" and "outer", then the ref "inner/branch" on the
> second remote will conflict with just "branch" on the former (they both
> want to write to "refs/remotes/outer/inner/branch").
>
> We probably don't want to forbid this kind of overlap completely. While
> the results can be confusing, there are legitimate reasons to have
> multiple refs write into the same namespace (e.g., if one is a "backup"
> of the other that is rarely fetched from).
>
> But it may be worth limiting the porcelain "git remote" command to avoid
> this confusion. The example above cannot be done with "git remote",
> because it always[1] matches the refspecs to the remote name, and you
> can only have one instance of each remote name. But you can still
> trigger the more subtle variant like this:
>
>   git remote add outer [...]
>   git remote add outer/inner [...]
>
> So let's detect that kind of name collision (in both directions) and
> forbid it. You can still do whatever you like by manipulating the config
> directly, but this should prevent the most obvious foot-gun.
>
> [1] Almost always. With the --mirror option, the resulting refspec will
>     just write into "refs/*"; the remote name does not appear in the ref
>     namespace at all.
>
>     Our new "names must not overlap" rule is not necessary for that
>     case, but it seems reasonable to enforce it consistently. We already
>     require all remote names to be valid in the ref namespace, even
>     though we won't ever use them in that context for --mirror remotes.
>
>     Likewise, our new rule doesn't help with overlap here. Any two
>     mirror remotes will always overlap (in fact, any mirror remote along
>     with any other single one, since refs/remotes/ is a subset of the
>     mirrored refs). I'm not sure this is worth worrying about, but if it
>     is, we'd want an additional rule like "mirror remotes must be the
>     only remote".
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/remote.c  | 17 +++++++++++++++++
>  t/t5505-remote.sh | 14 ++++++++++++++
>  2 files changed, 31 insertions(+)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 0d6755bcb7..a770df669c 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 *data)
> +{
> +	const char *name = data;
> +	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;
> +}
> +
>  static int add(int argc, const char **argv, const char *prefix,
>  	       struct repository *repo UNUSED)
>  {
> @@ -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);
>  
> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
> index bef0250e89..2701eef85e 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'forbid adding subset of existing remote' '
> +	test_when_finished "git remote rm outer" &&
> +	git remote add outer url &&
> +	test_must_fail git remote add outer/inner url 2>err &&
> +	test_grep ".outer/inner. is a subset of existing remote .outer." err
> +'
> +
> +test_expect_success 'forbid adding superset of existing remote' '
> +	test_when_finished "git remote rm outer/inner" &&
> +	git remote add outer/inner url &&
> +	test_must_fail git remote add outer url 2>err &&
> +	test_grep ".outer. is a superset of existing remote .outer/inner." err
> +'
> +
>  test_done

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

[Jeff King]

On Tue, Jul 08, 2025 at 04:28:43PM -0700, Junio C Hamano wrote:

> >   [trailing underscore: mostly a_, b_ in comparators]
> >   $ git grep 'void \*[a-zA-Z0-9]_' '*.c'  | wc -l
> >   30
> 
> Only a single letter followed by an underscore, which may be
> followed by more letters legal in names (like a_bcde)?
> 
> A more fair pattern may be something like
> 
> $ git grep 'void \*[A-Za-z_0-9]*_[^A-Za-z_0-9]' \*.c | wc -l
> 52

Doh, yeah. No wonder it mostly found "a_" and "b_". ;) Yours is a much
better pattern.

> > Although I think we're all a bit lukewarm on the concept, I feel like it
> > won't hurt anything, isn't too much code, and disables a potential (if
> > somewhat rare) footgun. So probably worth doing?
> 
> Even though it does not cover all cases, at least those coming from
> "git remote" will be able to avoid surprises, so let me replace with
> this version, wait for a few days for more inputs from others and
> then mark it for 'next' if nobody sees any downsides.

Sounds good, thanks.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 have firmly been in the "if it hurts..." camp.  People can do
weird things that may not make much sense to me, but do make sense
in their workflow that may be vastly different from mine.

But I do not think of any downsides from forbidding outer and
outer/inner existing at the same time, either ;-).

Thanks.

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

[Raymond E. Pasco]

On 25/07/05 02:58PM, Jeff King wrote:
> When two remotes collide in the destinations of their fetch refspecs,
> the results can be confusing. For example, in this silly example:
> 
>   git config remote.one.url [...]
>   git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*
>   git config remote.two.url [...]
>   git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*
>   git fetch --all
> 
> we may try to write to the same ref twice (once for each remote we're
> fetching). There's also a more subtle version of this. If you have
> remotes "outer/inner" and "outer", then the ref "inner/branch" on the
> second remote will conflict with just "branch" on the former (they both
> want to write to "refs/remotes/outer/inner/branch").

I can give my thoughts from the perspective of someone with an affected
workflow, if no one else is doing that.

I would expect '/' in remote names to be fairly common among people who
name remotes at all (a minority compared to those who have one remote
autonamed 'origin', probably); many things, from kernel.org to Github,
use path-like names (often username/reponame) to name repositories,
and the most relevant subset of that path is a natural thing to name a
remote. But that part doesn't seem controversial, despite the initial
message in this thread. So that's not a problem for me.

What this patch disallows, at least in porcelain, is something like
(these names are just examples) my naming a remote for gregkh/linux.git
"gregkh" and also naming a remote for gregkh/scsi.git "gregkh/scsi",
because it might lead to colliding names if gregkh makes a branch named
"scsi" on the former.

I've probably ever named remotes like this before, though I don't see
any examples in repositories I'm actively using this week. It's
plausible that other people have done this, or are doing it, though if I
had ever shot myself in the foot doing so I would have stopped.

Because it does seem prone to annoying mishaps, I think a change like
this is probably a good idea. It's not a confusing concept, because it's
familiar from how branch names with '/' in them already work.

What would the 'git remote' porcelain do in cases where remotes like
this already exist? I think, from this patch, nothing, since it's
only changing add()?