From: Kaartic Sivaraam Date: Wed, 06 Dec 2017 17:58:33 GMT Subject: Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax Message-ID: <59e24608-cf7e-6654-90a2-95e6dc22dc3d@gmail.com> In-Reply-To: On Sunday 03 December 2017 07:38 AM, Junio C Hamano wrote: > Kaartic Sivaraam 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.