Re: [PATCH] branch: Forbid to create local branches with confusing names
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 6, 2019, 22:15 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1911062101580.46@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20191106165628.28563-1-ingo.rohloff@lauterbach.com>
Hi Ingo,
On Wed, 6 Nov 2019, Ingo Rohloff wrote:
> Without this patch, git allows to do something like this:
Maybe start the patch with a description of the problem it tries to solve? In other words, I would have appreciated a first paragraph that starts with "Many Git users ...".
Show 87 quoted lines
> git branch remotes/origin/master
> git branch refs/remotes/origin/master
> git branch heads/master
> git branch tags/v3.4
> All of these local branch names lead to severe confusion,
> not only for a user but also for git itself.
>
> This patch forbids to create local branches, with names which start
> with any of
>
> refs/
> heads/
> remotes/
> tags/
>
> With this patch, you might still create these kind of local branches,
> but now you have to additionally specify the '-f' option.
>
> Signed-off-by: Ingo Rohloff <ingo.rohloff@lauterbach.com>
> ---
> branch.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> This patch refers way back to the discussion from 2014:
> From: Josef Wolf
> To: git@vger.kernel.org
> Subject: error: src refspec refs/heads/master matches more than one.
> Date: Fri, 14 Feb 2014 12:31:36 +0100
> See for example here:
> https://public-inbox.org/git/20140214113136.GA17817@raven.inka.de/
>
> The origin of the problem is, that git has (almost) no constraints
> what kind of names are allowed for local branches.
> There nowadays is a constraint that you are NOT allowed to create
> a branch which is called HEAD. See commit 16169285f1e6 ("Merge
> branch 'jc/branch-name-sanity'", 2017-11-28).
>
> In the old mail thread a lot of potential constraints for local
> branch names were discussed; in particular a lot of strategies
> were discussed what kind of local branch names might be a problem
> (the gist is: avoid ambiguities, by finding out which names
> lead to ambiguities NOW).
>
> I personally think it makes more sense to forbid a much
> bigger class of confusing branch names.
> In particular I think all local branch names starting with
> refs/
> heads/
> remotes/
> tags/
> should be forbidden (per default, can still be forced).
> This also avoids trouble for an unforseeable future.
> Example:
> git branch remotes/blub/master
> This might not be a problem now, because you have no
> remote called "blub" right now.
> But if you later use
> git remote add blub <URL>
> git fetch blub
> you very likely run into trouble.
>
> The above approach still allows you to create local branches
> with a name of the form
> <remote name>/<something ...>
> but I cannot see how this might be avoided; remotes might
> be added later so what would you do in the case that a local
> branch already existed named like the remote or a remote
> tracking branch.
>
> With the proposed constraints you are at least are able to use
> heads/<remote name>/<something ...>
> remotes/<remote name>/<something ...>
> to differentiate between the two.
>
> This really is an issue; people seem to stumble over it
> and I guess this is particularly true if you control git
> via scripts. See for example (two years later):
> From: Duy Nguyen
> To: Junio C Hamano
> Cc: Git Mailing List <git@vger.kernel.org>,
> Subject: Re: error: src refspec refs/heads/master matches more than one.
> Date: Wed, 23 Mar 2016 18:17:05 +0700
>
> So with this patch I want to pick up this old discussion yet again.
>
> This code can probably be done a lot better I guess, but I wanted to
> send in something, to start the discussion.A lot of this text should probably go into the commit message itself, possibly with accompanying Message-IDs or even public-inbox URLs right away.
Show 14 quoted lines
> > diff --git a/branch.c b/branch.c > index 579494738a..e74872dac5 100644 > --- a/branch.c > +++ b/branch.c > @@ -256,6 +256,16 @@ void create_branch(struct repository *r, > int dont_change_ref = 0; > int explicit_tracking = 0; > > + if (!force && ( > + starts_with(name, "refs/") || > + starts_with(name, "heads/") || > + starts_with(name, "remotes/") || > + starts_with(name, "tags/")
A more common problem for me, personally, is when I manage to fool myself by creating a local branch like `origin/master`. Clearly, I want to refer to the remote-tracking branch, but by mistake I create a local branch that now conflicts with the (short) name of the remote-tracking branch.
To remedy this, you would not only have to ensure that `create_branch()` verifies that the branch name does not have a `<remote-name>/` prefix where `<remote-name>` refers to a valid remote, but you would also need a corresponding patch that teaches `git add remote <nick> <url>` to verify that no local branch starts with `<nick>/`.
What do you think?
Ciao, Johannes
Show 12 quoted lines
> + )) {
> + die(_("A local branch name should not start with "
> + "\"refs/\", \"heads/\", \"remotes/\" or \"tags/\""));
> + }
> +
> if (track == BRANCH_TRACK_EXPLICIT || track == BRANCH_TRACK_OVERRIDE)
> explicit_tracking = 1;
>
> --
> 2.24.0.1.g6c2c19214d.dirty
>
>