Re: [PATCH] branch: Forbid to create local branches with confusing names

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 7, 2019, 06:05 UTC
Message-ID: <xmqqftj0qkzm.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20191106165628.28563-1-ingo.rohloff@lauterbach.com>

Ingo Rohloff <ingo.rohloff@lauterbach.com> writes:

Show 15 quoted lines

> Without this patch, git allows to do something like this:
>   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/

Is there a reason why notes/ hierarchy is excluded? What about pull/? Are we dealing with an unbounded set? Should this list be somehow end-user configurable so that say Gerrit users can add for/ and changes/ to the "forbidden" set?

> With this patch, you might still create these kind of local branches,
> but now you have to additionally specify the '-f' option.

This is not a change to builtin/branch.c, so other commands that call create_branch() would be affected---are they all equipped to toggle on the same bit that is affected by the '-f' option you have in mind (presumably "git branch -f")? Wouldn't forcing for those other command have undesirable side effects?

I didn't check the code, but I think "checkout -b" also calls create_branch() so presumably it also is affected. Using "-B" instead of "-b" for "checkout" might pass the force bit on, but typically that is done only to recreate an existing branch. Is it a good idea to change the meaning of "-B" to also mean "do not bother checking the sanity of the branch name"?

Show 18 quoted lines

> --- 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/")
> +	)) {
> +		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;