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

From: Ingo Rohloff <ingo.rohloff@lauterbach.com>
Date: Nov 7, 2019, 12:54 UTC
Message-ID: <20191107135409.13fa0336@ingpc3.intern.lauterbach.com>
In-Reply-To: <xmqqftj0qkzm.fsf@gitster-ct.c.googlers.com>

On Thu, 07 Nov 2019 15:05:49 +0900 Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Ingo Rohloff <ingo.rohloff@lauterbach.com> writes:
> 
> > ...
> > 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?

I think I did not explain the intention that well. What I basically want to avoid is a situation in which there is no way at all to refer unambiguously to a particular reference.

Right now this is easily possible. I use the following sequence of commands to get into this situation ("gcc_build" is just a test repository with nothing special).

  rm -rf gcc_build
  git clone ssh://ds1/home/irohloff/git/gcc_build.git
  cd gcc_build
  git checkout -b work
  echo "work..." >> 00_readme.txt
  git commit -a -m "work..."

  git branch origin/master
  git branch remotes/origin/master
  git branch refs/remotes/origin/master
  git branch -avv

The last "git branch -avv" outputs:

  master                     520d29e [refs/remotes/origin/master] initial scripts for building cross compiler GCC
  origin/master              b8efa13 work...
  refs/remotes/origin/master b8efa13 work...
  remotes/origin/master      b8efa13 work...
* work                       b8efa13 work...
  remotes/origin/HEAD        -> refs/remotes/origin/master
  remotes/origin/master      520d29e initial scripts for building cross compiler GCC

So this already is a major confusion, because git reports there are two references with the same name "remotes/origin/master" which point to DIFFERENT objects.

What's worse: I cannot figure out a way to unambiguously refer to the remote tracking branch remotes/origin/master:

git log origin/master
  warning: refname 'origin/master' is ambiguous.
  True: This refers to both
      refs/remotes/origin/master
      refs/heads/origin/master

git log remotes/origin/master
  warning: refname 'remotes/origin/master' is ambiguous.
  True: This refers to both
      refs/remotes/origin/master
      refs/heads/remotes/origin/master

git branch refs/remotes/origin/master
  warning: refname 'remotes/origin/master' is ambiguous.
  True: This refers to both
      refs/remotes/origin/master
      refs/heads/refs/remotes/origin/master

Now I have run out of ideas how to refer to the remote tracking branch.

So what I want to ensure is that there exists at least one way to address a reference unambiguously. (The SHA-1 is not really an option, because it changes each time you update the branch head.)

I do not want to prevent people from creating ambiguities in general or weird branch names in general, because I think that's a much harder and maybe unsolvable problem. I just want to make sure that there is at least one unambiguous way to refer to a specific reference.

So to answer your questions:

> Is there a reason why notes/ hierarchy is excluded?  
> What about pull/?

I basically wanted to exclude the prefixes which git auto-adds when expanding reference names.

As far as I understand the source code, this means excluding the prefixes 
which are a result of the declaration of
  ref_rev_parse_rules
So these are
  refs/
  remotes/
  tags/
  heads/
Maybe I do not really understand the source code (or my logic is bogus)
and git might somehow expand a reference abbreviation by adding 
"notes/" or "pull/" but I do not know how to trigger this expansion.

That is my rationale behind this set of prefixes and why I did not include things like "notes/", "pull/" ...

Having said this: I think it might be enough to just forbid any local branch names, which have a prefix of "refs/". Rationale behind that: I said that I want to have at least one way to unambiguously refer to a reference. If I forbid "refs/" as prefix, then I think in the above example,

I can always use

   git log refs/remotes/origin/master

because
   .git/refs/heads/refs/....  does not exist.

Thinking more about this:
Preventing local branches names with a "refs/" prefix 
is just the tip of the iceberg.
Someone might use
  git tag refs/remotes/origin/master
or
  git add remote refs/remotes/origin <URL>
  git fetch refs/remotes/origin

After this refs/remotes/origin/master is again ambiguous.

> Should this list be somehow end-user configurable so that say Gerrit users 
> can add for/ and changes/ to the "forbidden" set?

This might be interesting in the future, but at least for now the goal was to prevent a situation in which there is no unambiguous name at all for a reference.

So I was not thinking about a "convenience" feature for users but really preventing to get the repository into an almost unusable state.

So to answer your question: No; right now I did not think that this should be end-user configurable, because the set of prefixes is derived from the declaration of "ref_rev_parse_rules", which contains a non-configurable set of rules.

Show 14 quoted lines

> 
> 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"?
>

Yes you are completely right: I was not at all sure where to put the check. As mentioned above: If the goal is to prevent to get a git repository into a super confusing state, then you probably also need to add constraints for remote names and tag names.

Are there more names which are part of reference names ?

so long
  Ingo