On Mon, Jul 17, 2017 at 10:27:09AM -0700, Jonathan Nieder wrote:
> > --- 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
