Re: [BUG] branch renamed to 'HEAD'

From: Jeff King <peff@peff.net>
Date: Feb 28, 2017, 12:06 UTC
Message-ID: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
In-Reply-To: <20170228005302.k6fyfinaxyl3ti76@sigill.intra.peff.net>

On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:

Show 21 quoted lines

> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
> 
> > A flag to affect the behaviour (as opposed to &flag as a secondary
> > return value, like Peff's patch does) can be made to work.  Perhaps
> > a flag that says "keep the input as is if the result is not a local
> > branch name" would pass an input "@" intact and that may be
> > sufficient to allow "git branch -m @" to rename the current branch
> > to "@" (I do not think it is a sensible rename, though ;-).  But
> > probably some callers need to keep the original input and compare
> > with the result to see if we expanded anything if we go that route.
> > At that point, I am not sure if there are much differences in the
> > ease of use between the two approaches.
> 
> I just went into more detail in my reply to Jacob, but I do think this
> is a workable approach (and fortunately we seem to have banned bare "@"
> as a name, along with anything containing "@{}", so I think we would end
> up rejecting these nonsense names).
> 
> I'll see if I can work up a patch. We'll still need to pass the flag
> around through the various functions, but at least it will be a flag and
> not a confusing negated out-parameter.

OK, I have a series which fixes this (diffstat below). When I audited the other callers of interpret_branch_name() and strbuf_branchname(), it turned out to be even more complicated. The callers basically fall into a few buckets:

  1. Callers like get_sha1() and merge_name() pass the result to
     dwim_ref(), and are prepared to handle anything.

  2. Some callers stick "refs/heads/" in front of the result, and
     obviously only want local names. Most of git-branch and
     git-checkout fall into this boat.

  3. "git branch -d" can delete local _or_ remote branches, depending on
     the "-r" flag. So the expansion it wants varies, and we need to
     handle "just local" or "just remote".

So I converted the "only_branch" flag to an "allowed" bit-field. No callers actually ask for more than a single type at once, but it was easy to do it that way. It serves all of the callers, and will easily adapt for the future (e.g., if "git branch -a -d" were ever allowed).

  [1/8]: interpret_branch_name: move docstring to header file
  [2/8]: strbuf_branchname: drop return value
  [3/8]: strbuf_branchname: add docstring
  [4/8]: interpret_branch_name: allow callers to restrict expansions
  [5/8]: t3204: test git-branch @-expansion corner cases
  [6/8]: branch: restrict @-expansions when deleting
  [7/8]: strbuf_check_ref_format(): expand only local branches
  [8/8]: checkout: restrict @-expansions when finding branch

 builtin/branch.c                      |   5 +-
 builtin/checkout.c                    |   2 +-
 builtin/merge.c                       |   2 +-
 cache.h                               |  32 ++++++++-
 refs.c                                |   2 +-
 revision.c                            |   2 +-
 sha1_name.c                           |  76 ++++++++++-----------
 strbuf.h                              |  21 +++++-
 t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++
 9 files changed, 220 insertions(+), 44 deletions(-)
 create mode 100755 t/t3204-branch-name-interpretation.sh

-Peff