Re: [PATCH] check-ref-format: require a repository for --branch

From: Jeff King <peff@peff.net>
Date: Aug 17, 2017, 10:22 UTC
Message-ID: <20170817102217.3yw7uxnkupdy3lh5@sigill.intra.peff.net>
In-Reply-To: <20170717172709.GL93855@aiede.mtv.corp.google.com>

On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:

Show 30 quoted lines

> > --- a/t/t1402-check-ref-format.sh
> > +++ b/t/t1402-check-ref-format.sh
> > @@ -161,6 +161,10 @@ test_expect_success 'check-ref-format --branch from subdir' '
> >  	test "$refname" = "$sha1"
> >  '
> >  
> > +test_expect_success 'check-ref-format --branch from non-repo' '
> > +	test_must_fail nongit git check-ref-format --branch @{-1}
> > +'
> > +
> >  valid_ref_normalized() {
> >  	prereq=
> >  	case $1 in
> 
> I don't think it's right.  Today I can do
> 
> 	$ cd /tmp
> 	$ git check-ref-format --branch master
> 	master
> 
> You might wonder why I'd ever do such a thing.  But that's what "git
> check-ref-format --branch" is for --- it is for taking a <branch>
> argument and turning it into a branch name.  For example, if you have
> a script with an $opt_branch variable that defaults to "master", it
> may do
> 
> 	resolved_branch=$(git check-ref-format --branch "$opt_branch")
> 
> even though it is in a mode that not going to have to use
> $resolved_branch and it is not running from a repository.

I'm not sure I buy that. What does "turning it into a branch name" even mean when you are not in a repository? Clearly @{-1} must fail. And everything else is just going to output the exact input you provided. So any script calling "check-ref-format --branch" outside of a repo would be better off not calling it at all. At best it does nothing, and at worst it's going to give a confusing error when $opt_branch is something like "@{-1}".

A more compelling argument along these lines is something like:

  Accepting --branch outside of a repo is pointless, but it's something
  we've historically accepted. To avoid breaking existing scripts (even
  if they are doing something pointless), we'll continue to allow it.

I'm not sure I buy _that_ line of reasoning either, but it at least makes sense to me (I just think it isn't worth the complexity of trying to audit the innards of interpret_branch_name()).

-Peff