git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Git mailing list <git@vger.kernel.org>
Subject: Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
Date: Wed, 6 Dec 2017 23:28:33 +0530	[thread overview]
Message-ID: <59e24608-cf7e-6654-90a2-95e6dc22dc3d@gmail.com> (raw)
In-Reply-To: <xmqq609olg1p.fsf@gitster.mtv.corp.google.com>

On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote:
> 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.


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


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


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


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


>> +`@{-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.


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

  reply	other threads:[~2017-12-06 17:58 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-11-19 17:54 [PATCH] docs: checking out using @{-N} can lead to detached state Kaartic Sivaraam
2017-11-20  2:09 ` Junio C Hamano
2017-11-20 15:18   ` Kaartic Sivaraam
2017-11-27 17:28   ` [PATCH v2 1/2] Doc/checkout: " Kaartic Sivaraam
2017-11-27 17:28     ` [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax Kaartic Sivaraam
2017-11-28  2:40       ` Junio C Hamano
2017-11-28 14:43         ` Kaartic Sivaraam
2017-12-03  1:52           ` Junio C Hamano
2017-12-04 17:25             ` Kaartic Sivaraam
2017-12-04 18:44               ` Junio C Hamano
2017-12-05  5:20                 ` Kaartic Sivaraam
2017-11-28 16:34         ` [PATCH v3 " Kaartic Sivaraam
2017-12-03  2:08           ` Junio C Hamano
2017-12-06 17:58             ` Kaartic Sivaraam [this message]
2017-12-14 12:30             ` [PATCH v4 " Kaartic Sivaraam
2017-12-14 18:02               ` Junio C Hamano
2017-12-16  5:38                 ` Kaartic Sivaraam
2017-12-16  8:13                 ` [PATCH v5 0/1] clarify about @{-N} syntax in check-ref-format documentation Kaartic Sivaraam
2017-12-16  8:13                   ` [PATCH v5 1/1] Doc/check-ref-format: clarify information about @{-N} syntax Kaartic Sivaraam
2017-11-28  2:33     ` [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=59e24608-cf7e-6654-90a2-95e6dc22dc3d@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).