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

[Kaartic Sivaraam <kaartic.sivaraam@gmail.com>]

On Sat, 2017-12-02 at 17:52 -0800, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > > 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.
> > 

OK. Seems I was out of mind in the above reply. I have answered the
question about whether "branch=$(git check-ref-format @{-1}) && git
checkout -B $branch" should fail or not. Sorry, for the confusion in
case you mind it.


> > 
> > > 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})
> 

I could see how,

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

could be equivalent. I'm not sure how the previous two might be
equivalent except in the case when the previous thing checked out was
not a branch.

1. "git check-ref-format --branch @{-1}" returns the previous thing
checked out which might be a branch name or a commit hash

2. "git check-ref-format --branch $(git rev-parse --verify @{-1})"
returns the commit hash of the previous thing (the hash of the tip if
was a branch). IIUC, this thing never returns a branch name.


> 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,

I guess you mean '... "git checkout -B $(git check-ref-format --branch
@{-1}", when used in place of "git checkout -B @{-1}" ...' ?


>  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.
> 

Right.


> 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.
> 

Exactly what I thought.


> 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.
> 

As said before, IIUC, they give equivalent output to stdout only when
the previous thing checked out was not a branch.


-- 
Kaartic