On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote:
Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> 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.
