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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2017, 02:08 UTC
Message-ID: <xmqq609olg1p.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171128163406.15452-1-kaartic.sivaraam@gmail.com>

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

Show 17 quoted lines

> When the N-th previous thing checked out syntax (@{-N}) is used
> with '--branch' option of check-ref-format the result might not
> always be a valid branch name (see NOTE below). This is because
> @{-N} is used to refer to the N-th last checked out "thing" which
> might be any commit (sometimes a branch). The documentation thus
> does a wrong thing by promoting it as the "previous branch syntax".
>
> So, correctly state @{-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 a "commit hash".
>
> 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. 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. If so, we shouldn't be casting that buggy behaviour in stone by documenting, but should be fixing it.

But because this patch is about documenting, the farthest you can go is to say that "check-ref-format --branch only checks if the name is syntactically valid, and if you came from a detached HEAD, or if you came from a branch that you deleted since then, the command will say 'yes, that looks like a good branch name to use'. That may not match what you expect, but that is the way it is. Get used to it and that is why we document that behaviour here."

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, but it may not name a branch that currently exists or ever existed.

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

Show 13 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)".

Show 12 quoted lines

>  may be stricter than what `git check-ref-format refs/heads/$name`
>  says (e.g. a dash may appear at the beginning of a ref component,
>  but it is explicitly forbidden at the beginning of a branch name).
>  When run with `--branch` option in a repository, the input is first
> -expanded for the ``previous branch syntax''
> -`@{-n}`.  For example, `@{-1}` is a way to refer the last branch you
> -were on.  This option should be used by porcelains to accept this
> -syntax anywhere a branch name is expected, so they can act as if you
> -typed the branch name.
> +expanded for the ``previous checkout syntax''
> +`@{-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/;

> +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/;