Re: [PATCH] check-ref-format: require a repository for --branch
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 17, 2017, 21:30 UTC
- Message-ID
- <xmqqy3qhevaq.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170817102217.3yw7uxnkupdy3lh5@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 36 quoted lines
> 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.This "just going to output the exact input" is not entirely correct; there is just one use case for it.
"git check-ref-format --branch a..b" would fail with a helpful error message, while the command run with "a.b" would tell you the name is safe.
Use of 'check-ref-format --branch "@{-1}"' *IS* a nonsense, whether it is inside or outside a repository; it is OK to fail it outside a repository and I would say it is even OK to fail it inside a repository. After all "check-ref-format" is about checking if the string is a sensible name to use.
I think calling interpret_branch_name() in the codepath is a mistake and we should instead report if "refs/heads/@{-1}" literally is a valid refname we could create instead.