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

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Dec 6, 2017, 17:58 UTC
Message-ID: <59e24608-cf7e-6654-90a2-95e6dc22dc3d@gmail.com>
In-Reply-To: <xmqq609olg1p.fsf@gitster.mtv.corp.google.com>

On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote:

Show 11 quoted lines

> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>>
>> NOTE: Though a commit-hash is a "syntactically" valid branch name,
>> it is generally not considered as one for the use cases of
>> "git check-ref-format --branch". That's because a user who does
>> "git check-ref-format --branch @{-$N}" would except the output
>> to be a "existing" branch name apart from it being syntactically
>> valid.
> 
> s/except/expect/ I suspect.

Correct suspicion.

Show 6 quoted lines

>  But I do not think this description is
> correct.  "check-ref-format --branch @{-1}", when you come from the
> detached HEAD state, happily report success so it does not hold that
> it is "generally not considered".
> 
> Unless you are saying "check-ref-format --branch" is buggy, that is.

I was thinking it was "buggy" from v1 of this patch. The `--branch` option is expected to be used by porcelains but they are also wanted to be aware that the output might NOT be a branch name when the @{-N} syntax is used[1]. This sounds unintuitive given the name of the option(`--branch`). No user would expect anything but a branch name from such an option, I guess. At least, I don't. So, I thought clarifying the Doc was a good "first step" (the Doc guaranteed more than it should).

> If so, we shouldn't be casting that buggy behaviour in stone by
> documenting, but should be fixing it.
>

Yes. I wasn't sure how to go about fixing it and thus took the easier way of updating the Doc. I also think it would be a good way to trigger discussion. Now that the attention has come, any ideas about how it could be FIXED? Should we drop support for @{-N} option in check-branch-ref (highly backward incompatible)? Should we check if @{-$N} is a branch name and fail if it's not(not such a bad thing, I guess)?

> And the paragraph that leads to this NOTE and this NOTE itself are
> both misleading from that point of view.  The result *is* always a
> valid branch name,

I wasn't referring to "syntactic validity" when I mentioned "valid" in the commit message. Though, I understand how I wasn't clear by not disambiguating.

Show 9 quoted lines

> 
> Taking the above together, perhaps.
> 
>      When the N-th previous thing checked out syntax (@{-N}) is used
>      with '--branch' option of check-ref-format the result may not be
>      the name of a branch that currently exists or ever existed.
>      This is because @{-N} is used to refer to the N-th last checked
>      out "thing", which might be any commit (sometimes a branch) in
>      the detached HEAD state.

I don't get the "... any in the detached HEAD state ..." part. I seem to interpret it as "@{-N}" always returns commits from the detached HEAD state. How about,

     When the N-th previous thing checked out syntax (@{-N}) is used
     with '--branch' option of check-ref-format the result may not be
     the name of a branch that currently exists or ever existed. This
     is because @{-N} is used to refer to the N-th last checked out
     "thing", which might be a commit object name if the previous check
     out was a detached HEAD state; or a branch name, otherwise. The
     documentation thus does a wrong thing by promoting it as the
     "previous branch syntax".

Show 5 quoted lines

> 
>      State that @{-N} is the syntax for specifying "N-th last thing
>      checked out" and also state that the result of using @{-N} might
>      also result in an commit object name.
>

This one's nice.

Show 19 quoted lines

>> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
>> index cf0a0b7df..5ddb562d0 100644
>> --- a/Documentation/git-check-ref-format.txt
>> +++ b/Documentation/git-check-ref-format.txt
>> @@ -78,17 +78,20 @@ reference name expressions (see linkgit:gitrevisions[7]):
>>   . at-open-brace `@{` is used as a notation to access a reflog entry.
>>   
>>   With the `--branch` option, the command takes a name and checks if
>> -it can be used as a valid branch name (e.g. when creating a new
>> -branch).  The rule `git check-ref-format --branch $name` implements
>> +it can be used as a valid branch name e.g. when creating a new branch
>> +(except for one exception related to the previous checkout syntax
>> +noted below). The rule `git check-ref-format --branch $name` implements
> 
> And "except for" is also wrong here.  40-hex still *IS* a valid
> branch name; it is just it may not be what we expect.  So perhaps
> rephrase it to something like "(but be cautious when using the
> previous checkout syntax that may refer to a detached HEAD state)".
>

Nice suggestion.

Show 5 quoted lines

>> +`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
>> +was checkout using "git checkout" operation. This option should be
> 
> s/was checkout/was checked out/;
>

Good catch.

Show 7 quoted lines

>> +used by porcelains to accept this syntax anywhere a branch name is
>> +expected, so they can act as if you typed the branch name. As an
>> +exception note that, the ``previous checkout operation'' might result
>> +in a commit hash when the N-th last thing checked out was not a branch.
> 
> s/a commit hash/a commit object name/;
>

Ok.

[1]: Though the users are not currently warned about the weird behaviour when they use the @{-N} syntax, they would be expected to check for commit object name at least after this patch get in. We warn them.