Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2017, 01:52 UTC
Message-ID: <xmqqa7z0lgsd.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1511880237.10193.5.camel@gmail.com>

Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:

Show 17 quoted lines

>> I have a mild suspicion that "git checkout -B @{-1}" would want to
>> error out instead of creating a valid new branch whose name is
>> 40-hex that happen to be the name of the commit object you were
>> detached at previously.
>
> I thought this the other way round. Rather than letting the callers
> error out when @{-N} didn't expand to a branch name, I thought we
> should not be expanding @{-N} syntax for "check-ref-format --branch" at
> all to make a "stronger guarantee" that the result is "always" a valid
> branch name. Then I thought it might be too restrictive and didn't
> mention it. So, I dunno.
>
>
>> I am not sure if "check-ref-format --branch" should the same; it is
>> more about the syntax and the 40-hex _is_ valid there, so...
>
> I'm not sure what you were trying to say here, sorry.

The "am not sure if" comes from this question: should these two be equivalent?

    $ git check-ref-format --branch @{-1}
    $ git check-ref-format --branch $(git rev-parse --verify @{-1})

If they should be equivalent (and I think the current implementation says they are), then the answer to "if ... should do the same?" becomes "no, we should not error out".

Stepping back a bit, the mild suspicion above says

    $ git checkout HEAD^0
    ... do things ...
    $ git checkout -b temp
    ... do more things ...
    $ git checkout -B @{-1}

that creates a new branch whose name is 40-hex of a commit that happens to be where we started the whole dance *is* a bug. No sane user expects that to happen, and the last step "checkout -B @{-1}" should result in an error instead [*1*].

I was wondering if "git check-ref-format --branch @{-1}", when used in place of "checkout -B @{-1}" in the above sequence, should or should not fail. It really boils down to this question: what @{-1} expands to and what the user wants to do with it.

In the context of getting a revision (i.e. "rev-parse @{-1}") where we are asking what the object name is, the value of the detached HEAD we were on previously is a valid answer we are "expanding @{-1} to". If we were on a concrete branch and @{-1} yields a concrete branch name, then rev-parse would turn that into an object name, and in the end, in either case, the object name is what we wanted to get. So we do not want to error this out.

But a user of "git check-ref-format --branch" is not asking about the object name ("git rev-parse" would have been used otherwise). Which argues for erroring out "check-ref-format --branch @{-1}" if we were not on a branch in the previous state.

And that argues for erroring out "check-ref-format --branch @{-1}" in such a case, i.e. declaring that the first two commands in this message are not equivalent.

[Footnote]

*1* "It should instead detach HEAD at that commit if @{-1} refers to
    a detached HEAD state" is not a good suggestion (we wouldn't
    have "-B" if a mere branch switching is desired).