{"thread":{"id":"52202","subject":"[PATCH] branch: Forbid to create local branches with confusing names","startedAt":"2019-11-06T17:04:08Z","lastAt":"2019-11-08T12:45:41Z","messageCount":12,"participants":["Ingo Rohloff","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"385610","messageId":"20191106165628.28563-1-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":null,"subject":"[PATCH] branch: Forbid to create local branches with confusing names","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-06T16:56:28Z","receivedAt":"2019-11-06T17:04:08Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"Without this patch, git allows to do something like this:\n git branch remotes/origin/master\n git branch refs/remotes/origin/master\n git branch heads/master\n git branch tags/v3.4\nAll of these local branch names lead to severe confusion,\nnot only for a user but also for git itself.\n\nThis patch forbids to create local branches, with names which start\nwith any of\n\n refs/\n heads/\n remotes/\n tags/\n\nWith this patch, you might still create these kind of local branches,\nbut now you have to additionally specify the '-f' option.\n\nSigned-off-by: Ingo Rohloff \n---\n branch.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\n This patch refers way back to the discussion from 2014:\n From: Josef Wolf\n To: git@vger.kernel.org\n Subject: error: src refspec refs/heads/master matches more than one.\n Date: Fri, 14 Feb 2014 12:31:36 +0100\n See for example here:\n https://public-inbox.org/git/20140214113136.GA17817@raven.inka.de/\n\n The origin of the problem is, that git has (almost) no constraints\n what kind of names are allowed for local branches.\n There nowadays is a constraint that you are NOT allowed to create\n a branch which is called HEAD. See commit 16169285f1e6 (\"Merge\n branch 'jc/branch-name-sanity'\", 2017-11-28).\n\n In the old mail thread a lot of potential constraints for local\n branch names were discussed; in particular a lot of strategies\n were discussed what kind of local branch names might be a problem\n (the gist is: avoid ambiguities, by finding out which names\n lead to ambiguities NOW).\n\n I personally think it makes more sense to forbid a much\n bigger class of confusing branch names.\n In particular I think all local branch names starting with\n refs/\n heads/\n remotes/\n tags/\n should be forbidden (per default, can still be forced).\n This also avoids trouble for an unforseeable future.\n Example:\n git branch remotes/blub/master\n This might not be a problem now, because you have no \n remote called \"blub\" right now.\n But if you later use\n git remote add blub \n git fetch blub\n you very likely run into trouble.\n\n The above approach still allows you to create local branches\n with a name of the form\n /\n but I cannot see how this might be avoided; remotes might\n be added later so what would you do in the case that a local\n branch already existed named like the remote or a remote\n tracking branch.\n\n With the proposed constraints you are at least are able to use\n heads//\n remotes//\n to differentiate between the two.\n\n This really is an issue; people seem to stumble over it\n and I guess this is particularly true if you control git\n via scripts. See for example (two years later):\n From: Duy Nguyen\n To: Junio C Hamano\n Cc: Git Mailing List ,\n Subject: Re: error: src refspec refs/heads/master matches more than one.\n Date: Wed, 23 Mar 2016 18:17:05 +0700\n\n So with this patch I want to pick up this old discussion yet again.\n\n This code can probably be done a lot better I guess, but I wanted to\n send in something, to start the discussion.\n\ndiff --git a/branch.c b/branch.c\nindex 579494738a..e74872dac5 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -256,6 +256,16 @@ void create_branch(struct repository *r,\n \tint dont_change_ref = 0;\n \tint explicit_tracking = 0;\n \n+\tif (!force && (\n+\t\tstarts_with(name, \"refs/\") ||\n+\t\tstarts_with(name, \"heads/\") ||\n+\t\tstarts_with(name, \"remotes/\") ||\n+\t\tstarts_with(name, \"tags/\")\n+\t)) {\n+\t\tdie(_(\"A local branch name should not start with \"\n+\t\t \"\\\"refs/\\\", \\\"heads/\\\", \\\"remotes/\\\" or \\\"tags/\\\"\"));\n+\t}\n+\n \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n \t\texplicit_tracking = 1;\n \n-- \n2.24.0.1.g6c2c19214d.dirty\n\n"},{"id":"385630","messageId":"nycvar.QRO.7.76.6.1911062101580.46@tvgsbejvaqbjf.bet","threadId":"52202","inReplyTo":"20191106165628.28563-1-ingo.rohloff@lauterbach.com","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-06T22:15:44Z","receivedAt":"2019-11-06T22:16:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ingo,\n\nOn Wed, 6 Nov 2019, Ingo Rohloff wrote:\n\n> Without this patch, git allows to do something like this:\n\nMaybe start the patch with a description of the problem it tries to\nsolve? In other words, I would have appreciated a first paragraph that\nstarts with \"Many Git users ...\".\n\n> git branch remotes/origin/master\n> git branch refs/remotes/origin/master\n> git branch heads/master\n> git branch tags/v3.4\n> All of these local branch names lead to severe confusion,\n> not only for a user but also for git itself.\n>\n> This patch forbids to create local branches, with names which start\n> with any of\n>\n> refs/\n> heads/\n> remotes/\n> tags/\n>\n> With this patch, you might still create these kind of local branches,\n> but now you have to additionally specify the '-f' option.\n>\n> Signed-off-by: Ingo Rohloff \n> ---\n> branch.c | 10 ++++++++++\n> 1 file changed, 10 insertions(+)\n>\n> This patch refers way back to the discussion from 2014:\n> From: Josef Wolf\n> To: git@vger.kernel.org\n> Subject: error: src refspec refs/heads/master matches more than one.\n> Date: Fri, 14 Feb 2014 12:31:36 +0100\n> See for example here:\n> https://public-inbox.org/git/20140214113136.GA17817@raven.inka.de/\n>\n> The origin of the problem is, that git has (almost) no constraints\n> what kind of names are allowed for local branches.\n> There nowadays is a constraint that you are NOT allowed to create\n> a branch which is called HEAD. See commit 16169285f1e6 (\"Merge\n> branch 'jc/branch-name-sanity'\", 2017-11-28).\n>\n> In the old mail thread a lot of potential constraints for local\n> branch names were discussed; in particular a lot of strategies\n> were discussed what kind of local branch names might be a problem\n> (the gist is: avoid ambiguities, by finding out which names\n> lead to ambiguities NOW).\n>\n> I personally think it makes more sense to forbid a much\n> bigger class of confusing branch names.\n> In particular I think all local branch names starting with\n> refs/\n> heads/\n> remotes/\n> tags/\n> should be forbidden (per default, can still be forced).\n> This also avoids trouble for an unforseeable future.\n> Example:\n> git branch remotes/blub/master\n> This might not be a problem now, because you have no\n> remote called \"blub\" right now.\n> But if you later use\n> git remote add blub \n> git fetch blub\n> you very likely run into trouble.\n>\n> The above approach still allows you to create local branches\n> with a name of the form\n> /\n> but I cannot see how this might be avoided; remotes might\n> be added later so what would you do in the case that a local\n> branch already existed named like the remote or a remote\n> tracking branch.\n>\n> With the proposed constraints you are at least are able to use\n> heads//\n> remotes//\n> to differentiate between the two.\n>\n> This really is an issue; people seem to stumble over it\n> and I guess this is particularly true if you control git\n> via scripts. See for example (two years later):\n> From: Duy Nguyen\n> To: Junio C Hamano\n> Cc: Git Mailing List ,\n> Subject: Re: error: src refspec refs/heads/master matches more than one.\n> Date: Wed, 23 Mar 2016 18:17:05 +0700\n>\n> So with this patch I want to pick up this old discussion yet again.\n>\n> This code can probably be done a lot better I guess, but I wanted to\n> send in something, to start the discussion.\n\nA lot of this text should probably go into the commit message itself,\npossibly with accompanying Message-IDs or even public-inbox URLs right\naway.\n\n>\n> diff --git a/branch.c b/branch.c\n> index 579494738a..e74872dac5 100644\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -256,6 +256,16 @@ void create_branch(struct repository *r,\n> \tint dont_change_ref = 0;\n> \tint explicit_tracking = 0;\n>\n> +\tif (!force && (\n> +\t\tstarts_with(name, \"refs/\") ||\n> +\t\tstarts_with(name, \"heads/\") ||\n> +\t\tstarts_with(name, \"remotes/\") ||\n> +\t\tstarts_with(name, \"tags/\")\n\nA more common problem for me, personally, is when I manage to fool\nmyself by creating a local branch like `origin/master`. Clearly, I want\nto refer to the remote-tracking branch, but by mistake I create a local\nbranch that now conflicts with the (short) name of the remote-tracking\nbranch.\n\nTo remedy this, you would not only have to ensure that `create_branch()`\nverifies that the branch name does not have a `/` prefix\nwhere `` refers to a valid remote, but you would also need\na corresponding patch that teaches `git add remote ` to\nverify that no local branch starts with `/`.\n\nWhat do you think?\n\nCiao,\nJohannes\n\n> +\t)) {\n> +\t\tdie(_(\"A local branch name should not start with \"\n> +\t\t \"\\\"refs/\\\", \\\"heads/\\\", \\\"remotes/\\\" or \\\"tags/\\\"\"));\n> +\t}\n> +\n> \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n> \t\texplicit_tracking = 1;\n>\n> --\n> 2.24.0.1.g6c2c19214d.dirty\n>\n>\n"},{"id":"385665","messageId":"xmqqftj0qkzm.fsf@gitster-ct.c.googlers.com","threadId":"52202","inReplyTo":"20191106165628.28563-1-ingo.rohloff@lauterbach.com","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-07T06:05:49Z","receivedAt":"2019-11-07T06:05:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ingo Rohloff writes:\n\n> Without this patch, git allows to do something like this:\n> git branch remotes/origin/master\n> git branch refs/remotes/origin/master\n> git branch heads/master\n> git branch tags/v3.4\n> All of these local branch names lead to severe confusion,\n> not only for a user but also for git itself.\n>\n> This patch forbids to create local branches, with names which start\n> with any of\n>\n> refs/\n> heads/\n> remotes/\n> tags/\n\nIs there a reason why notes/ hierarchy is excluded? What about\npull/? Are we dealing with an unbounded set? Should this list be\nsomehow end-user configurable so that say Gerrit users can add for/\nand changes/ to the \"forbidden\" set?\n\n> With this patch, you might still create these kind of local branches,\n> but now you have to additionally specify the '-f' option.\n\nThis is not a change to builtin/branch.c, so other commands that\ncall create_branch() would be affected---are they all equipped to\ntoggle on the same bit that is affected by the '-f' option you have\nin mind (presumably \"git branch -f\")? Wouldn't forcing for those\nother command have undesirable side effects?\n\nI didn't check the code, but I think \"checkout -b\" also calls\ncreate_branch() so presumably it also is affected. Using \"-B\"\ninstead of \"-b\" for \"checkout\" might pass the force bit on, but\ntypically that is done only to recreate an existing branch. Is it a\ngood idea to change the meaning of \"-B\" to also mean \"do not bother\nchecking the sanity of the branch name\"?\n\n> --- a/branch.c\n> +++ b/branch.c\n> @@ -256,6 +256,16 @@ void create_branch(struct repository *r,\n> \tint dont_change_ref = 0;\n> \tint explicit_tracking = 0;\n> \n> +\tif (!force && (\n> +\t\tstarts_with(name, \"refs/\") ||\n> +\t\tstarts_with(name, \"heads/\") ||\n> +\t\tstarts_with(name, \"remotes/\") ||\n> +\t\tstarts_with(name, \"tags/\")\n> +\t)) {\n> +\t\tdie(_(\"A local branch name should not start with \"\n> +\t\t \"\\\"refs/\\\", \\\"heads/\\\", \\\"remotes/\\\" or \\\"tags/\\\"\"));\n> +\t}\n> +\n> \tif (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)\n> \t\texplicit_tracking = 1;\n"},{"id":"385703","messageId":"20191107135409.13fa0336@ingpc3.intern.lauterbach.com","threadId":"52202","inReplyTo":"xmqqftj0qkzm.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T12:54:09Z","receivedAt":"2019-11-07T12:54:19Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"On Thu, 07 Nov 2019 15:05:49 +0900\nJunio C Hamano wrote:\n> Ingo Rohloff writes:\n> \n> > ...\n> > This patch forbids to create local branches, with names which start\n> > with any of\n> >\n> > refs/\n> > heads/\n> > remotes/\n> > tags/ \n> \n> Is there a reason why notes/ hierarchy is excluded? What about\n> pull/? Are we dealing with an unbounded set? Should this list be\n> somehow end-user configurable so that say Gerrit users can add for/\n> and changes/ to the \"forbidden\" set?\n\nI think I did not explain the intention that well.\nWhat I basically want to avoid is a situation in which there is\nno way at all to refer unambiguously to a particular reference.\n\nRight now this is easily possible. \nI use the following sequence of commands to get into this\nsituation (\"gcc_build\" is just a test repository with nothing special).\n\n rm -rf gcc_build\n git clone ssh://ds1/home/irohloff/git/gcc_build.git\n cd gcc_build\n git checkout -b work\n echo \"work...\" >> 00_readme.txt\n git commit -a -m \"work...\"\n\n git branch origin/master\n git branch remotes/origin/master\n git branch refs/remotes/origin/master\n git branch -avv\n\nThe last \"git branch -avv\" outputs:\n\n master 520d29e [refs/remotes/origin/master] initial scripts for building cross compiler GCC\n origin/master b8efa13 work...\n refs/remotes/origin/master b8efa13 work...\n remotes/origin/master b8efa13 work...\n* work b8efa13 work...\n remotes/origin/HEAD -> refs/remotes/origin/master\n remotes/origin/master 520d29e initial scripts for building cross compiler GCC\n\n\nSo this already is a major confusion, because git reports there are two references \nwith the same name \"remotes/origin/master\" which point to DIFFERENT objects.\n\nWhat's worse: I cannot figure out a way to unambiguously refer to \nthe remote tracking branch remotes/origin/master:\n\ngit log origin/master\n warning: refname 'origin/master' is ambiguous.\n True: This refers to both\n refs/remotes/origin/master\n refs/heads/origin/master\n\ngit log remotes/origin/master\n warning: refname 'remotes/origin/master' is ambiguous.\n True: This refers to both\n refs/remotes/origin/master\n refs/heads/remotes/origin/master\n\ngit branch refs/remotes/origin/master\n warning: refname 'remotes/origin/master' is ambiguous.\n True: This refers to both\n refs/remotes/origin/master\n refs/heads/refs/remotes/origin/master\n\nNow I have run out of ideas how to refer to the remote tracking branch.\n\nSo what I want to ensure is that there exists at least one way to \naddress a reference unambiguously.\n(The SHA-1 is not really an option, because it changes each time you\nupdate the branch head.)\n\nI do not want to prevent people from creating ambiguities in general\nor weird branch names in general, because I think that's a much harder and \nmaybe unsolvable problem.\nI just want to make sure that there is at least one unambiguous way to \nrefer to a specific reference.\n\nSo to answer your questions:\n\n> Is there a reason why notes/ hierarchy is excluded? \n> What about pull/?\n\nI basically wanted to exclude the prefixes which git auto-adds when \nexpanding reference names.\n\nAs far as I understand the source code, this means excluding the prefixes \nwhich are a result of the declaration of\n ref_rev_parse_rules\nSo these are\n refs/\n remotes/\n tags/\n heads/\nMaybe I do not really understand the source code (or my logic is bogus)\nand git might somehow expand a reference abbreviation by adding \n\"notes/\" or \"pull/\" but I do not know how to trigger this expansion.\n\nThat is my rationale behind this set of prefixes and why \nI did not include things like \"notes/\", \"pull/\" ...\n\nHaving said this: \nI think it might be enough to just forbid any local branch names,\nwhich have a prefix of \"refs/\".\nRationale behind that: \nI said that I want to have at least one way to unambiguously \nrefer to a reference. \nIf I forbid \"refs/\" as prefix, then I think in the above example,\n\nI can always use\n\n git log refs/remotes/origin/master\n\nbecause\n .git/refs/heads/refs/.... does not exist.\n\nThinking more about this:\nPreventing local branches names with a \"refs/\" prefix \nis just the tip of the iceberg.\nSomeone might use\n git tag refs/remotes/origin/master\nor\n git add remote refs/remotes/origin \n git fetch refs/remotes/origin\n\nAfter this refs/remotes/origin/master is again ambiguous.\n\n> Should this list be somehow end-user configurable so that say Gerrit users \n> can add for/ and changes/ to the \"forbidden\" set?\n\nThis might be interesting in the future, but at least for now the goal was to\nprevent a situation in which there is no unambiguous name at all for a reference.\n\nSo I was not thinking about a \"convenience\" feature for users but really preventing\nto get the repository into an almost unusable state.\n\nSo to answer your question: No; right now I did not think that this should be \nend-user configurable, because the set of prefixes is derived from the declaration \nof \"ref_rev_parse_rules\", which contains a non-configurable set of rules.\n\n\n\n> \n> This is not a change to builtin/branch.c, so other commands that\n> call create_branch() would be affected---are they all equipped to\n> toggle on the same bit that is affected by the '-f' option you have\n> in mind (presumably \"git branch -f\")? Wouldn't forcing for those\n> other command have undesirable side effects?\n> \n> I didn't check the code, but I think \"checkout -b\" also calls\n> create_branch() so presumably it also is affected. Using \"-B\"\n> instead of \"-b\" for \"checkout\" might pass the force bit on, but\n> typically that is done only to recreate an existing branch. Is it a\n> good idea to change the meaning of \"-B\" to also mean \"do not bother\n> checking the sanity of the branch name\"?\n> \n\nYes you are completely right: \nI was not at all sure where to put the check.\nAs mentioned above: If the goal is to prevent to get a git repository into\na super confusing state, then you probably also need to add constraints for\nremote names and tag names.\n\nAre there more names which are part of reference names ?\n\nso long\n Ingo\n"},{"id":"385731","messageId":"20191107200421.6214b2e1@ingpc3.intern.lauterbach.com","threadId":"52202","inReplyTo":"nycvar.QRO.7.76.6.1911062101580.46@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:04:21Z","receivedAt":"2019-11-07T19:04:26Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"Hello Johannes,\n\nOn Wed, 6 Nov 2019 23:15:44 +0100 (CET)\nJohannes Schindelin wrote:\n\n> Hi Ingo,\n> \n> On Wed, 6 Nov 2019, Ingo Rohloff wrote:\n> \n> > Without this patch, git allows to do something like this: \n> \n> Maybe start the patch with a description of the problem it tries to\n> solve? In other words, I would have appreciated a first paragraph that\n> starts with \"Many Git users ...\".\n\nThat's actually one of the problems: \nIt's not clear what exactly the problem is :-).\n\nAfter thinking about it more: The minimal goal I can think of is to make sure \nthat if you use\n git log refs/\n\nyou will never get a \n warning: refname '...' is ambiguous\n\nRationale behind that: If even \"refs/\" gives you this warning, \nthen you might be in a lot of trouble. It means even giving a \"full\" refname \nis not enough to resolve ambiguities.\nI think this is bad, because it means it might be hard to get out of this \nsituation, because you might get the \"ambiguous\" warnings when you try to \nget rid of the offending refnames.\n\n\n> \n> A lot of this text should probably go into the commit message itself,\n> possibly with accompanying Message-IDs or even public-inbox URLs right\n> away.\n\nI did read \"Documentation/SubmittingPatches\". There it says:\n\n Try to make sure your explanation can be understood\n without external resources. Instead of giving a URL to a \n mailing list archive, summarize the relevant points of \n the discussion.\n\nso that's what I tried to do.\n\n> \n> A more common problem for me, personally, is when I manage to fool\n> myself by creating a local branch like `origin/master`. Clearly, I want\n> to refer to the remote-tracking branch, but by mistake I create a local\n> branch that now conflicts with the (short) name of the remote-tracking\n> branch.\n> \n> To remedy this, you would not only have to ensure that `create_branch()`\n> verifies that the branch name does not have a `/` prefix\n> where `` refers to a valid remote, but you would also need\n> a corresponding patch that teaches `git add remote ` to\n> verify that no local branch starts with `/`.\n> \n> What do you think?\n> \n\nI agree: When I first started to use git, I was quite surprised that this\n\"double naming\" is allowed.\n\nBut I also think, this is for another patch series; you probably need to \nhonor \"--force\", or even add a git configuration option to allow this\nanyway.\n\nI am able to imagine that people intentionally set up a local branch\ncalled \"refs/heads/repoX/master\" which tracks \"refs/remotes/repoX/master\".\n\nFor me this sounds like an unnecessary complication (because now you always\nhave to use the \"long\" refname), but if you put some software on top of git, \nI can imagine that this might make a lot of sense...\n\nI am not enough of a git wizard to fully grasp the implications here.\n\nso long\n Ingo\n\n\n"},{"id":"385732","messageId":"20191107190750.26674-1-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":"20191106165628.28563-1-ingo.rohloff@lauterbach.com","subject":"[PATCH v2 0/4] Do not create new refnames \"refs\" or \"refs/*\"","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:07:46Z","receivedAt":"2019-11-07T19:08:07Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"After the previous round of input:\nThe intention of this patch series is to make sure, that you always\nmight use \"refs/\" to unambiguously refer to a specific \nreference.\n\nOr put in another way: With this patch series, refnames for \"git log\", \nstarting with \"refs/\" should never result in a\n warning: refname '...' is ambiguous.\nExceptions: \nYou already have strangely named references in your repository.\nYou pull such strangely named references from a remote repository.\n\nThis patch series does not prevent you from creating various other\nforms of ambiguous refnames. Example:\n\n git branch origin/master\n\nThis will very likely result in an ambiguity, because now the\nreferences\n\n refs/heads/origin/master\n refs/remotes/origin/master\n\nwill very likely both exist. \"origin/master\" matches both refnames.\n\nThis patch series forbids to create new reference names, which start\nwith \"refs\" or \"refs/*\" with the commands\n\n git branch \n git checkout -b \n git tag \n git remote add \n\nNote: This patch does NOT prevent you from working with references\nwhich already exist. It just prevents you from creating new ones\nwith the commands listed above.\n\nThat's also the reason why the '--force' option is not evaluated here.\nI cannot think of a good reason, why you ever should want to create\nrefnames matching any of the following patterns:\n refs/heads/refs/*\n refs/tags/refs/*\n refs/remotes/refs/*\n \n\nIngo Rohloff (4):\n refs: new function newname_has_bad_prefix\n branch: Prevent creation of local branches named \"refs\" or \"refs/*\"\n remote: Prevent users from creating remotes named \"refs\" or \"refs/*\"\n tag: Prevent users from creating tags named \"refs\" or \"refs/*\"\n\n builtin/branch.c | 6 ++++++\n builtin/checkout.c | 3 +++\n builtin/remote.c | 3 +++\n builtin/tag.c | 3 +++\n refs.c | 30 ++++++++++++++++++++++++++++++\n refs.h | 2 ++\n 6 files changed, 47 insertions(+)\n\n-- \n2.24.0\n\n"},{"id":"385733","messageId":"20191107190750.26674-2-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":"20191107190750.26674-1-ingo.rohloff@lauterbach.com","subject":"[PATCH v2 1/4] refs: new function newname_has_bad_prefix","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:07:47Z","receivedAt":"2019-11-07T19:08:08Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"The function is intended to check if a user specified name\nfor a new reference (for remotes, branches or tags) has\na bad prefix.\n\nIntention: Prevent users from creating tags/remotes/branches\nwhich are named \"refs\" or \"refs/...\"\n\nSigned-off-by: Ingo Rohloff \n---\n refs.c | 30 ++++++++++++++++++++++++++++++\n refs.h | 2 ++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/refs.c b/refs.c\nindex 1ab0bb54d3..49e4f662df 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -321,6 +321,36 @@ int ref_exists(const char *refname)\n \treturn refs_ref_exists(get_main_ref_store(the_repository), refname);\n }\n \n+\n+static int starts_with_component(const char *str, const char *prefix)\n+{\n+\tfor (; ; str++, prefix++) {\n+\t\tif (!*prefix) {\n+\t\t\tif (!*str || *str == '/')\n+\t\t\t\treturn 1;\n+\t\t\treturn 0;\n+\t\t} else if (*str != *prefix)\n+\t\t\treturn 0;\n+\t}\n+}\n+\n+static const char *newname_bad_prefixes[] = {\n+\t\"refs\",\n+\tNULL\n+};\n+\n+int newname_has_bad_prefix(const char *refname)\n+{\n+\tconst char **p;\n+\tp = newname_bad_prefixes;\n+\twhile (*p) {\n+\t\tif (starts_with_component(refname, *p))\n+\t\t\treturn 1;\n+\t\tp++;\n+\t}\n+\treturn 0;\n+}\n+\n static int match_ref_pattern(const char *refname,\n \t\t\t const struct string_list_item *item)\n {\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..00c11ec589 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -107,6 +107,8 @@ int refs_verify_refname_available(struct ref_store *refs,\n \n int ref_exists(const char *refname);\n \n+int newname_has_bad_prefix(const char *refname);\n+\n int should_autocreate_reflog(const char *refname);\n \n int is_branch(const char *refname);\n-- \n2.24.0\n\n"},{"id":"385734","messageId":"20191107190750.26674-4-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":"20191107190750.26674-1-ingo.rohloff@lauterbach.com","subject":"[PATCH v2 3/4] remote: Prevent users from creating remotes named \"refs\" or \"refs/*\"","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:07:49Z","receivedAt":"2019-11-07T19:08:10Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"To explain the intention, here is an example:\n\nA user executes\n git remote add refs/heads \n git fetch refs/heads\nafter this operation\n git log refs/heads/master\nwill very likely complain that this reference is ambiguous.\n\nThe reason is, that you now very likely have the following two\nreferences which both match:\n\n refs/heads/master\n refs/remotes/refs/heads/master\n\ngit cannot decide which of the two references is meant.\n\nBy preventing the creation of remotes which are named\n refs or refs/*\nthis issue is circumvented:\n git log refs/*\nwill never refer to a remote tracking branch located under\n refs/remotes/refs/*\nbecause such remotes should not exist.\n\nSigned-off-by: Ingo Rohloff \n---\n builtin/remote.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5591cef775..2272c16d18 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -189,6 +189,9 @@ static int add(int argc, const char **argv)\n \tname = argv[0];\n \turl = argv[1];\n \n+\tif (newname_has_bad_prefix(name))\n+\t\tdie(_(\"Invalid new remote name: '%s'\"), name);\n+\n \tremote = remote_get(name);\n \tif (remote_is_configured(remote, 1))\n \t\tdie(_(\"remote %s already exists.\"), name);\n-- \n2.24.0\n\n"},{"id":"385735","messageId":"20191107190750.26674-5-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":"20191107190750.26674-1-ingo.rohloff@lauterbach.com","subject":"[PATCH v2 4/4] tag: Prevent users from creating tags named \"refs\" or \"refs/*\"","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:07:50Z","receivedAt":"2019-11-07T19:08:11Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"To explain the intention, here is an example:\n\nA user executes\n git tag -m \"a tag\" refs/heads/master\nafter this operation\n git log refs/heads/master\nwill very likely complain that this reference is ambiguous.\n\nThe reason is, that you now very likely have the following two\nreferences which both match:\n\n refs/heads/master\n refs/tags/refs/heads/master\n\ngit cannot decide which of the two references is meant.\n\nBy preventing the creation of tags which are named\n refs or refs/*\nthis issue is circumvented:\n git log refs/*\nwill never refer to a tag located under\n refs/tags/refs/*\nbecause such a tag should not exist.\n\nSigned-off-by: Ingo Rohloff \n---\n builtin/tag.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex e0a4c25382..c818fe3fcd 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -537,6 +537,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \n \ttag = argv[0];\n \n+\tif (!cmdmode && newname_has_bad_prefix(tag))\n+\t\tdie(_(\"Invalid new tag name: '%s'\"), tag);\n+\n \tobject_ref = argc == 2 ? argv[1] : \"HEAD\";\n \tif (argc > 2)\n \t\tdie(_(\"too many params\"));\n-- \n2.24.0\n\n"},{"id":"385736","messageId":"20191107190750.26674-3-ingo.rohloff@lauterbach.com","threadId":"52202","inReplyTo":"20191107190750.26674-1-ingo.rohloff@lauterbach.com","subject":"[PATCH v2 2/4] branch: Prevent creation of local branches named \"refs\" or \"refs/*\"","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-07T19:07:48Z","receivedAt":"2019-11-07T19:08:17Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"To explain the intention, here is an example:\n\nA user executes\n git branch refs/remotes/origin/master\nor\n git checkout -b refs/remotes/origin/master\nafter this operation\n git log refs/remotes/origin/master\nwill very likely complain that this reference is ambiguous.\n\nThe reason is, that you now very likely have the following two\nreferences which both match:\n\n refs/remotes/origin/master\n refs/heads/refs/remotes/origin/master\n\ngit cannot decide which of the two references is meant.\n\nBy preventing the creation of local branches which are named\n refs or refs/*\nthis issue is circumvented:\n git log refs/*\nwill never refer to local branches located under\n refs/heads/refs/*\nbecause such local branches should not exist.\n\nSigned-off-by: Ingo Rohloff \n---\n builtin/branch.c | 6 ++++++\n builtin/checkout.c | 3 +++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2ef214632f..c2e45ff52c 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -497,6 +497,9 @@ static void copy_or_rename_branch(const char *oldname, const char *newname, int\n \tconst char *interpreted_newname = NULL;\n \tint recovery = 0;\n \n+\tif (newname_has_bad_prefix(newname))\n+\t\tdie(_(\"Invalid new branch name: '%s'\"), newname);\n+\n \tif (!oldname) {\n \t\tif (copy)\n \t\t\tdie(_(\"cannot copy the current branch while not on any.\"));\n@@ -844,6 +847,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tif (track == BRANCH_TRACK_OVERRIDE)\n \t\t\tdie(_(\"the '--set-upstream' option is no longer supported. Please use '--track' or '--set-upstream-to' instead.\"));\n \n+\t\tif (newname_has_bad_prefix(argv[0]))\n+\t\t\tdie(_(\"Invalid new branch name: '%s'\"), argv[0]);\n+\n \t\tcreate_branch(the_repository,\n \t\t\t argv[0], (argc == 2) ? argv[1] : head,\n \t\t\t force, 0, reflog, quiet, track);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 3634a3dac1..51ac2cae43 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1566,6 +1566,9 @@ static int checkout_main(int argc, const char **argv, const char *prefix,\n \tif (opts->new_orphan_branch)\n \t\topts->new_branch = opts->new_orphan_branch;\n \n+\tif (opts->new_branch && newname_has_bad_prefix(opts->new_branch))\n+\t\tdie(_(\"Invalid new branch name: '%s'\"), opts->new_branch);\n+\n \t/* --track without -b/-B/--orphan should DWIM */\n \tif (opts->track != BRANCH_TRACK_UNSPECIFIED && !opts->new_branch) {\n \t\tconst char *argv0 = argv[0];\n-- \n2.24.0\n\n"},{"id":"385747","messageId":"xmqq36ezoz6i.fsf@gitster-ct.c.googlers.com","threadId":"52202","inReplyTo":"20191107135409.13fa0336@ingpc3.intern.lauterbach.com","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-08T02:54:29Z","receivedAt":"2019-11-08T02:54:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ingo Rohloff writes:\n\n> On Thu, 07 Nov 2019 15:05:49 +0900\n> Junio C Hamano wrote:\n>> Ingo Rohloff writes:\n>> \n>> > ...\n>> > This patch forbids to create local branches, with names which start\n>> > with any of\n>> >\n>> > refs/\n>> > heads/\n>> > remotes/\n>> > tags/ \n>> \n>> Is there a reason why notes/ hierarchy is excluded? What about\n>> pull/? Are we dealing with an unbounded set? Should this list be\n>> somehow end-user configurable so that say Gerrit users can add for/\n>> and changes/ to the \"forbidden\" set?\n>\n> I think I did not explain the intention that well.\n> What I basically want to avoid is a situation in which there is\n> no way at all to refer unambiguously to a particular reference.\n\nHmph, I thought this was a solved problem, but maybe I am still\nmisunderstanding you.\n\nWhen you have a possibly ambigous branch whose name is $X, whether\n$X begins with one of the strings you listed above or not, you can\nalways\n\n - refer to the commit at the tip of that branch by saying\n refs/heads/$X (e.g. \"git show refs/heads/$X\") and that always\n refers to the commit, even if there are other branches and tags\n that may begin with refs/ or refs/heads/. You would of course\n get a warning about possible ambiguity because saying just $X\n (e.g. \"git show $X\") may not refer to refs/heads/$X when you have\n other refs that make $X ambiguous.\n\n - refer to the branch by saying $X (and not refs/heads/$X).\n\nThe thing to know is when you are naming a branch and when you are\nnaming a commit.\n\n - \"git branch -d $X\" is referring to the branch itself and removes\n refs/heads/$X. \n\n - \"git branch new \" uses to specify the\n commit to begin the 'new' branch at, and does not necessarily\n take a branch name, so you should say refs/heads/$X. You may not\n be able to rely on lazy-man's short-hands, like \"git checkout -t\n origin/master\" that DWIMs what you did not say, as you have to\n say refs/heads/$X that is *not* in the /\n form (you can still do so with \"git checkout -b master\n refs/heads/$X\" followed by necessary configuration updates), but\n that is a price to pay for using ambiguous names.\n\nAnd for those who do not want to pay for using ambiguous names, we\nissue warnings when resolving refnames.\n\n"},{"id":"385781","messageId":"20191108134536.43d6e476@ingpc3.intern.lauterbach.com","threadId":"52202","inReplyTo":"xmqq36ezoz6i.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] branch: Forbid to create local branches with confusing names","fromName":"Ingo Rohloff","fromEmail":"ingo.rohloff@lauterbach.com","sentAt":"2019-11-08T12:45:36Z","receivedAt":"2019-11-08T12:45:41Z","isPatch":true,"sender":{"key":"ingo.rohloff@lauterbach.com","avatar":null},"body":"Hello Junio,\n\n> > I think I did not explain the intention that well.\n> > What I basically want to avoid is a situation in which there is\n> > no way at all to refer unambiguously to a particular reference. \n> \n> Hmph, I thought this was a solved problem, but maybe I am still\n> misunderstanding you.\n\nYou are right, this is partly solved: \nDeleting such a reference to a local branch is always possible.\nBut these kind of problems are not solved fully.\n\nTo explain my issue more concisely:\nI start in any \"regular\" git repository.\n Regular == Repository was cloned from somewhere so a remote\n named \"origin\" and a remote-tracking branch named \"origin/master\"\n do exist. \n\nI fire off the following four commands\n(Yes: This is malicious and stupid ;-))\n\n git checkout master\n git branch origin/master\n git branch remotes/origin/master\n git branch refs/remotes/origin/master\n\ngit does not complain at all here.\n\nThen I try the following three commands.\n\n git branch somework origin/master\n warning: refname 'origin/master' is ambiguous.\n fatal: Ambiguous object name: 'origin/master'.\n\n git branch somework remotes/origin/master\n warning: refname 'remotes/origin/master' is ambiguous.\n fatal: Ambiguous object name: 'remotes/origin/master'.\n\n git branch somework refs/remotes/origin/master\n warning: refname 'refs/remotes/origin/master' is ambiguous.\n fatal: Ambiguous object name: 'refs/remotes/origin/master'.\n\n\nQUESTION: \nI think I am lost now. That's where I might have overlooked something ?\n\n\n\nI might continue with (this is the solved case):\n\n git branch -d refs/remotes/origin/master\n Deleted branch refs/remotes/origin/master (was 3454f30).\n\nSounds rather scary (because this sounds like you deleted a \nremote-tracking branch), but actually does the right thing I guess.\n(The command deletes refs/heads/refs/remotes/origin/master)\n\n\nAfter which this succeeds:\n\n git branch somework refs/remotes/origin/master\n Branch 'somework' set up to track remote branch 'master' from 'origin'.\n\n\n\n\nPATCH:\nMake this fail:\n\n git branch refs/remotes/origin/master\n fatal: Invalid new branch name: 'refs/remotes/origin/master'\n\nThis avoids the failure for \n\n git branch somework refs/remotes/origin/master\n\n\nand to avoid very similar issues make these fail too:\n\n git tag -m \"a tag\" refs/remotes/origin/master\n fatal: Invalid new tag name: 'refs/remotes/origin/master'\n\n git remote add refs/heads ssh://ds1/home/irohloff/git/gcc_build.git\n fatal: Invalid new remote name: 'refs/heads'\n\n\nAll of these examples use really pathological names for tags/remotes/branches.\nI cannot believe that anyone wants to do this intentionally.\n\nQUESTION:\nAre there more user created, command line specified refnames \nin addition to tags/remotes/branches ?\n\n\n\n\nIf you have time:\n\nSome more background.\nThe whole idea behind the patch: \nMake sure \"refs/\" is a \"unique\" prefix, which only\nappears as \".git/refs/\".\nThis should ensure that \"refs/\" only matches\nto the very first entry from:\n\tstatic const char *ref_rev_parse_rules[] = {\n\t\t\"%.*s\",\n\t\t\"refs/%.*s\",\n\t\t\"refs/tags/%.*s\",\n\t\t\"refs/heads/%.*s\",\n\t\t\"refs/remotes/%.*s\",\n\t\t\"refs/remotes/%.*s/HEAD\",\n\t\tNULL\n\t}; \n\nSo goal: Make sure\n refs/refs/* does not exist\n refs/tags/refs/* does not exist\n refs/heads/refs/* does not exist\n refs/remotes/refs/* does not exist\n\nTo avoid the existence of refs/remotes/refs/* it is necessary to \nalso prohibit a standalone \"refs\" as remote name (not just \"refs/*\");\nand to handle that more easily I also prohibit a standalone \"refs\"\nfor tags and branches.\n\nOf course you might still create all these nasty subdirs with plumbing.\nI try to avoid that this is done with porcelain.\n(At least that's as far as I understand git terminology.)\nOf course future git extensions might try to create something like\n .git/refs/refs/*\nbut since these extensions are reviewed, I guess it is easy to nudge\nauthors of extensions (like git-svn, git-bisect, ...) to NOT do this.\n\nso long\n Ingo\n\n\nPS: \nI really think per default more prefixes than just \"refs/\" should\nbe forbidden when creating tags/remotes/branches.\nBut I also agree with you that this is much less straight forward\n(Which prefixes to forbid ? Config option ? How much does this break ? ...).\nAs far as I can tell tags/remotes/branches, which \nare called \"refs/*\" or \"refs\" are completely pathological; \nI think unconditionally forbidding to create these kind of \nrefnames for tags/remotes/branches with porcelain is OK.\n\nBTW: This is also quite confusing \n(but does not really hurt and is consistent with what you described)\n\n git branch -r -d refs/remotes/origin/master\n error: remote-tracking branch 'refs/remotes/origin/master' not found.\n\nWhat is meant here is I think\n\n remote-tracking branch 'refs/remotes/refs/remotes/origin/master' not found.\n"}]}