git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] docs: checking out using @{-N} can lead to detached state
@ 2017-11-19 17:54 Kaartic Sivaraam
  2017-11-20  2:09 ` Junio C Hamano
  0 siblings, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-19 17:54 UTC (permalink / raw)
  To: Git mailing list

The commit message of 75d6e552a (Documentation: @{-N} can refer to
a commit, 2014-01-19) clearly specifies how @{-N} can be used to
refer not only to a branch but also to a commit. IOW, @{-N} is a
syntax for the N-th last "checkout" and not the N-th last "branch"
Therefore, in some cases using `git checkout @{-$N}` does lead to a
"detached HEAD" state.

Correct the misleading sentence which states that @{-N} doesn't
detach HEAD.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Documentation/git-checkout.txt | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e108b0f74..238880f10 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -272,9 +272,8 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	commit, your HEAD becomes "detached" and you are no longer on
 	any branch (see below for details).
 +
-As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
-checks out branches (instead of detaching).  You may also specify
-`-` which is synonymous with `"@{-1}"`.
+As a special case, the `@{-N}` syntax checks out the N-th last branch/commit(checkout).
+You may also specify `-` which is synonymous with `@{-1}`.
 +
 As a further special case, you may use `"A...B"` as a shortcut for the
 merge base of `A` and `B` if there is exactly one merge base. You can
-- 
2.15.0.291.g0d8980c5d


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH] docs: checking out using @{-N} can lead to detached state
  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
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-11-20  2:09 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

> The commit message of 75d6e552a (Documentation: @{-N} can refer to
> a commit, 2014-01-19) clearly specifies how @{-N} can be used to
> refer not only to a branch but also to a commit. IOW, @{-N} is a
> syntax for the N-th last "checkout" and not the N-th last "branch"

If you define a new term "checkout" to mean "what was checked out",
yes that is correct.  After re-reading the said commit and the text
it tried to update, I agree with you that it did not go far enough.

After the first paragraph explains what happens during "checkout
<branch>" and goes from the normal case where <branch> is really a
branch name to an arbitrary commit (where "detaching" needs to be
mentioned), a commit before 75d6e552a added mention of @{-N} and
made it appear as if it were a reference to a commit (i.e. not a
branch name) and that was why it said "As a special case" and
mentioned "detaching".  The problem lies in a lot older one,
696acf45 ("checkout: implement "-" abbreviation, add docs and
tests", 2009-01-17).

I agree that a real fix should ensure that @{-N} is merely a
short-hand for what was checked out in the Nth-previous "git
checkout" operation, *and* regardless of which between a branch name
or a commit object name that Nth thing is, the previous rules we
already gave in the first paragraph apply---if the thing checked out
in the Nth-previous "git checkout" was a branch, we checkout the
branch.  If it was a commit, we detach.

So perhaps we should start from dropping that "As a special case".

    You can also use the `"@{-N}"` syntax to refer to the thing the N-th
    last "git checkout" operation checked out; if it was a branch, that
    branch is checked out, and otherwise the HEAD is detached at the
    commit.  You may also specify `-` which is synonymous to `"@{-1}"`.

or something like that.  If we do so, we'd further need to tweak "As
a further special case", as this rewrite makes it clear that "@{-N}"
is not a special case at all (instead it is merely a different way
to spell <branch> or <commit> that is already covered).

Thanks.


> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index e108b0f74..238880f10 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -272,9 +272,8 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
>  	commit, your HEAD becomes "detached" and you are no longer on
>  	any branch (see below for details).
>  +
> -As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
> -checks out branches (instead of detaching).  You may also specify
> -`-` which is synonymous with `"@{-1}"`.
> +As a special case, the `@{-N}` syntax checks out the N-th last branch/commit(checkout).
> +You may also specify `-` which is synonymous with `@{-1}`.
>  +
>  As a further special case, you may use `"A...B"` as a shortcut for the
>  merge base of `A` and `B` if there is exactly one merge base. You can

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH] docs: checking out using @{-N} can lead to detached state
  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
  1 sibling, 0 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-20 15:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

On Monday 20 November 2017 07:39 AM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> After the first paragraph explains what happens during "checkout
> <branch>" and goes from the normal case where <branch> is really a
> branch name to an arbitrary commit (where "detaching" needs to be
> mentioned), a commit before 75d6e552a added mention of @{-N} and
> made it appear as if it were a reference to a commit (i.e. not a
> branch name) and that was why it said "As a special case" and
> mentioned "detaching".  The problem lies in a lot older one,
> 696acf45 ("checkout: implement "-" abbreviation, add docs and
> tests", 2009-01-17).
> 

Thanks for the analysis. Just to be sure, I referred to 75d6e552a just 
to back up my claim, was that intention clear in my log message? Also, 
should I mention the old commit (696acf45) also in the log message?


> 
> So perhaps we should start from dropping that "As a special case".
> 
>      You can also use the `"@{-N}"` syntax to refer to the thing the N-th
>      last "git checkout" operation checked out; if it was a branch, that
>      branch is checked out, and otherwise the HEAD is detached at the
>      commit.  You may also specify `-` which is synonymous to `"@{-1}"`.
> 
> or something like that.  If we do so, we'd further need to tweak "As
> a further special case", as this rewrite makes it clear that "@{-N}"
> is not a special case at all (instead it is merely a different way
> to spell <branch> or <commit> that is already covered).

Good point. I did use your rewritten message but with some modification,

     You can also use the `@{-N}` syntax to refer to the N-th last
     branch/commit checked out using "git checkout" operation. You may
     also specify `-` which is synonymous to `@{-1}`.

I tweaked the first part of the first sentence and dropped the last part 
of it just to avoid redundancy with the paragraph above it. Hope that 
sounds good. If it seems to need some modification, let me know.


Thanks,
Kaartic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state
  2017-11-20  2:09 ` Junio C Hamano
  2017-11-20 15:18   ` Kaartic Sivaraam
@ 2017-11-27 17:28   ` 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:33     ` [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state Junio C Hamano
  1 sibling, 2 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-27 17:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

@{-N} is a syntax for the N-th last "checkout" and not the N-th
last "branch". Therefore, in some cases using `git checkout @{-$N}`
DOES lead to a "detached HEAD" state. This can also be ensured by
the commit message of 75d6e552a (Documentation: @{-N} can refer to
a commit, 2014-01-19) which clearly specifies how @{-N} can be used
to refer not only to a branch but also to a commit.

Correct the misleading sentence which states that @{-N} doesn't
detach HEAD.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Documentation/git-checkout.txt | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
index e108b0f74..d5a57ac90 100644
--- a/Documentation/git-checkout.txt
+++ b/Documentation/git-checkout.txt
@@ -272,11 +272,11 @@ section of linkgit:git-add[1] to learn how to operate the `--patch` mode.
 	commit, your HEAD becomes "detached" and you are no longer on
 	any branch (see below for details).
 +
-As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
-checks out branches (instead of detaching).  You may also specify
-`-` which is synonymous with `"@{-1}"`.
+You can use the `"@{-N}"` syntax to refer to the N-th last
+branch/commit checked out using "git checkout" operation. You may
+also specify `-` which is synonymous to `"@{-1}`.
 +
-As a further special case, you may use `"A...B"` as a shortcut for the
+As a special case, you may use `"A...B"` as a shortcut for the
 merge base of `A` and `B` if there is exactly one merge base. You can
 leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.
 
-- 
2.15.0.345.gf926f18f3


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-11-27 17:28   ` [PATCH v2 1/2] Doc/checkout: " Kaartic Sivaraam
@ 2017-11-27 17:28     ` Kaartic Sivaraam
  2017-11-28  2:40       ` Junio C Hamano
  2017-11-28  2:33     ` [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-27 17:28 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

When the N-th previous thing checked out sytax is used with
'--branch' option of check-ref-format the results might not
always be a valid branch name as @{-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".

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

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
 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
+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.
 
 OPTIONS
 -------
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 --------
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 ------------
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.345.gf926f18f3


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 1/2] Doc/checkout: checking out using @{-N} can lead to detached state
  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:33     ` Junio C Hamano
  1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-11-28  2:33 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

>  	any branch (see below for details).
>  +
> -As a special case, the `"@{-N}"` syntax for the N-th last branch/commit
> -checks out branches (instead of detaching).  You may also specify
> -`-` which is synonymous with `"@{-1}"`.
> +You can use the `"@{-N}"` syntax to refer to the N-th last
> +branch/commit checked out using "git checkout" operation. You may
> +also specify `-` which is synonymous to `"@{-1}`.
>  +
> -As a further special case, you may use `"A...B"` as a shortcut for the
> +As a special case, you may use `"A...B"` as a shortcut for the
>  merge base of `A` and `B` if there is exactly one merge base. You can
>  leave out at most one of `A` and `B`, in which case it defaults to `HEAD`.

Looks sensible.  Will queue.

Thanks.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  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-11-28 16:34         ` [PATCH v3 " Kaartic Sivaraam
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-11-28  2:40 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

> When the N-th previous thing checked out sytax is used with
> '--branch' option of check-ref-format the results might not
> always be a valid branch name

I wonder if you want to rephrase this, because 40-hex object name is
syntactically a valid branch name.  It's (1) cumbersome to type and
(2) may not be what the user expects.

I have a mild suspicion that "git checkout -B @{-1}" would want to
error out instead of creating a valid new branch whose name is
40-hex that happen to be the name of the commit object you were
detached at previously.

I am not sure if "check-ref-format --branch" should the same; it is
more about the syntax and the 40-hex _is_ valid there, so...

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-11-28  2:40       ` Junio C Hamano
@ 2017-11-28 14:43         ` Kaartic Sivaraam
  2017-12-03  1:52           ` Junio C Hamano
  2017-11-28 16:34         ` [PATCH v3 " Kaartic Sivaraam
  1 sibling, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-28 14:43 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

On Tue, 2017-11-28 at 11:40 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > When the N-th previous thing checked out sytax is used with
> > '--branch' option of check-ref-format the results might not
> > always be a valid branch name
> 
> I wonder if you want to rephrase this, because 40-hex object name is
> syntactically a valid branch name.  It's (1) cumbersome to type and
> (2) may not be what the user expects.
> 

You're right. Actually a previous draft of that log message did say
something like,

	Though a commit-hash might be a valid branch name, it isn't
	something that's expected by the users of "check-ref-format".

I removed as I thought it would be unnecessary. It seems I took the
wrong decision. Will fix it. :-)

> I have a mild suspicion that "git checkout -B @{-1}" would want to
> error out instead of creating a valid new branch whose name is
> 40-hex that happen to be the name of the commit object you were
> detached at previously.
> 

I thought this the other way round. Rather than letting the callers
error out when @{-N} didn't expand to a branch name, I thought we
should not be expanding @{-N} syntax for "check-ref-format --branch" at
all to make a "stronger guarantee" that the result is "always" a valid
branch name. Then I thought it might be too restrictive and didn't
mention it. So, I dunno.


> I am not sure if "check-ref-format --branch" should the same; it is
> more about the syntax and the 40-hex _is_ valid there, so...

I'm not sure what you were trying to say here, sorry.


-- 
Kaartic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-11-28  2:40       ` Junio C Hamano
  2017-11-28 14:43         ` Kaartic Sivaraam
@ 2017-11-28 16:34         ` Kaartic Sivaraam
  2017-12-03  2:08           ` Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-11-28 16:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

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
 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
+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.
 
 OPTIONS
 -------
@@ -116,7 +119,7 @@ OPTIONS
 EXAMPLES
 --------
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 ------------
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-11-28 14:43         ` Kaartic Sivaraam
@ 2017-12-03  1:52           ` Junio C Hamano
  2017-12-04 17:25             ` Kaartic Sivaraam
  0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2017-12-03  1:52 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

>> I have a mild suspicion that "git checkout -B @{-1}" would want to
>> error out instead of creating a valid new branch whose name is
>> 40-hex that happen to be the name of the commit object you were
>> detached at previously.
>
> I thought this the other way round. Rather than letting the callers
> error out when @{-N} didn't expand to a branch name, I thought we
> should not be expanding @{-N} syntax for "check-ref-format --branch" at
> all to make a "stronger guarantee" that the result is "always" a valid
> branch name. Then I thought it might be too restrictive and didn't
> mention it. So, I dunno.
>
>
>> I am not sure if "check-ref-format --branch" should the same; it is
>> more about the syntax and the 40-hex _is_ valid there, so...
>
> I'm not sure what you were trying to say here, sorry.

The "am not sure if" comes from this question: should these two be
equivalent?

    $ git check-ref-format --branch @{-1}
    $ git check-ref-format --branch $(git rev-parse --verify @{-1})

If they should be equivalent (and I think the current implementation
says they are), then the answer to "if ... should do the same?"
becomes "no, we should not error out".

Stepping back a bit, the mild suspicion above says

    $ git checkout HEAD^0
    ... do things ...
    $ git checkout -b temp
    ... do more things ...
    $ git checkout -B @{-1}

that creates a new branch whose name is 40-hex of a commit that
happens to be where we started the whole dance *is* a bug.  No sane
user expects that to happen, and the last step "checkout -B @{-1}"
should result in an error instead [*1*].

I was wondering if "git check-ref-format --branch @{-1}", when used
in place of "checkout -B @{-1}" in the above sequence, should or
should not fail.  It really boils down to this question: what @{-1}
expands to and what the user wants to do with it.

In the context of getting a revision (i.e. "rev-parse @{-1}") where
we are asking what the object name is, the value of the detached
HEAD we were on previously is a valid answer we are "expanding @{-1}
to".  If we were on a concrete branch and @{-1} yields a concrete
branch name, then rev-parse would turn that into an object name, and
in the end, in either case, the object name is what we wanted to
get.  So we do not want to error this out.

But a user of "git check-ref-format --branch" is not asking about
the object name ("git rev-parse" would have been used otherwise).
Which argues for erroring out "check-ref-format --branch @{-1}" if
we were not on a branch in the previous state.

And that argues for erroring out "check-ref-format --branch @{-1}"
in such a case, i.e. declaring that the first two commands in this
message are not equivalent.


[Footnote]

*1* "It should instead detach HEAD at that commit if @{-1} refers to
    a detached HEAD state" is not a good suggestion (we wouldn't
    have "-B" if a mere branch switching is desired).
    

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-11-28 16:34         ` [PATCH v3 " Kaartic Sivaraam
@ 2017-12-03  2:08           ` Junio C Hamano
  2017-12-06 17:58             ` Kaartic Sivaraam
  2017-12-14 12:30             ` [PATCH v4 " Kaartic Sivaraam
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-12-03  2:08 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

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

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

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-12-03  1:52           ` Junio C Hamano
@ 2017-12-04 17:25             ` Kaartic Sivaraam
  2017-12-04 18:44               ` Junio C Hamano
  0 siblings, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-04 17:25 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

On Sat, 2017-12-02 at 17:52 -0800, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> > > I have a mild suspicion that "git checkout -B @{-1}" would want to
> > > error out instead of creating a valid new branch whose name is
> > > 40-hex that happen to be the name of the commit object you were
> > > detached at previously.
> > 
> > I thought this the other way round. Rather than letting the callers
> > error out when @{-N} didn't expand to a branch name, I thought we
> > should not be expanding @{-N} syntax for "check-ref-format --branch" at
> > all to make a "stronger guarantee" that the result is "always" a valid
> > branch name. Then I thought it might be too restrictive and didn't
> > mention it. So, I dunno.
> > 

OK. Seems I was out of mind in the above reply. I have answered the
question about whether "branch=$(git check-ref-format @{-1}) && git
checkout -B $branch" should fail or not. Sorry, for the confusion in
case you mind it.


> > 
> > > I am not sure if "check-ref-format --branch" should the same; it is
> > > more about the syntax and the 40-hex _is_ valid there, so...
> > 
> > I'm not sure what you were trying to say here, sorry.
> 
> The "am not sure if" comes from this question: should these two be
> equivalent?
> 
>     $ git check-ref-format --branch @{-1}
>     $ git check-ref-format --branch $(git rev-parse --verify @{-1})
> 

I could see how,

	$ git rev-parse --verify @{-1}
	$ git rev-parse --verify $(git check-ref-format --branch @{-1})

could be equivalent. I'm not sure how the previous two might be
equivalent except in the case when the previous thing checked out was
not a branch.

1. "git check-ref-format --branch @{-1}" returns the previous thing
checked out which might be a branch name or a commit hash

2. "git check-ref-format --branch $(git rev-parse --verify @{-1})"
returns the commit hash of the previous thing (the hash of the tip if
was a branch). IIUC, this thing never returns a branch name.


> If they should be equivalent (and I think the current implementation
> says they are), then the answer to "if ... should do the same?"
> becomes "no, we should not error out".
> 
> Stepping back a bit, the mild suspicion above says
> 
>     $ git checkout HEAD^0
>     ... do things ...
>     $ git checkout -b temp
>     ... do more things ...
>     $ git checkout -B @{-1}
> 
> that creates a new branch whose name is 40-hex of a commit that
> happens to be where we started the whole dance *is* a bug.  No sane
> user expects that to happen, and the last step "checkout -B @{-1}"
> should result in an error instead [*1*].
> 
> I was wondering if "git check-ref-format --branch @{-1}", when used
> in place of "checkout -B @{-1}" in the above sequence,

I guess you mean '... "git checkout -B $(git check-ref-format --branch
@{-1}", when used in place of "git checkout -B @{-1}" ...' ?


>  should or
> should not fail.  It really boils down to this question: what @{-1}
> expands to and what the user wants to do with it.
> 
> In the context of getting a revision (i.e. "rev-parse @{-1}") where
> we are asking what the object name is, the value of the detached
> HEAD we were on previously is a valid answer we are "expanding @{-1}
> to".  If we were on a concrete branch and @{-1} yields a concrete
> branch name, then rev-parse would turn that into an object name, and
> in the end, in either case, the object name is what we wanted to
> get.  So we do not want to error this out.
> 

Right.


> But a user of "git check-ref-format --branch" is not asking about
> the object name ("git rev-parse" would have been used otherwise).
> Which argues for erroring out "check-ref-format --branch @{-1}" if
> we were not on a branch in the previous state.
> 

Exactly what I thought.


> And that argues for erroring out "check-ref-format --branch @{-1}"
> in such a case, i.e. declaring that the first two commands in this
> message are not equivalent.
> 

As said before, IIUC, they give equivalent output to stdout only when
the previous thing checked out was not a branch.


-- 
Kaartic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-12-04 17:25             ` Kaartic Sivaraam
@ 2017-12-04 18:44               ` Junio C Hamano
  2017-12-05  5:20                 ` Kaartic Sivaraam
  0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2017-12-04 18:44 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

>> Stepping back a bit, the mild suspicion above says
>> 
>>     $ git checkout HEAD^0
>>     ... do things ...
>>     $ git checkout -b temp
>>     ... do more things ...
>>     $ git checkout -B @{-1}
>> 
>> that creates a new branch whose name is 40-hex of a commit that
>> happens to be where we started the whole dance *is* a bug.  No sane
>> user expects that to happen, and the last step "checkout -B @{-1}"
>> should result in an error instead [*1*].
>> 
>> I was wondering if "git check-ref-format --branch @{-1}", when used
>> in place of "checkout -B @{-1}" in the above sequence,
>
> I guess you mean '... "git checkout -B $(git check-ref-format --branch
> @{-1}", when used in place of "git checkout -B @{-1}" ...' ?

No you guessed wrong.  I was (and am) wondering if the last step in
the following sequence should fail.

>>     $ git checkout HEAD^0
>>     ... do things ...
>>     $ git checkout -b temp
>>     ... do more things ...
>>     $ git check-ref-format --branch @{-1}

And I am leaning towards saying that it is a bug that it does not
fail; @{-1} is a detached HEAD and not a concrete branch name in
this case, so "check-ref-format --branch" should at least notice
and say that it is a request that may lead to a nonsense next step
(which is to create a branch with that 40-hex name).

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v2 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-12-04 18:44               ` Junio C Hamano
@ 2017-12-05  5:20                 ` Kaartic Sivaraam
  0 siblings, 0 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-05  5:20 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

On Tuesday 05 December 2017 12:14 AM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
>>> Stepping back a bit, the mild suspicion above says
>>>
>>>      $ git checkout HEAD^0
>>>      ... do things ...
>>>      $ git checkout -b temp
>>>      ... do more things ...
>>>      $ git checkout -B @{-1}
>>>
>>> that creates a new branch whose name is 40-hex of a commit that
>>> happens to be where we started the whole dance *is* a bug.  No sane
>>> user expects that to happen, and the last step "checkout -B @{-1}"
>>> should result in an error instead [*1*].
>>>
>>> I was wondering if "git check-ref-format --branch @{-1}", when used
>>> in place of "checkout -B @{-1}" in the above sequence,
>>
>> I guess you mean '... "git checkout -B $(git check-ref-format --branch
>> @{-1}", when used in place of "git checkout -B @{-1}" ...' ?
> 
> No you guessed wrong.  I was (and am) wondering if the last step in
> the following sequence should fail.
>>>      $ git checkout HEAD^0
>>>      ... do things ...
>>>      $ git checkout -b temp
>>>      ... do more things ...
>>>      $ git check-ref-format --branch @{-1}
> 

Ok. Now I get what you say.


> And I am leaning towards saying that it is a bug that it does not
> fail; @{-1} is a detached HEAD and not a concrete branch name in
> this case, 

It seems your thought is similar to the following thought that I 
expressed in [1],

-- 8< --
> 
> I thought this the other way round. Rather than letting the callers
> error out when @{-N} didn't expand to a branch name, I thought we
> should not be expanding @{-N} syntax for "check-ref-format --branch" at
> all to make a "stronger guarantee" that the result is "always" a valid
> branch name. Then I thought it might be too restrictive and didn't
> mention it. So, I dunno.

-- >8 --



> so "check-ref-format --branch" should at least notice
> and say that it is a request that may lead to a nonsense next step
> (which is to create a branch with that 40-hex name).
> 

Makes sense, this should at least be noted in the Documentation. Is that 
what you had in mind too or do you expect 'check-ref-format' to do 
something else too?


[1]: https://public-inbox.org/git/1511880237.10193.5.camel@gmail.com/

---
Kaartic

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v3 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-12-03  2:08           ` Junio C Hamano
@ 2017-12-06 17:58             ` Kaartic Sivaraam
  2017-12-14 12:30             ` [PATCH v4 " Kaartic Sivaraam
  1 sibling, 0 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-06 17:58 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  2017-12-03  2:08           ` Junio C Hamano
  2017-12-06 17:58             ` Kaartic Sivaraam
@ 2017-12-14 12:30             ` Kaartic Sivaraam
  2017-12-14 18:02               ` Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-14 12:30 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---

Changes in v4:

		- updated the commit message
		- made changes suggested by Junio

 Documentation/git-check-ref-format.txt | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index cf0a0b7df..8172a6b9a 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,17 +78,21 @@ 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
-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.
+it can be used as a valid branch name e.g. when creating a new branch
+(but be cautious when using the previous checkout syntax; it may refer
+to a detached HEAD state). The rule `git check-ref-format --branch
+$name` implements 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 checkout syntax''
+`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
+was checked out using "git checkout" operation. 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. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 -------
@@ -116,7 +120,7 @@ OPTIONS
 EXAMPLES
 --------
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 ------------
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  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
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2017-12-14 18:02 UTC (permalink / raw)
  To: Kaartic Sivaraam; +Cc: Git mailing list

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

>  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
> -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.
> +it can be used as a valid branch name e.g. when creating a new branch
> +(but be cautious when using the previous checkout syntax; it may refer
> +to a detached HEAD state). The rule `git check-ref-format --branch
> +$name` implements 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 checkout syntax''
> +`@{-n}`.  For example, `@{-1}` is a way to refer the last thing that
> +was checked out using "git checkout" operation. 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. As an
> +exception note that, the ``previous checkout operation'' might result
> +in a commit object name when the N-th last thing checked out was not
> +a branch.

Looks alright.  

It was made unnecessarily harder to review because it was marked as
2/2, even though this no longer applies on top of the copy of 1/2
that was merged some time ago.  I needed to find that it was rebased
on top of 'master'; it wouldn't have been necessary if this was sent
as a single patch (with comment saying that this used to be 2/2
whose first one already graduated to 'master' under the three-dash
line).

Also re-wrapping the lines only to squeeze in "but be cautious..."
and replace s/branch/checkout/ in a few places did not help to make
it easy to spot what's changed.

This part looked a bit strange.

> +it can be used as a valid branch name e.g. when creating a new branch
> +(but be cautious when using the previous checkout syntax; it may refer
> +to a detached HEAD state). The rule `git check-ref-format --branch

I think "e.g. when creating a new branch" is a parenthetical remark,
so it should be inside parenthesis.  As the last three lines in the
new text (quoted above) already warns that it may not be a branch name,
I am not sure if the "but be cautious" adds much value, though.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH v4 2/2] Doc/check-ref-format: clarify information about @{-N} syntax
  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
  1 sibling, 0 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-16  5:38 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

On Thursday 14 December 2017 11:32 PM, Junio C Hamano wrote:
> Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes:
> 
> Looks alright.
> 
> It was made unnecessarily harder to review because it was marked as
> 2/2, even though this no longer applies on top of the copy of 1/2
> that was merged some time ago.

Sorry about that but I don't remember doing anything that made it not to 
apply on top of 1/2. (I just amended my changes to my topic branch. It 
can be found at [1])


> I needed to find that it was rebased
> on top of 'master';

I don't remember doing any rebase on top of 'master'. My topic was (and 
still is) based on the 'master' when it was pointing at 89ea799ff (Sync 
with maint, 2017-11-15). Anyways, it's my mistake as I didn't specify 
the branch on which I based this. Sorry about that.


> 
> Also re-wrapping the lines only to squeeze in "but be cautious..."
> and replace s/branch/checkout/ in a few places did not help to make
> it easy to spot what's changed.
> 

I expected this would happen but I thought the line shouldn't grew too 
much so that they have to re-wrapped. Seems it would have been better if 
I did the re-wrapping as a follow-up commit (didn't strike me then).


> This part looked a bit strange.
> 
>> +it can be used as a valid branch name e.g. when creating a new branch
>> +(but be cautious when using the previous checkout syntax; it may refer
>> +to a detached HEAD state). The rule `git check-ref-format --branch
> 
> I think "e.g. when creating a new branch" is a parenthetical remark,
> so it should be inside parenthesis.

It was. I brought them out to introduce the parenthetical warning. I'll 
send a v5 by putting the remark back inside parantheses and bringing the 
warning out. If it's not ok, let me know. I'll also try to do the 
re-wrapping as a separate cleanup patch.


>  As the last three lines in the
> new text (quoted above) already warns that it may not be a branch name,
> I am not sure if the "but be cautious" adds much value, though.
> 

That warning was for the impatient readers, who might want to find quick 
answers as to why they saw an odd behaviour (check-ref-froamt --branch 
not failing for a commit object name) (or) those who would want to use 
'check-ref-format --branch' but do not find time to read the whole 
paragraph.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v5 0/1] clarify about @{-N} syntax in check-ref-format documentation
  2017-12-14 18:02               ` Junio C Hamano
  2017-12-16  5:38                 ` Kaartic Sivaraam
@ 2017-12-16  8:13                 ` Kaartic Sivaraam
  2017-12-16  8:13                   ` [PATCH v5 1/1] Doc/check-ref-format: clarify information about @{-N} syntax Kaartic Sivaraam
  1 sibling, 1 reply; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-16  8:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

This was previously [PATCH v4 2/2] of this thread. It has now been
detached from 1/2 as it got merged to 'master'.

This patch applies on top of 'master'.

As this is almost a v2 of the v4 2/2 I'm comparing the changes with
v3 2/2 of the series. Apart from the changes shown by the below interdiff
the commit message got changed,

--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -78,20 +78,22 @@ 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
-(except for one exception related to the previous checkout syntax
-noted below). 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). But be cautious when using the
+previous checkout syntax that may refer to a detached HEAD state.
+The rule `git check-ref-format --branch $name` implements
 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 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
+was checked out using "git checkout" operation. 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. 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.
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 -------



Kaartic Sivaraam (1):
  Doc/check-ref-format: clarify information about @{-N} syntax

 Documentation/git-check-ref-format.txt | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

-- 
2.15.0.531.g2ccb3012c


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH v5 1/1] Doc/check-ref-format: clarify information about @{-N} syntax
  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                   ` Kaartic Sivaraam
  0 siblings, 0 replies; 20+ messages in thread
From: Kaartic Sivaraam @ 2017-12-16  8:13 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git mailing list

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.

Signed-off-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
---
 Documentation/git-check-ref-format.txt | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
index cf0a0b7df..d9de99258 100644
--- a/Documentation/git-check-ref-format.txt
+++ b/Documentation/git-check-ref-format.txt
@@ -79,16 +79,21 @@ reference name expressions (see linkgit:gitrevisions[7]):
 
 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
+branch). But be cautious when using the
+previous checkout syntax that may refer to a detached HEAD state.
+The rule `git check-ref-format --branch $name` implements
 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 checked out using "git checkout" operation. 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. As an
+exception note that, the ``previous checkout operation'' might result
+in a commit object name when the N-th last thing checked out was not
+a branch.
 
 OPTIONS
 -------
@@ -116,7 +121,7 @@ OPTIONS
 EXAMPLES
 --------
 
-* Print the name of the previous branch:
+* Print the name of the previous thing checked out:
 +
 ------------
 $ git check-ref-format --branch @{-1}
-- 
2.15.0.531.g2ccb3012c


^ permalink raw reply related	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2017-12-16  8:14 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

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