{"thread":{"id":"63739","subject":"Allowing \"/\" in the name of a git remote is a strange choice","startedAt":"2025-07-03T19:33:34Z","lastAt":"2025-07-09T11:56:56Z","messageCount":18,"participants":["Per Cederqvist","Junio C Hamano","Patrick Steinhardt","Lidong Yan","Jeff King","Raymond E. Pasco"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"521286","messageId":"CAHx6-Um1dq0xJ-RkW+qXe=sEa6JGViSJxjzNw56u55DHLYoT2Q@mail.gmail.com","threadId":"63739","inReplyTo":null,"subject":"Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Per Cederqvist","fromEmail":"ceder@lysator.liu.se","sentAt":"2025-07-03T19:33:20Z","receivedAt":"2025-07-03T19:33:34Z","isPatch":false,"sender":{"key":"ceder@lysator.liu.se","avatar":"https://gravatar.com/avatar/2fb7fdd80e190aad4640112b9d07fda1db01a5eb9b1d643d9e07d047e52bd71e?d=mp&s=160"},"body":"Today I realized that git accepts \"/\" in a remote name.\n\nThis can lead to problems. I have a repository that contains a branch\ncalled \"master\" and another called \"chat/master\". Just for fun, I\nadded a second remote in this repository and named it\n\"origin/chat\".\n\nNow, does \"refs/remotes/origin/chat/master\" refer to the branch\n\"chat/master\" from \"origin\", or the branch \"master\" from\n\"origin/chat\"? Git seems to think it refers to both:\n\n> $ git fetch --all\n> Fetching origin\n> From $PRIVATE_URL\n> + 4e31956300f...30e26ebbb19 chat/master -> origin/chat/master (forced update)\n> Fetching origin/chat\n> From $PRIVATE_URL\n> + 30e26ebbb19...4e31956300f master -> origin/chat/master (forced update)\n\nEvery time I run \"git fetch --all\" git updates the origin/chat/master ref twice.\n\nIf it was up to me, I'd add a check to valid_remote_name() to ensure\nthe name doesn't contain any \"/\" character. I doubt it is used often.\n\n /ceder\n"},{"id":"521292","messageId":"xmqqikk8bltr.fsf@gitster.g","threadId":"63739","inReplyTo":"CAHx6-Um1dq0xJ-RkW+qXe=sEa6JGViSJxjzNw56u55DHLYoT2Q@mail.gmail.com","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-04T04:51:12Z","receivedAt":"2025-07-04T04:51:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Per Cederqvist writes:\n\n> Today I realized that git accepts \"/\" in a remote name.\n>\n> This can lead to problems. I have a repository that contains a branch\n> called \"master\" and another called \"chat/master\". Just for fun, I\n> added a second remote in this repository and named it\n> \"origin/chat\".\n>\n> Now, does \"refs/remotes/origin/chat/master\" refer to the branch\n> \"chat/master\" from \"origin\", or the branch \"master\" from\n> \"origin/chat\"? Git seems to think it refers to both:\n\nThat would have been a fun experiment ;-)\n\n> If it was up to me, I'd add a check to valid_remote_name() to ensure\n> the name doesn't contain any \"/\" character. I doubt it is used often.\n\nIf your remote-naming discipline is to always use two-levels\n(e.g. origin/chat, origin/chien, origin/lapin but never origin or\norigin/chat/blanc mixed in), then there is no confusion.\n\nIt becomes only confusing if you mix origin and origin/chat. \n\nSo it is not like we can just forbid '/' retroactively and expect no\nrepercussions, especially given that I hear there are more than a\nfew thousands of existing Git users in the world.\n"},{"id":"521293","messageId":"aGdi6GRbI6Txm25Q@pks.im","threadId":"63739","inReplyTo":"xmqqikk8bltr.fsf@gitster.g","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-04T05:13:12Z","receivedAt":"2025-07-04T05:13:20Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 03, 2025 at 09:51:12PM -0700, Junio C Hamano wrote:\n> Per Cederqvist writes:\n> \n> > Today I realized that git accepts \"/\" in a remote name.\n> >\n> > This can lead to problems. I have a repository that contains a branch\n> > called \"master\" and another called \"chat/master\". Just for fun, I\n> > added a second remote in this repository and named it\n> > \"origin/chat\".\n> >\n> > Now, does \"refs/remotes/origin/chat/master\" refer to the branch\n> > \"chat/master\" from \"origin\", or the branch \"master\" from\n> > \"origin/chat\"? Git seems to think it refers to both:\n> \n> That would have been a fun experiment ;-)\n> \n> > If it was up to me, I'd add a check to valid_remote_name() to ensure\n> > the name doesn't contain any \"/\" character. I doubt it is used often.\n> \n> If your remote-naming discipline is to always use two-levels\n> (e.g. origin/chat, origin/chien, origin/lapin but never origin or\n> origin/chat/blanc mixed in), then there is no confusion.\n> \n> It becomes only confusing if you mix origin and origin/chat. \n> \n> So it is not like we can just forbid '/' retroactively and expect no\n> repercussions, especially given that I hear there are more than a\n> few thousands of existing Git users in the world.\n\nWe cannot just blanket-disallow this now, true. But shouldn't Git be\nable to detect this conflict, similar to how a user cannot have both\nrefs/heads/branch and refs/heads/branch/nested?\n\nPatrick\n"},{"id":"521295","messageId":"CAHx6-UmL7qHf-0SoD1qrOKbWK5JjuESJaZdQK_rjy66RrYg0Xg@mail.gmail.com","threadId":"63739","inReplyTo":"xmqqikk8bltr.fsf@gitster.g","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Per Cederqvist","fromEmail":"ceder@lysator.liu.se","sentAt":"2025-07-04T06:42:56Z","receivedAt":"2025-07-04T06:43:09Z","isPatch":false,"sender":{"key":"ceder@lysator.liu.se","avatar":"https://gravatar.com/avatar/2fb7fdd80e190aad4640112b9d07fda1db01a5eb9b1d643d9e07d047e52bd71e?d=mp&s=160"},"body":"On Fri, Jul 4, 2025 at 6:51 AM Junio C Hamano wrote:\n>\n> Per Cederqvist writes:\n>\n> > Today I realized that git accepts \"/\" in a remote name.\n> >\n> > This can lead to problems. I have a repository that contains a branch\n> > called \"master\" and another called \"chat/master\". Just for fun, I\n> > added a second remote in this repository and named it\n> > \"origin/chat\".\n> >\n> > Now, does \"refs/remotes/origin/chat/master\" refer to the branch\n> > \"chat/master\" from \"origin\", or the branch \"master\" from\n> > \"origin/chat\"? Git seems to think it refers to both:\n>\n> That would have been a fun experiment ;-)\n\nIt was. Luckily I figured this out while trying to deduce the allowed format\nof a remote name by reading the source code, not while trying to understand\nconfusing behaviour from git.\n\n> > If it was up to me, I'd add a check to valid_remote_name() to ensure\n> > the name doesn't contain any \"/\" character. I doubt it is used often.\n>\n> If your remote-naming discipline is to always use two-levels\n> (e.g. origin/chat, origin/chien, origin/lapin but never origin or\n> origin/chat/blanc mixed in), then there is no confusion.\n>\n> It becomes only confusing if you mix origin and origin/chat.\n>\n> So it is not like we can just forbid '/' retroactively and expect no\n> repercussions, especially given that I hear there are more than a\n> few thousands of existing Git users in the world.\n\nI wonder how many use \"/\" in a remote name, though. My guess is\nvery few.\n\nIf you want to do anything about this, there are a few possible ways:\n\n- forbid \"/\", but add a setting that allows it. Note that even if you forbid\n \"/\", existing clones will continue to work. It is only when you add a\n new remote that the name is checked.\n\n- require that the number of \"/\" character in a remote is equal for all\n remotes in a particular clone\n\n- deperecate \"/\" and start warning about it now, and forbid it after a\nsuitable period\n\n /ceder\n"},{"id":"521296","messageId":"10608B81-587A-4DED-ADC6-8F57B0B67E39@gmail.com","threadId":"63739","inReplyTo":"aGdi6GRbI6Txm25Q@pks.im","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-07-04T08:10:00Z","receivedAt":"2025-07-04T08:10:16Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Patrick Steinhardt writes:\n\n> We cannot just blanket-disallow this now, true. But shouldn't Git be\n> able to detect this conflict, similar to how a user cannot have both\n> refs/heads/branch and refs/heads/branch/nested?\n\nPerhaps we should forbid having two remotes where one is the directory base\n(i.e., a prefix) of the other.\n"},{"id":"521297","messageId":"AC45E9DD-5E2B-4DD5-B2C4-9276C70D05A6@gmail.com","threadId":"63739","inReplyTo":"aGdi6GRbI6Txm25Q@pks.im","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Lidong Yan","fromEmail":"yldhome2d2@gmail.com","sentAt":"2025-07-04T08:17:43Z","receivedAt":"2025-07-04T08:17:58Z","isPatch":false,"sender":{"key":"yldhome2d2@gmail.com","avatar":"https://avatars.githubusercontent.com/u/77328395?v=4"},"body":"Patrick Steinhardt write:\n> \n> We cannot just blanket-disallow this now, true. But shouldn't Git be\n> able to detect this conflict, similar to how a user cannot have both\n> refs/heads/branch and refs/heads/branch/nested?\n> \n\nI also find `git fetch` works fine, but check out remote branch detect this\nconflict\n\n$ git branch --set-upstream-to=origin/chat/master chat/master\n\nGives\n\nfatal: not tracking: ambiguous information for ref 'refs/remotes/origin/chat/master'\nhint: There are multiple remotes whose fetch refspecs map to the remote\nhint: tracking ref 'refs/remotes/origin/chat/master':\nhint: origin/chat\nhint: origin\nhint: \nhint: This is typically a configuration error.\nhint: \nhint: To support setting up tracking branches, ensure that\nhint: different remotes' fetch refspecs map into different\nhint: tracking namespaces\n\n"},{"id":"521325","messageId":"xmqqecuwavk3.fsf@gitster.g","threadId":"63739","inReplyTo":"aGdi6GRbI6Txm25Q@pks.im","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-04T14:18:36Z","receivedAt":"2025-07-04T14:18:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> So it is not like we can just forbid '/' retroactively and expect no\n>> repercussions, especially given that I hear there are more than a\n>> few thousands of existing Git users in the world.\n>\n> We cannot just blanket-disallow this now, true. But shouldn't Git be\n> able to detect this conflict, similar to how a user cannot have both\n> refs/heads/branch and refs/heads/branch/nested?\n\nYup. Sorry but I should probably have not left it out, as that was\nway too obvious an improved \"solution\", compared to \"just forbid '/',\nas I cannot imagine anybody using it\".\n"},{"id":"521343","messageId":"20250705165750.GA1951664@coredump.intra.peff.net","threadId":"63739","inReplyTo":"CAHx6-Um1dq0xJ-RkW+qXe=sEa6JGViSJxjzNw56u55DHLYoT2Q@mail.gmail.com","subject":"Re: Allowing \"/\" in the name of a git remote is a strange choice","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-05T16:57:50Z","receivedAt":"2025-07-05T16:57:57Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 03, 2025 at 09:33:20PM +0200, Per Cederqvist wrote:\n\n> > $ git fetch --all\n> > Fetching origin\n> > From $PRIVATE_URL\n> > + 4e31956300f...30e26ebbb19 chat/master -> origin/chat/master (forced update)\n> > Fetching origin/chat\n> > From $PRIVATE_URL\n> > + 30e26ebbb19...4e31956300f master -> origin/chat/master (forced update)\n> \n> Every time I run \"git fetch --all\" git updates the origin/chat/master ref twice.\n> \n> If it was up to me, I'd add a check to valid_remote_name() to ensure\n> the name doesn't contain any \"/\" character. I doubt it is used often.\n\nI think the \"/\" here is really just a special case of a more general\nproblem: overlapping fetch refspec destinations.\n\nFor example, try this:\n\n git init repo\n cd repo\n\n git init one\n git -C one commit --allow-empty -m foo\n\n git init two\n git -C two commit --allow-empty -m bar\n\n git config remote.one.url one\n git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n git config remote.two.url two\n git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n\n git fetch --all\n\nwhich gives similar output to what you showed above. Of course it's\neasier to see here when the names are identical rather than one being a\nprefix of the other. But it's fundamentally the same issue, and\nforbidding \"/\" would not fix it.\n\nWe could perhaps detect these kinds of overlap, but I wonder:\n\n 1. How expensive is it to do so, and when should we do it? Obviously\n for a handful of refs a quadratic approach is OK. But what if you\n had 10,000 remotes (this is not purely hypothetical; GitHub used to\n manage object migration in its fork networks with configured\n remotes, but hit enough performance issues to switch away from\n that). So I'd be hesitant to check this on every \"git fetch\".\n\n 2. Is it something people actually want to do? It's certainly a\n _weird_ configuration, but I could imagine there being useful\n corner cases (e.g., one URL is an infrequently backup of the other,\n so you don't usually do \"--all\", or you set skipDefaultUpdate for\n one of them.\n\nSo I dunno. It feels like a configuration error in most cases, but not\nall. I'd probably say that people touching the config manually should be\nallowed to do what they want, but maybe \"git remote\" should be a bit\nmore careful about names being proper subsets of existing remotes (it\nshould already prevent the exact-match above, I'd think, because the ref\nnamespace it uses will always match the configuration name).\n\n-Peff\n"},{"id":"521345","messageId":"20250705185842.GA2496172@coredump.intra.peff.net","threadId":"63739","inReplyTo":"20250705165750.GA1951664@coredump.intra.peff.net","subject":"[PATCH] remote: detect collisions in remote names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-05T18:58:42Z","receivedAt":"2025-07-05T18:58:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 05, 2025 at 12:57:50PM -0400, Jeff King wrote:\n\n> So I dunno. It feels like a configuration error in most cases, but not\n> all. I'd probably say that people touching the config manually should be\n> allowed to do what they want, but maybe \"git remote\" should be a bit\n> more careful about names being proper subsets of existing remotes (it\n> should already prevent the exact-match above, I'd think, because the ref\n> namespace it uses will always match the configuration name).\n\nSo I'm not entirely convinced we should do anything here. The answer\nmight just be \"if it hurts, don't do it\". But if we wanted any\nprotections in the \"git remote\" porcelain, they might look like this:\n\n-- >8 --\nSubject: [PATCH] remote: detect collisions in remote names\n\nWhen two remotes collide in the destinations of their fetch refspecs,\nthe results can be confusing. For example, in this silly example:\n\n git config remote.one.url [...]\n git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n git config remote.two.url [...]\n git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n git fetch --all\n\nwe may try to write to the same ref twice (once for each remote we're\nfetching). There's also a more subtle version of this. If you have\nremotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\nsecond remote will conflict with just \"branch\" on the former (they both\nwant to write to \"refs/remotes/outer/inner/branch\").\n\nWe probably don't want to forbid this kind of overlap completely. While\nthe results can be confusing, there are legitimate reasons to have\nmultiple refs write into the same namespace (e.g., if one is a \"backup\"\nof the other that is rarely fetched from).\n\nBut it may be worth limiting the porcelain \"git remote\" command to avoid\nthis confusion. The example above cannot be done with \"git remote\",\nbecause it always[1] matches the refspecs to the remote name, and you\ncan only have one instance of each remote name. But you can still\ntrigger the more subtle variant like this:\n\n git remote add outer [...]\n git remote add outer/inner [...]\n\nSo let's detect that kind of name collision (in both directions) and\nforbid it. You can still do whatever you like by manipulating the config\ndirectly, but this should prevent the most obvious foot-gun.\n\n[1] Almost always. With the --mirror option, the resulting refspec will\n just write into \"refs/*\"; the remote name does not appear in the ref\n namespace at all.\n\n Our new \"names must not overlap\" rule is not necessary for that\n case, but it seems reasonable to enforce it consistently. We already\n require all remote names to be valid in the ref namespace, even\n though we won't ever use them in that context for --mirror remotes.\n\n Likewise, our new rule doesn't help with overlap here. Any two\n mirror remotes will always overlap (in fact, any mirror remote along\n with any other single one, since refs/remotes/ is a subset of the\n mirrored refs). I'm not sure this is worth worrying about, but if it\n is, we'd want an additional rule like \"mirror remotes must be the\n only remote\".\n\nSigned-off-by: Jeff King \n---\n builtin/remote.c | 17 +++++++++++++++++\n t/t5505-remote.sh | 14 ++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 0d6755bcb7..b18730ddb2 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -157,6 +157,21 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)\n \treturn 0;\n }\n \n+static int check_remote_collision(struct remote *remote, void *vname)\n+{\n+\tconst char *name = vname;\n+\tconst char *p;\n+\n+\tif (skip_prefix(name, remote->name, &p) && *p == '/')\n+\t\tdie(_(\"remote name '%s' is a subset of existing remote '%s'\"),\n+\t\t name, remote->name);\n+\tif (skip_prefix(remote->name, name, &p) && *p == '/')\n+\t\tdie(_(\"remote name '%s' is a superset of existing remote '%s'\"),\n+\t\t name, remote->name);\n+\n+\treturn 0;\n+}\n+\n static int add(int argc, const char **argv, const char *prefix,\n \t struct repository *repo UNUSED)\n {\n@@ -208,6 +223,8 @@ static int add(int argc, const char **argv, const char *prefix,\n \tif (!valid_remote_name(name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n+\tfor_each_remote(check_remote_collision, (void *)name);\n+\n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n \tgit_config_set(buf.buf, url);\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex bef0250e89..2701eef85e 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'forbid adding subset of existing remote' '\n+\ttest_when_finished \"git remote rm outer\" &&\n+\tgit remote add outer url &&\n+\ttest_must_fail git remote add outer/inner url 2>err &&\n+\ttest_grep \".outer/inner. is a subset of existing remote .outer.\" err\n+'\n+\n+test_expect_success 'forbid adding superset of existing remote' '\n+\ttest_when_finished \"git remote rm outer/inner\" &&\n+\tgit remote add outer/inner url &&\n+\ttest_must_fail git remote add outer url 2>err &&\n+\ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n+'\n+\n test_done\n-- \n2.50.0.438.g3b3bebd3e8\n\n"},{"id":"521420","messageId":"aGuP3Q5xykmRNp0m@pks.im","threadId":"63739","inReplyTo":"20250705185842.GA2496172@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-07T09:14:05Z","receivedAt":"2025-07-07T09:14:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Jul 05, 2025 at 02:58:42PM -0400, Jeff King wrote:\n> On Sat, Jul 05, 2025 at 12:57:50PM -0400, Jeff King wrote:\n> \n> > So I dunno. It feels like a configuration error in most cases, but not\n> > all. I'd probably say that people touching the config manually should be\n> > allowed to do what they want, but maybe \"git remote\" should be a bit\n> > more careful about names being proper subsets of existing remotes (it\n> > should already prevent the exact-match above, I'd think, because the ref\n> > namespace it uses will always match the configuration name).\n> \n> So I'm not entirely convinced we should do anything here. The answer\n> might just be \"if it hurts, don't do it\". But if we wanted any\n> protections in the \"git remote\" porcelain, they might look like this:\n\nI think having these protections is sensible. And I also agree with you\nthat we shouldn't keep people from doing weird things by manipulating\nthe config directly.\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 0d6755bcb7..b18730ddb2 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -157,6 +157,21 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)\n> \treturn 0;\n> }\n> \n> +static int check_remote_collision(struct remote *remote, void *vname)\n\nTiniest nit: I was a bit puzzled what the `v` in `vname` stands for, and\nit took a while until I noticed that it probably stands for `void`. If\nyou end up rerolling, I'd suggest to either call this `payload` or\n`_name`.\n\n> +{\n> +\tconst char *name = vname;\n> +\tconst char *p;\n> +\n> +\tif (skip_prefix(name, remote->name, &p) && *p == '/')\n> +\t\tdie(_(\"remote name '%s' is a subset of existing remote '%s'\"),\n> +\t\t name, remote->name);\n> +\tif (skip_prefix(remote->name, name, &p) && *p == '/')\n> +\t\tdie(_(\"remote name '%s' is a superset of existing remote '%s'\"),\n> +\t\t name, remote->name);\n> +\n> +\treturn 0;\n> +}\n> +\n\nHm. Do we have to care about '\\' on Windows, as well? This made me\nrediscover the following function:\n\n static int valid_remote_nick(const char *name)\n {\n \tif (!name[0] || is_dot_or_dotdot(name))\n \t\treturn 0;\n \n \t/* remote nicknames cannot contain slashes */\n \twhile (*name)\n \t\tif (is_dir_sep(*name++))\n \t\t\treturn 0;\n \treturn 1;\n }\n\nWhich... puzzled me a bit at first, as it seems to indicate that a\nremote with a path separator is invalid. But as it turns out we only use\nthis function if remotes are configured via \".git/remotes\" or\n\".git/branches\". Looks like we eventually lost this limitation, probably\nwhen config-based remotes were introduced.\n\n> @@ -208,6 +223,8 @@ static int add(int argc, const char **argv, const char *prefix,\n> \tif (!valid_remote_name(name))\n> \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n> \n> +\tfor_each_remote(check_remote_collision, (void *)name);\n> +\n> \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n> \tgit_config_set(buf.buf, url);\n> \n\nNice and simple.\n\nPatrick\n"},{"id":"521438","messageId":"xmqqqzys5cgr.fsf@gitster.g","threadId":"63739","inReplyTo":"20250705185842.GA2496172@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T13:59:00Z","receivedAt":"2025-07-07T13:59:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jul 05, 2025 at 12:57:50PM -0400, Jeff King wrote:\n>\n>> So I dunno. It feels like a configuration error in most cases, but not\n>> all. I'd probably say that people touching the config manually should be\n>> allowed to do what they want, but maybe \"git remote\" should be a bit\n>> more careful about names being proper subsets of existing remotes (it\n>> should already prevent the exact-match above, I'd think, because the ref\n>> namespace it uses will always match the configuration name).\n>\n> So I'm not entirely convinced we should do anything here. The answer\n> might just be \"if it hurts, don't do it\". But if we wanted any\n> protections in the \"git remote\" porcelain, they might look like this:\n\nI have firmly been in the \"if it hurts...\" camp. People can do\nweird things that may not make much sense to me, but do make sense\nin their workflow that may be vastly different from mine.\n\nBut I do not think of any downsides from forbidding outer and\nouter/inner existing at the same time, either ;-).\n\nThanks.\n"},{"id":"521471","messageId":"20250707202801.GA3115893@coredump.intra.peff.net","threadId":"63739","inReplyTo":"aGuP3Q5xykmRNp0m@pks.im","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-07T20:28:01Z","receivedAt":"2025-07-07T20:28:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 07, 2025 at 11:14:05AM +0200, Patrick Steinhardt wrote:\n\n> > +static int check_remote_collision(struct remote *remote, void *vname)\n> \n> Tiniest nit: I was a bit puzzled what the `v` in `vname` stands for, and\n> it took a while until I noticed that it probably stands for `void`. If\n> you end up rerolling, I'd suggest to either call this `payload` or\n> `_name`.\n\nYeah, it's for \"void\". This is a pattern used elsewhere for callbacks\n(usually as \"vdata\", but here we didn't need a container struct since\nthere's only one item). I think \"payload\" is not a term we usually use,\nbut maybe just \"data\" would be the usual thing (we only need \"vdata\"\nwhen we're assigning to the non-void data type).\n\nIMHO we should probably avoid the underscore pattern. It's OK here, but\nit runs close to violating the reserved names rules (a global variable\nvariable _name is bad, and _Name anywhere is bad).\n\n> Hm. Do we have to care about '\\' on Windows, as well? This made me\n> rediscover the following function:\n> \n> static int valid_remote_nick(const char *name)\n> {\n> \tif (!name[0] || is_dot_or_dotdot(name))\n> \t\treturn 0;\n> \n> \t/* remote nicknames cannot contain slashes */\n> \twhile (*name)\n> \t\tif (is_dir_sep(*name++))\n> \t\t\treturn 0;\n> \treturn 1;\n> }\n> \n> Which... puzzled me a bit at first, as it seems to indicate that a\n> remote with a path separator is invalid. But as it turns out we only use\n> this function if remotes are configured via \".git/remotes\" or\n> \".git/branches\". Looks like we eventually lost this limitation, probably\n> when config-based remotes were introduced.\n\nAFAICT \"remote add\" allows anything that parses as a refspec, which\nimplies that refs/remotes// passes check_refname_format(). And we\ndon't allow backslashes there:\n\n $ git remote add foo/bar url\n [no output, $? is 0]\n $ git remote add 'bar\\foo' url\n fatal: 'bar\\foo' is not a valid remote name\n\nI don't think this is platform dependent. It's coming from the\nrefname_disposition table, so we're not calling is_dir_sep(). Only '/'\nis marked in that table as end-of-component, and \"\\\\\" is forbidden.\n\nSo I don't think we need to worry about backslashes here.\n\n-Peff\n"},{"id":"521475","messageId":"xmqqtt3n3e7g.fsf@gitster.g","threadId":"63739","inReplyTo":"20250707202801.GA3115893@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-07T21:04:19Z","receivedAt":"2025-07-07T21:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 07, 2025 at 11:14:05AM +0200, Patrick Steinhardt wrote:\n>\n>> > +static int check_remote_collision(struct remote *remote, void *vname)\n>> \n>> Tiniest nit: I was a bit puzzled what the `v` in `vname` stands for, and\n>> it took a while until I noticed that it probably stands for `void`. If\n>> you end up rerolling, I'd suggest to either call this `payload` or\n>> `_name`.\n>\n> Yeah, it's for \"void\". This is a pattern used elsewhere for callbacks\n> (usually as \"vdata\", but here we didn't need a container struct since\n> there's only one item). I think \"payload\" is not a term we usually use,\n> but maybe just \"data\" would be the usual thing (we only need \"vdata\"\n> when we're assigning to the non-void data type).\n>\n> IMHO we should probably avoid the underscore pattern. It's OK here, but\n> it runs close to violating the reserved names rules (a global variable\n> variable _name is bad, and _Name anywhere is bad).\n\n\"name_\" is available. In fact I think it is a very common pattern\nin this codebase to name an incoming parameter with trailing \"_\",\nand assign it to a local variable with the right name and with the\nright type at the top of the function.\n\n> AFAICT \"remote add\" allows anything that parses as a refspec, which\n> implies that refs/remotes// passes check_refname_format(). And we\n> don't allow backslashes there:\n>\n> $ git remote add foo/bar url\n> [no output, $? is 0]\n> $ git remote add 'bar\\foo' url\n> fatal: 'bar\\foo' is not a valid remote name\n>\n> I don't think this is platform dependent. It's coming from the\n> refname_disposition table, so we're not calling is_dir_sep(). Only '/'\n> is marked in that table as end-of-component, and \"\\\\\" is forbidden.\n>\n> So I don't think we need to worry about backslashes here.\n\nThat agrees with my understanding. Thanks for carefully checking.\n\n"},{"id":"521587","messageId":"20250708225946.GC1180568@coredump.intra.peff.net","threadId":"63739","inReplyTo":"xmqqtt3n3e7g.fsf@gitster.g","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-08T22:59:46Z","receivedAt":"2025-07-08T22:59:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 07, 2025 at 02:04:19PM -0700, Junio C Hamano wrote:\n\n> > IMHO we should probably avoid the underscore pattern. It's OK here, but\n> > it runs close to violating the reserved names rules (a global variable\n> > variable _name is bad, and _Name anywhere is bad).\n> \n> \"name_\" is available. In fact I think it is a very common pattern\n> in this codebase to name an incoming parameter with trailing \"_\",\n> and assign it to a local variable with the right name and with the\n> right type at the top of the function.\n\nYeah, that is legal and is a pattern we use (though I admit that I find\nany underscores kind of ugly and easy to miss). I was curious how often\neach pattern appeared:\n\n [\"v\" prefix: vdata, va, etc]\n $ git grep 'void \\*v' '*.c' | wc -l\n 51\n\n [leading underscore: _data, _a, etc]\n $ git grep 'void \\*_' '*.c' | wc -l\n 52\n\n [trailing underscore: mostly a_, b_ in comparators]\n $ git grep 'void \\*[a-zA-Z0-9]_' '*.c' | wc -l\n 30\n\n [just calling it \"data\"]\n $ git grep 'void \\*data' '*.c' | wc -l\n 314\n\nThe last one is cheating a little because it catches function pointer\ndeclarations, too, but grepping for \"= data;\" returns over a hundred\nhits, too.\n\nSo that was mostly for fun, and I think any is OK. ;) But here is the\npatch again with the void pointer just called \"data\".\n\nAlthough I think we're all a bit lukewarm on the concept, I feel like it\nwon't hurt anything, isn't too much code, and disables a potential (if\nsomewhat rare) footgun. So probably worth doing?\n\n-- >8 --\nSubject: [PATCH] remote: detect collisions in remote names\n\nWhen two remotes collide in the destinations of their fetch refspecs,\nthe results can be confusing. For example, in this silly example:\n\n git config remote.one.url [...]\n git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n git config remote.two.url [...]\n git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n git fetch --all\n\nwe may try to write to the same ref twice (once for each remote we're\nfetching). There's also a more subtle version of this. If you have\nremotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\nsecond remote will conflict with just \"branch\" on the former (they both\nwant to write to \"refs/remotes/outer/inner/branch\").\n\nWe probably don't want to forbid this kind of overlap completely. While\nthe results can be confusing, there are legitimate reasons to have\nmultiple refs write into the same namespace (e.g., if one is a \"backup\"\nof the other that is rarely fetched from).\n\nBut it may be worth limiting the porcelain \"git remote\" command to avoid\nthis confusion. The example above cannot be done with \"git remote\",\nbecause it always[1] matches the refspecs to the remote name, and you\ncan only have one instance of each remote name. But you can still\ntrigger the more subtle variant like this:\n\n git remote add outer [...]\n git remote add outer/inner [...]\n\nSo let's detect that kind of name collision (in both directions) and\nforbid it. You can still do whatever you like by manipulating the config\ndirectly, but this should prevent the most obvious foot-gun.\n\n[1] Almost always. With the --mirror option, the resulting refspec will\n just write into \"refs/*\"; the remote name does not appear in the ref\n namespace at all.\n\n Our new \"names must not overlap\" rule is not necessary for that\n case, but it seems reasonable to enforce it consistently. We already\n require all remote names to be valid in the ref namespace, even\n though we won't ever use them in that context for --mirror remotes.\n\n Likewise, our new rule doesn't help with overlap here. Any two\n mirror remotes will always overlap (in fact, any mirror remote along\n with any other single one, since refs/remotes/ is a subset of the\n mirrored refs). I'm not sure this is worth worrying about, but if it\n is, we'd want an additional rule like \"mirror remotes must be the\n only remote\".\n\nSigned-off-by: Jeff King \n---\nSubject: [PATCH] remote: detect collisions in remote names\n\nWhen two remotes collide in the destinations of their fetch refspecs,\nthe results can be confusing. For example, in this silly example:\n\n git config remote.one.url [...]\n git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n git config remote.two.url [...]\n git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n git fetch --all\n\nwe may try to write to the same ref twice (once for each remote we're\nfetching). There's also a more subtle version of this. If you have\nremotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\nsecond remote will conflict with just \"branch\" on the former (they both\nwant to write to \"refs/remotes/outer/inner/branch\").\n\nWe probably don't want to forbid this kind of overlap completely. While\nthe results can be confusing, there are legitimate reasons to have\nmultiple refs write into the same namespace (e.g., if one is a \"backup\"\nof the other that is rarely fetched from).\n\nBut it may be worth limiting the porcelain \"git remote\" command to avoid\nthis confusion. The example above cannot be done with \"git remote\",\nbecause it always[1] matches the refspecs to the remote name, and you\ncan only have one instance of each remote name. But you can still\ntrigger the more subtle variant like this:\n\n git remote add outer [...]\n git remote add outer/inner [...]\n\nSo let's detect that kind of name collision (in both directions) and\nforbid it. You can still do whatever you like by manipulating the config\ndirectly, but this should prevent the most obvious foot-gun.\n\n[1] Almost always. With the --mirror option, the resulting refspec will\n just write into \"refs/*\"; the remote name does not appear in the ref\n namespace at all.\n\n Our new \"names must not overlap\" rule is not necessary for that\n case, but it seems reasonable to enforce it consistently. We already\n require all remote names to be valid in the ref namespace, even\n though we won't ever use them in that context for --mirror remotes.\n\n Likewise, our new rule doesn't help with overlap here. Any two\n mirror remotes will always overlap (in fact, any mirror remote along\n with any other single one, since refs/remotes/ is a subset of the\n mirrored refs). I'm not sure this is worth worrying about, but if it\n is, we'd want an additional rule like \"mirror remotes must be the\n only remote\".\n\nSigned-off-by: Jeff King \n---\n builtin/remote.c | 17 +++++++++++++++++\n t/t5505-remote.sh | 14 ++++++++++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 0d6755bcb7..a770df669c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -157,6 +157,21 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)\n \treturn 0;\n }\n \n+static int check_remote_collision(struct remote *remote, void *data)\n+{\n+\tconst char *name = data;\n+\tconst char *p;\n+\n+\tif (skip_prefix(name, remote->name, &p) && *p == '/')\n+\t\tdie(_(\"remote name '%s' is a subset of existing remote '%s'\"),\n+\t\t name, remote->name);\n+\tif (skip_prefix(remote->name, name, &p) && *p == '/')\n+\t\tdie(_(\"remote name '%s' is a superset of existing remote '%s'\"),\n+\t\t name, remote->name);\n+\n+\treturn 0;\n+}\n+\n static int add(int argc, const char **argv, const char *prefix,\n \t struct repository *repo UNUSED)\n {\n@@ -208,6 +223,8 @@ static int add(int argc, const char **argv, const char *prefix,\n \tif (!valid_remote_name(name))\n \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n \n+\tfor_each_remote(check_remote_collision, (void *)name);\n+\n \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n \tgit_config_set(buf.buf, url);\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex bef0250e89..2701eef85e 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'forbid adding subset of existing remote' '\n+\ttest_when_finished \"git remote rm outer\" &&\n+\tgit remote add outer url &&\n+\ttest_must_fail git remote add outer/inner url 2>err &&\n+\ttest_grep \".outer/inner. is a subset of existing remote .outer.\" err\n+'\n+\n+test_expect_success 'forbid adding superset of existing remote' '\n+\ttest_when_finished \"git remote rm outer/inner\" &&\n+\tgit remote add outer/inner url &&\n+\ttest_must_fail git remote add outer url 2>err &&\n+\ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n+'\n+\n test_done\n-- \n2.50.1.488.g2a977559af\n\n"},{"id":"521588","messageId":"20250708230217.GA1185024@coredump.intra.peff.net","threadId":"63739","inReplyTo":"20250708225946.GC1180568@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-08T23:02:17Z","receivedAt":"2025-07-08T23:02:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 08, 2025 at 06:59:47PM -0400, Jeff King wrote:\n\n> Although I think we're all a bit lukewarm on the concept, I feel like it\n> won't hurt anything, isn't too much code, and disables a potential (if\n> somewhat rare) footgun. So probably worth doing?\n\n> -- >8 --\n> Subject: [PATCH] remote: detect collisions in remote names\n> [...]\n> Signed-off-by: Jeff King \n> ---\n> Subject: [PATCH] remote: detect collisions in remote names\n\nSorry, I am apparently bad at using my editor.\n\nI _think_ this will just apply correctly for you, since the duplicated\ncommit message is all after the \"---\".\n\n-Peff\n"},{"id":"521590","messageId":"xmqq5xg2s1n8.fsf@gitster.g","threadId":"63739","inReplyTo":"20250708225946.GC1180568@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-08T23:28:43Z","receivedAt":"2025-07-08T23:28:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 07, 2025 at 02:04:19PM -0700, Junio C Hamano wrote:\n>\n>> > IMHO we should probably avoid the underscore pattern. It's OK here, but\n>> > it runs close to violating the reserved names rules (a global variable\n>> > variable _name is bad, and _Name anywhere is bad).\n>> \n>> \"name_\" is available. In fact I think it is a very common pattern\n>> in this codebase to name an incoming parameter with trailing \"_\",\n>> and assign it to a local variable with the right name and with the\n>> right type at the top of the function.\n>\n> Yeah, that is legal and is a pattern we use (though I admit that I find\n> any underscores kind of ugly and easy to miss). I was curious how often\n> each pattern appeared:\n>\n> [\"v\" prefix: vdata, va, etc]\n> $ git grep 'void \\*v' '*.c' | wc -l\n> 51\n>\n> [leading underscore: _data, _a, etc]\n> $ git grep 'void \\*_' '*.c' | wc -l\n> 52\n>\n> [trailing underscore: mostly a_, b_ in comparators]\n> $ git grep 'void \\*[a-zA-Z0-9]_' '*.c' | wc -l\n> 30\n\nOnly a single letter followed by an underscore, which may be\nfollowed by more letters legal in names (like a_bcde)?\n\nA more fair pattern may be something like\n\n$ git grep 'void \\*[A-Za-z_0-9]*_[^A-Za-z_0-9]' \\*.c | wc -l\n52\n\n> [just calling it \"data\"]\n> $ git grep 'void \\*data' '*.c' | wc -l\n> 314\n>\n> The last one is cheating a little because it catches function pointer\n> declarations, too, but grepping for \"= data;\" returns over a hundred\n> hits, too.\n\nAlso \"void *cb_data\" is fairly common, I think, as we have some\ncallback API functions.\n\n$ git grep 'void \\*cb_data' \\*.c | wc -l\n234\n\n> So that was mostly for fun, and I think any is OK. ;) But here is the\n> patch again with the void pointer just called \"data\".\n\nYeah, I think any would be fine. I was a bit surprised that\nv-something was so widely used, though, as I find that it makes the\nleast sense among all possibilities (and \"data\" is the distant\nsecond, as it would become awkward when you have to have more than\none, like my_custom_cmp(void *left, void *right), if your rule says\nthat you must say \"data\").\n\n> Although I think we're all a bit lukewarm on the concept, I feel like it\n> won't hurt anything, isn't too much code, and disables a potential (if\n> somewhat rare) footgun. So probably worth doing?\n\nEven though it does not cover all cases, at least those coming from\n\"git remote\" will be able to avoid surprises, so let me replace with\nthis version, wait for a few days for more inputs from others and\nthen mark it for 'next' if nobody sees any downsides.\n\n\n> -- >8 --\n> Subject: [PATCH] remote: detect collisions in remote names\n>\n> When two remotes collide in the destinations of their fetch refspecs,\n> the results can be confusing. For example, in this silly example:\n>\n> git config remote.one.url [...]\n> git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n> git config remote.two.url [...]\n> git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n> git fetch --all\n>\n> we may try to write to the same ref twice (once for each remote we're\n> fetching). There's also a more subtle version of this. If you have\n> remotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\n> second remote will conflict with just \"branch\" on the former (they both\n> want to write to \"refs/remotes/outer/inner/branch\").\n>\n> We probably don't want to forbid this kind of overlap completely. While\n> the results can be confusing, there are legitimate reasons to have\n> multiple refs write into the same namespace (e.g., if one is a \"backup\"\n> of the other that is rarely fetched from).\n>\n> But it may be worth limiting the porcelain \"git remote\" command to avoid\n> this confusion. The example above cannot be done with \"git remote\",\n> because it always[1] matches the refspecs to the remote name, and you\n> can only have one instance of each remote name. But you can still\n> trigger the more subtle variant like this:\n>\n> git remote add outer [...]\n> git remote add outer/inner [...]\n>\n> So let's detect that kind of name collision (in both directions) and\n> forbid it. You can still do whatever you like by manipulating the config\n> directly, but this should prevent the most obvious foot-gun.\n>\n> [1] Almost always. With the --mirror option, the resulting refspec will\n> just write into \"refs/*\"; the remote name does not appear in the ref\n> namespace at all.\n>\n> Our new \"names must not overlap\" rule is not necessary for that\n> case, but it seems reasonable to enforce it consistently. We already\n> require all remote names to be valid in the ref namespace, even\n> though we won't ever use them in that context for --mirror remotes.\n>\n> Likewise, our new rule doesn't help with overlap here. Any two\n> mirror remotes will always overlap (in fact, any mirror remote along\n> with any other single one, since refs/remotes/ is a subset of the\n> mirrored refs). I'm not sure this is worth worrying about, but if it\n> is, we'd want an additional rule like \"mirror remotes must be the\n> only remote\".\n>\n> Signed-off-by: Jeff King \n> ---\n> Subject: [PATCH] remote: detect collisions in remote names\n>\n> When two remotes collide in the destinations of their fetch refspecs,\n> the results can be confusing. For example, in this silly example:\n>\n> git config remote.one.url [...]\n> git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n> git config remote.two.url [...]\n> git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n> git fetch --all\n>\n> we may try to write to the same ref twice (once for each remote we're\n> fetching). There's also a more subtle version of this. If you have\n> remotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\n> second remote will conflict with just \"branch\" on the former (they both\n> want to write to \"refs/remotes/outer/inner/branch\").\n>\n> We probably don't want to forbid this kind of overlap completely. While\n> the results can be confusing, there are legitimate reasons to have\n> multiple refs write into the same namespace (e.g., if one is a \"backup\"\n> of the other that is rarely fetched from).\n>\n> But it may be worth limiting the porcelain \"git remote\" command to avoid\n> this confusion. The example above cannot be done with \"git remote\",\n> because it always[1] matches the refspecs to the remote name, and you\n> can only have one instance of each remote name. But you can still\n> trigger the more subtle variant like this:\n>\n> git remote add outer [...]\n> git remote add outer/inner [...]\n>\n> So let's detect that kind of name collision (in both directions) and\n> forbid it. You can still do whatever you like by manipulating the config\n> directly, but this should prevent the most obvious foot-gun.\n>\n> [1] Almost always. With the --mirror option, the resulting refspec will\n> just write into \"refs/*\"; the remote name does not appear in the ref\n> namespace at all.\n>\n> Our new \"names must not overlap\" rule is not necessary for that\n> case, but it seems reasonable to enforce it consistently. We already\n> require all remote names to be valid in the ref namespace, even\n> though we won't ever use them in that context for --mirror remotes.\n>\n> Likewise, our new rule doesn't help with overlap here. Any two\n> mirror remotes will always overlap (in fact, any mirror remote along\n> with any other single one, since refs/remotes/ is a subset of the\n> mirrored refs). I'm not sure this is worth worrying about, but if it\n> is, we'd want an additional rule like \"mirror remotes must be the\n> only remote\".\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/remote.c | 17 +++++++++++++++++\n> t/t5505-remote.sh | 14 ++++++++++++++\n> 2 files changed, 31 insertions(+)\n>\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 0d6755bcb7..a770df669c 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -157,6 +157,21 @@ static int parse_mirror_opt(const struct option *opt, const char *arg, int not)\n> \treturn 0;\n> }\n> \n> +static int check_remote_collision(struct remote *remote, void *data)\n> +{\n> +\tconst char *name = data;\n> +\tconst char *p;\n> +\n> +\tif (skip_prefix(name, remote->name, &p) && *p == '/')\n> +\t\tdie(_(\"remote name '%s' is a subset of existing remote '%s'\"),\n> +\t\t name, remote->name);\n> +\tif (skip_prefix(remote->name, name, &p) && *p == '/')\n> +\t\tdie(_(\"remote name '%s' is a superset of existing remote '%s'\"),\n> +\t\t name, remote->name);\n> +\n> +\treturn 0;\n> +}\n> +\n> static int add(int argc, const char **argv, const char *prefix,\n> \t struct repository *repo UNUSED)\n> {\n> @@ -208,6 +223,8 @@ static int add(int argc, const char **argv, const char *prefix,\n> \tif (!valid_remote_name(name))\n> \t\tdie(_(\"'%s' is not a valid remote name\"), name);\n> \n> +\tfor_each_remote(check_remote_collision, (void *)name);\n> +\n> \tstrbuf_addf(&buf, \"remote.%s.url\", name);\n> \tgit_config_set(buf.buf, url);\n> \n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index bef0250e89..2701eef85e 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -1644,4 +1644,18 @@ test_expect_success 'empty config clears remote.*.pushurl list' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'forbid adding subset of existing remote' '\n> +\ttest_when_finished \"git remote rm outer\" &&\n> +\tgit remote add outer url &&\n> +\ttest_must_fail git remote add outer/inner url 2>err &&\n> +\ttest_grep \".outer/inner. is a subset of existing remote .outer.\" err\n> +'\n> +\n> +test_expect_success 'forbid adding superset of existing remote' '\n> +\ttest_when_finished \"git remote rm outer/inner\" &&\n> +\tgit remote add outer/inner url &&\n> +\ttest_must_fail git remote add outer url 2>err &&\n> +\ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n> +'\n> +\n> test_done\n"},{"id":"521601","messageId":"20250709012134.GA1185474@coredump.intra.peff.net","threadId":"63739","inReplyTo":"xmqq5xg2s1n8.fsf@gitster.g","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-09T01:21:34Z","receivedAt":"2025-07-09T01:21:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 08, 2025 at 04:28:43PM -0700, Junio C Hamano wrote:\n\n> > [trailing underscore: mostly a_, b_ in comparators]\n> > $ git grep 'void \\*[a-zA-Z0-9]_' '*.c' | wc -l\n> > 30\n> \n> Only a single letter followed by an underscore, which may be\n> followed by more letters legal in names (like a_bcde)?\n> \n> A more fair pattern may be something like\n> \n> $ git grep 'void \\*[A-Za-z_0-9]*_[^A-Za-z_0-9]' \\*.c | wc -l\n> 52\n\nDoh, yeah. No wonder it mostly found \"a_\" and \"b_\". ;) Yours is a much\nbetter pattern.\n\n> > Although I think we're all a bit lukewarm on the concept, I feel like it\n> > won't hurt anything, isn't too much code, and disables a potential (if\n> > somewhat rare) footgun. So probably worth doing?\n> \n> Even though it does not cover all cases, at least those coming from\n> \"git remote\" will be able to avoid surprises, so let me replace with\n> this version, wait for a few days for more inputs from others and\n> then mark it for 'next' if nobody sees any downsides.\n\nSounds good, thanks.\n\n-Peff\n"},{"id":"521668","messageId":"xra2vj7fcdsieg4xkvxlctcoubdwalgmhyswub6dxi2pnb34e3@iadinufm23ez","threadId":"63739","inReplyTo":"20250705185842.GA2496172@coredump.intra.peff.net","subject":"Re: [PATCH] remote: detect collisions in remote names","fromName":"Raymond E. Pasco","fromEmail":"ray@ameretat.dev","sentAt":"2025-07-09T11:56:22Z","receivedAt":"2025-07-09T11:56:56Z","isPatch":true,"sender":{"key":"ray@ameretat.dev","avatar":"https://avatars.githubusercontent.com/u/115765?v=4"},"body":"On 25/07/05 02:58PM, Jeff King wrote:\n> When two remotes collide in the destinations of their fetch refspecs,\n> the results can be confusing. For example, in this silly example:\n> \n> git config remote.one.url [...]\n> git config remote.one.fetch +refs/heads/*:refs/remotes/collide/*\n> git config remote.two.url [...]\n> git config remote.two.fetch +refs/heads/*:refs/remotes/collide/*\n> git fetch --all\n> \n> we may try to write to the same ref twice (once for each remote we're\n> fetching). There's also a more subtle version of this. If you have\n> remotes \"outer/inner\" and \"outer\", then the ref \"inner/branch\" on the\n> second remote will conflict with just \"branch\" on the former (they both\n> want to write to \"refs/remotes/outer/inner/branch\").\n\nI can give my thoughts from the perspective of someone with an affected\nworkflow, if no one else is doing that.\n\nI would expect '/' in remote names to be fairly common among people who\nname remotes at all (a minority compared to those who have one remote\nautonamed 'origin', probably); many things, from kernel.org to Github,\nuse path-like names (often username/reponame) to name repositories,\nand the most relevant subset of that path is a natural thing to name a\nremote. But that part doesn't seem controversial, despite the initial\nmessage in this thread. So that's not a problem for me.\n\nWhat this patch disallows, at least in porcelain, is something like\n(these names are just examples) my naming a remote for gregkh/linux.git\n\"gregkh\" and also naming a remote for gregkh/scsi.git \"gregkh/scsi\",\nbecause it might lead to colliding names if gregkh makes a branch named\n\"scsi\" on the former.\n\nI've probably ever named remotes like this before, though I don't see\nany examples in repositories I'm actively using this week. It's\nplausible that other people have done this, or are doing it, though if I\nhad ever shot myself in the foot doing so I would have stopped.\n\nBecause it does seem prone to annoying mishaps, I think a change like\nthis is probably a good idea. It's not a confusing concept, because it's\nfamiliar from how branch names with '/' in them already work.\n\nWhat would the 'git remote' porcelain do in cases where remotes like\nthis already exist? I think, from this patch, nothing, since it's\nonly changing add()?\n"}]}