[PATCH 0/6] Fix git checkout - with rebase

[PATCH 0/6] Fix git checkout - with rebase

[Ramkumar Ramachandra]

Hi,

I'm happy to report that I have found a reasonable solution
to the problem: see [5/6].

The larger problem still persists: in my opinion, b397ea4 takes the
wrong approach to the problem it is attempting to solve; nobody cares
_how_ I got to a detached HEAD state; what is important is that I'm
stuck in such a state and need useful information.

The correct approach, in my opinion, is already taken by my prompt:
use git describe.

  artagnon|checkout-dash=$ git checkout @~1
  artagnon|(checkout-dash~1):~/src/git$

Now compare this with the approach taken by the patch:

  artagnon|(checkout-dash~1):~/src/git$ git status
  # HEAD detached at 7aa7992
  nothing added to commit but untracked files present

Completely useless.

Unfortunately, it is too late to revert b397ea4, as too much stuff
already depends on it now (see builtin/branch.c for example).
Reworking the code to use describe is not an easy task at all:
describe has no exposed API, and is polluted with die() statements.
Nevertheless, it can be a fruitful exercise for someone who is willing
to take on the challenge.

Thanks.

Ramkumar Ramachandra (6):
  t/checkout-last: checkout - doesn't work after rebase
  rebase: prepare to write reflog message for checkout
  rebase -i: prepare to write reflog message for checkout
  wt-status: remove unused field in grab_1st_switch_cbdata
  status: do not depend on flaky reflog messages
  checkout: respect GIT_REFLOG_ACTION

 builtin/checkout.c         | 11 ++++++++---
 git-rebase--interactive.sh |  2 ++
 git-rebase.sh              |  2 ++
 t/t2012-checkout-last.sh   | 16 ++++++++++++++++
 t/t7512-status-help.sh     | 37 +++++++++++++++++--------------------
 wt-status.c                | 13 ++++---------
 6 files changed, 49 insertions(+), 32 deletions(-)

-- 
1.8.3.1.384.g7cec0b4

[PATCH 1/6] t/checkout-last: checkout - doesn't work after rebase

[Ramkumar Ramachandra]

The following command

  $ git checkout -

does not work as expected after a rebase.  Every kind of rebase must
behave in the exactly same way: for the purposes of checkout -, the
rebase event should be inconsequential.

Add two failing tests documenting this bug: one for a normal rebase, and
another for an interactive rebase.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t2012-checkout-last.sh | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index b44de9d..ae6d319 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -116,4 +116,20 @@ test_expect_success 'master...' '
 	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)"
 '
 
+test_expect_failure '"checkout -" works after a rebase' '
+	git checkout master &&
+	git checkout other &&
+	git rebase master &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
+test_expect_failure '"checkout -" works after an interactive rebase' '
+	git checkout master &&
+	git checkout other &&
+	git rebase -i master &&
+	git checkout - &&
+	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
+'
+
 test_done
-- 
1.8.3.1.384.g7cec0b4

Re: [PATCH 1/6] t/checkout-last: checkout - doesn't work after rebase

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The following command
>
>   $ git checkout -
>
> does not work as expected after a rebase.  Every kind of rebase must
> behave in the exactly same way: for the purposes of checkout -, the
> rebase event should be inconsequential.
>
> Add two failing tests documenting this bug: one for a normal rebase, and
> another for an interactive rebase.

Why two?

After the discussion, I would have expected to see the two argument
form:

	git rebase [-i] master other
        
started on the 'other' branch and also started on a branch that is
not 'master' or 'other', also be tested to specify the desired
behaviour in these cases.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t2012-checkout-last.sh | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
> index b44de9d..ae6d319 100755
> --- a/t/t2012-checkout-last.sh
> +++ b/t/t2012-checkout-last.sh
> @@ -116,4 +116,20 @@ test_expect_success 'master...' '
>  	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)"
>  '
>  
> +test_expect_failure '"checkout -" works after a rebase' '
> +	git checkout master &&
> +	git checkout other &&
> +	git rebase master &&
> +	git checkout - &&
> +	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
> +'
> +
> +test_expect_failure '"checkout -" works after an interactive rebase' '
> +	git checkout master &&
> +	git checkout other &&
> +	git rebase -i master &&
> +	git checkout - &&
> +	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
> +'
> +
>  test_done

Re: [PATCH 1/6] t/checkout-last: checkout - doesn't work after rebase

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Why two?

What breaks checkout - is the initial HEAD detachment (which writes
that "checkout: " message), before anything else happens.  None of
<onto>, <upstream>, and <branch> make any difference: I'm testing
exactly the code that I patched.

I have recently been told that I should be testing "end-user behavior"
by treating the programs as black-boxes, instead of "implementation".
What is your opinion on the issue?  Should I write more tests?

Re: [PATCH 1/6] t/checkout-last: checkout - doesn't work after rebase

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Why two?
>
> What breaks checkout - is the initial HEAD detachment (which writes
> that "checkout: " message), before anything else happens.  None of
> <onto>, <upstream>, and <branch> make any difference: I'm testing
> exactly the code that I patched.
>
> I have recently been told that I should be testing "end-user behavior"
> by treating the programs as black-boxes, instead of "implementation".
> What is your opinion on the issue?  Should I write more tests?

Of course, the behaviours that should be observable by end-users
need be spelled out.  Also, an impementation detail that cannot be
observed or make any difference to the end-user experience should
not be cast in stone.

The guideline is a good one, but you need to realize that there is a
difference between "do not test implementation details" and "do not
look at implementation when designing tests".  Only the former is
necessary and correct: it lets you avoid over-specifying the
behaviour.

Sometimes you can trivially tell that some obvious implementations
that may be different from what you have can break the expectation
you are setting.  For example, in v1.5.0, "rebase A B" literally ran
"git-checkout B" as the first thing, and if you want the end-user to
expect that @{-$n} does not resolve to B because such internal
"checkout" is not an user action, covering "rebase A B", even if you
know that the current implemention happens to be immune to such a
form of breakage, would be a good way to future-proof your code.

That still is testing the behaviour.  You are just taking advantage
of the fact that you know the implementation and can anticipate how
future changes by careless people could break the behaviour.

[PATCH 2/6] rebase: prepare to write reflog message for checkout

[Ramkumar Ramachandra]

rebase should never write "checkout: " messages to the reflog, since it
is highly confusing to the end user, and breaks
grab_nth_branch_checkout(), as demonstrated by failing tests
in t/checkout-last.  Set a sensible GIT_REFLOG_ACTION: checkout does not
respect GIT_REFLOG_ACTION yet, but this defect will be addressed in a
future patch.

When the defect is addressed, rebase will write the following line to
the reflog when started:

  rebase: checkout master

This is much better than the confusing message it currently writes:

  checkout: moving from master to 1462b67

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-rebase.sh b/git-rebase.sh
index d0c11a9..6587019 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -568,6 +568,8 @@ test "$type" = interactive && run_specific_rebase
 
 # Detach HEAD and reset the tree
 say "$(gettext "First, rewinding head to replay your work on top of it...")"
+
+GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 git checkout -q "$onto^0" || die "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 
-- 
1.8.3.1.384.g7cec0b4

Re: [PATCH 2/6] rebase: prepare to write reflog message for checkout

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> rebase should never write "checkout: " messages to the reflog, since it
> is highly confusing to the end user, and breaks
> grab_nth_branch_checkout(), as demonstrated by failing tests
> in t/checkout-last.  

"breaks" because "the branch flipping rebase internally does is not
'checkout' as far as the end user is concerned", which essentially
is the tautology with the first part "should never write".

I would say

    The branch flipping rebase internally does is not 'checkout' as
    far as the end user is concerned; rebase should never write
    "checkout: " messages to the reflog.  Instead it should say
    "rebase: checkout master".

> Set a sensible GIT_REFLOG_ACTION: checkout does not
> respect GIT_REFLOG_ACTION yet, but this defect will be addressed in a
> future patch.

That is not a defect; it did not have to so far, until this patch.

I suspect doing 6/6 before this patch may make more sense.

Re: [PATCH 2/6] rebase: prepare to write reflog message for checkout

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]

Will fix those.

> I suspect doing 6/6 before this patch may make more sense.

Yeah, I'd done it like that in the original (early preview thing).
Allow me to explain why I flipped the ordering.

The problem I am facing is that 6/6 causes very major breakages, and
5/6 attempts to minimize that fallout and make life for 6/6 easier.
The problem with putting this patch (and the rebase -i) after those
two is simple: it calls set_reflog_action, but never explicitly
indicates that it wants to set the reflog message for checkout.  As a
result, the reflog messages are merely accidental and will look like:

  rebase
  rebase -i (start)

in both the critical patches (5/6 and 6/6).  This was an absolute
debugging disaster for me, and I didn't know what wt-status was trying
to tell me with its cryptic "detached HEAD to" and "detached HEAD
from" messages.

Makes sense?

[PATCH 3/6] rebase -i: prepare to write reflog message for checkout

[Ramkumar Ramachandra]

Interactive rebase should never write "checkout: " messages to the
reflog, since it is highly confusing to the end user, and breaks
grab_nth_branch_checkout(), as demonstrated by failing tests
in t/checkout-last.  Set a sensible GIT_REFLOG_ACTION: checkout does not
respect GIT_REFLOG_ACTION yet, but this defect will be addressed in a
future patch.

When the defect is addressed, rebase -i will write the following line to
the reflog when started:

  rebase -i (start): checkout master

This is much better than the confusing message it currently writes:

  checkout: moving from master to 1462b67

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-rebase--interactive.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index f953d8d..0f04425 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -838,6 +838,7 @@ comment_for_reflog start
 
 if test ! -z "$switch_to"
 then
+	GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $switch_to"
 	output git checkout "$switch_to" -- ||
 		die "Could not checkout $switch_to"
 fi
@@ -981,6 +982,7 @@ has_action "$todo" ||
 
 test -d "$rewritten" || test -n "$force_rebase" || skip_unnecessary_picks
 
+GIT_REFLOG_ACTION="$GIT_REFLOG_ACTION: checkout $onto_name"
 output git checkout $onto || die_abort "could not detach HEAD"
 git update-ref ORIG_HEAD $orig_head
 do_rest
-- 
1.8.3.1.384.g7cec0b4

Re: [PATCH 3/6] rebase -i: prepare to write reflog message for checkout

[Junio C Hamano]

The same comment as 2/6 applies to this one.  What these two do
makes sense, but I think having 6/6 before them would make the
series easier to follow.

[PATCH 4/6] wt-status: remove unused field in grab_1st_switch_cbdata

[Ramkumar Ramachandra]

The struct grab_1st_switch_cbdata has the field "found", which is set in
grab_1st_switch() when a match is found.  This information is redundant
and unused by any caller: the return value of the function serves to
communicate this information anyway.  Remove the field.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 wt-status.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index bf84a86..2511270 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1035,7 +1035,6 @@ got_nothing:
 }
 
 struct grab_1st_switch_cbdata {
-	int found;
 	struct strbuf buf;
 	unsigned char nsha1[20];
 };
@@ -1059,7 +1058,6 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
 	for (end = target; *end && *end != '\n'; end++)
 		;
 	strbuf_add(&cb->buf, target, end - target);
-	cb->found = 1;
 	return 1;
 }
 
-- 
1.8.3.1.384.g7cec0b4

Re: [PATCH 4/6] wt-status: remove unused field in grab_1st_switch_cbdata

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The struct grab_1st_switch_cbdata has the field "found", which is set in
> grab_1st_switch() when a match is found.  This information is redundant
> and unused by any caller: the return value of the function serves to
> communicate this information anyway.  Remove the field.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---

Good.

>  wt-status.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index bf84a86..2511270 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1035,7 +1035,6 @@ got_nothing:
>  }
>  
>  struct grab_1st_switch_cbdata {
> -	int found;
>  	struct strbuf buf;
>  	unsigned char nsha1[20];
>  };
> @@ -1059,7 +1058,6 @@ static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,
>  	for (end = target; *end && *end != '\n'; end++)
>  		;
>  	strbuf_add(&cb->buf, target, end - target);
> -	cb->found = 1;
>  	return 1;
>  }

[PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

b397ea4 (status: show more info than "currently not on any branch",
2013-03-13) made the output of 'git status' richer in the case of a
detached HEAD.  Before this patch, with a detached HEAD:

  $ git status
  # Not currently on any branch.

After the patch:

  $ git checkout v1.8.2
  # HEAD is now detached
  $ git status
  # HEAD detached at v1.8.2.

It works by digging the reflog for the most recent message of the form
"checkout: moving from xxxx to yyyy".  It then asserts that HEAD and
"yyyy" are the same, and displays this message.  When they aren't equal,
it displays:

  $ git status
  # HEAD detached from fe11db.

At this point, the utility of such a message is in question.  Moreover,
there are several tests in t/status-help that explicitly rely on rebase
writing "checkout: " messages to the reflog.  As evidenced by the
failing tests in t/checkout-last, these messages are completely
unintended and flaky.  Relying on them only makes it harder to improve
the reflog messages written by scripts.  As a reasonable compromise,
remove the logic to display the "HEAD detached from ..." message, and
fallback to "Not currently on any branch." in this case.  Update the
tests, giving scripts some breathing space.

This issue is not isolated to rebase at all.  Several other scripts like
bisect write (confusing) "checkout: " messages to the reflog.  Fixing
them is left as an exercise to other contributors.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 t/t7512-status-help.sh | 24 +++++++++++-------------
 wt-status.c            | 11 ++++-------
 2 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index bf08d4e..ed9d57c 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -188,10 +188,9 @@ test_expect_success 'status when rebasing -i in edit mode' '
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~2) &&
-	TGT=$(git rev-parse --short two_rebase_i) &&
 	git rebase -i HEAD~2 &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $TGT
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''rebase_i_edit'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -246,11 +245,10 @@ test_expect_success 'status after editing the last commit with --amend during a
 	export FAKE_LINES &&
 	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD~3) &&
-	TGT=$(git rev-parse --short three_amend) &&
 	git rebase -i HEAD~3 &&
 	git commit --amend -m "foo" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $TGT
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''amend_last'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -280,7 +278,7 @@ test_expect_success 'status: (continue first edit) second edit' '
 	git rebase -i HEAD~3 &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -302,7 +300,7 @@ test_expect_success 'status: (continue first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -329,7 +327,7 @@ test_expect_success 'status: (continue first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "foo" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -351,7 +349,7 @@ test_expect_success 'status: (amend first edit) second edit' '
 	git commit --amend -m "a" &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -374,7 +372,7 @@ test_expect_success 'status: (amend first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -402,7 +400,7 @@ test_expect_success 'status: (amend first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "d" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -426,7 +424,7 @@ test_expect_success 'status: (split first edit) second edit' '
 	git commit -m "e" &&
 	git rebase --continue &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
@@ -451,7 +449,7 @@ test_expect_success 'status: (split first edit) second edit and split' '
 	git rebase --continue &&
 	git reset HEAD^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -481,7 +479,7 @@ test_expect_success 'status: (split first edit) second edit and amend' '
 	git rebase --continue &&
 	git commit --amend -m "h" &&
 	cat >expected <<-EOF &&
-	# HEAD detached from $ONTO
+	# Not currently on any branch.
 	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
 	#   (use "git commit --amend" to amend the current commit)
 	#   (use "git rebase --continue" once you are satisfied with your changes)
diff --git a/wt-status.c b/wt-status.c
index 2511270..f7d46a1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1173,15 +1173,12 @@ void wt_status_print(struct wt_status *s)
 		if (!prefixcmp(branch_name, "refs/heads/"))
 			branch_name += 11;
 		else if (!strcmp(branch_name, "HEAD")) {
+			unsigned char sha1[20];
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.detached_from) {
-				unsigned char sha1[20];
+			if (state.detached_from && !get_sha1("HEAD", sha1) &&
+				!hashcmp(sha1, state.detached_sha1)) {
 				branch_name = state.detached_from;
-				if (!get_sha1("HEAD", sha1) &&
-				    !hashcmp(sha1, state.detached_sha1))
-					on_what = _("HEAD detached at ");
-				else
-					on_what = _("HEAD detached from ");
+				on_what = _("HEAD detached at ");
 			} else {
 				branch_name = "";
 				on_what = _("Not currently on any branch.");
-- 
1.8.3.1.384.g7cec0b4

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> b397ea4 (status: show more info than "currently not on any branch",
> 2013-03-13) made the output of 'git status' richer in the case of a
> detached HEAD.  Before this patch, with a detached HEAD:
>
>   $ git status
>   # Not currently on any branch.
>
> After the patch:
>
>   $ git checkout v1.8.2
>   # HEAD is now detached
>   $ git status
>   # HEAD detached at v1.8.2.
>
> It works by digging the reflog for the most recent message of the form
> "checkout: moving from xxxx to yyyy".  It then asserts that HEAD and
> "yyyy" are the same, and displays this message.  When they aren't equal,
> it displays:
>
>   $ git status
>   # HEAD detached from fe11db.
>
> At this point, the utility of such a message is in question.

You can question, but I am not convinced the answer is an
unambiguous "not useful"

You were at 1.8.2 but no longer are, so in the following sequence:

    $ git checkout v1.8.2
    $ git status
    $ git reset --hard HEAD^
    $ git status
    
the former would say "detached at v1.8.2" while the latter should
*not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
is too subtle a way to express the state, and is confusing, but I
would not be surprised if people find it useful to be able to learn
"v1.8.2" even after you strayed away.

> Moreover,
> there are several tests in t/status-help that explicitly rely on rebase
> writing "checkout: " messages to the reflog.  As evidenced by the
> failing tests in t/checkout-last, these messages are completely
> unintended and flaky.  

The above only helps to convince me that "rebase should not affect
what the last checked-out branch was by letting 'checkout' it
internally calls to write reflog entries for it"  With patches 6, 2,
and 3, I thought you fixed that issue.

So I am not convinced that is a good argument to justify to regress
"HEAD detached from" message to "Not on any branch".

At least, not just yet.

> This issue is not isolated to rebase at all.  Several other scripts like
> bisect write (confusing) "checkout: " messages to the reflog.  Fixing
> them is left as an exercise to other contributors.

Any scripted Porcelain that use "checkout", with patch 6, should be
able to do so with reflog-action environment variable, right?  And 2
and 3 are examples of such fixes to two of them, which argues even
more strongly that 6 should be earier in the series, I think.

>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t7512-status-help.sh | 24 +++++++++++-------------
>  wt-status.c            | 11 ++++-------
>  2 files changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index bf08d4e..ed9d57c 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -188,10 +188,9 @@ test_expect_success 'status when rebasing -i in edit mode' '
>  	export FAKE_LINES &&
>  	test_when_finished "git rebase --abort" &&
>  	ONTO=$(git rev-parse --short HEAD~2) &&
> -	TGT=$(git rev-parse --short two_rebase_i) &&
>  	git rebase -i HEAD~2 &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $TGT
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''rebase_i_edit'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -246,11 +245,10 @@ test_expect_success 'status after editing the last commit with --amend during a
>  	export FAKE_LINES &&
>  	test_when_finished "git rebase --abort" &&
>  	ONTO=$(git rev-parse --short HEAD~3) &&
> -	TGT=$(git rev-parse --short three_amend) &&
>  	git rebase -i HEAD~3 &&
>  	git commit --amend -m "foo" &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $TGT
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''amend_last'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -280,7 +278,7 @@ test_expect_success 'status: (continue first edit) second edit' '
>  	git rebase -i HEAD~3 &&
>  	git rebase --continue &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -302,7 +300,7 @@ test_expect_success 'status: (continue first edit) second edit and split' '
>  	git rebase --continue &&
>  	git reset HEAD^ &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (Once your working directory is clean, run "git rebase --continue")
>  	#
> @@ -329,7 +327,7 @@ test_expect_success 'status: (continue first edit) second edit and amend' '
>  	git rebase --continue &&
>  	git commit --amend -m "foo" &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -351,7 +349,7 @@ test_expect_success 'status: (amend first edit) second edit' '
>  	git commit --amend -m "a" &&
>  	git rebase --continue &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -374,7 +372,7 @@ test_expect_success 'status: (amend first edit) second edit and split' '
>  	git rebase --continue &&
>  	git reset HEAD^ &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (Once your working directory is clean, run "git rebase --continue")
>  	#
> @@ -402,7 +400,7 @@ test_expect_success 'status: (amend first edit) second edit and amend' '
>  	git rebase --continue &&
>  	git commit --amend -m "d" &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -426,7 +424,7 @@ test_expect_success 'status: (split first edit) second edit' '
>  	git commit -m "e" &&
>  	git rebase --continue &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> @@ -451,7 +449,7 @@ test_expect_success 'status: (split first edit) second edit and split' '
>  	git rebase --continue &&
>  	git reset HEAD^ &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently splitting a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (Once your working directory is clean, run "git rebase --continue")
>  	#
> @@ -481,7 +479,7 @@ test_expect_success 'status: (split first edit) second edit and amend' '
>  	git rebase --continue &&
>  	git commit --amend -m "h" &&
>  	cat >expected <<-EOF &&
> -	# HEAD detached from $ONTO
> +	# Not currently on any branch.
>  	# You are currently editing a commit while rebasing branch '\''several_edits'\'' on '\''$ONTO'\''.
>  	#   (use "git commit --amend" to amend the current commit)
>  	#   (use "git rebase --continue" once you are satisfied with your changes)
> diff --git a/wt-status.c b/wt-status.c
> index 2511270..f7d46a1 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1173,15 +1173,12 @@ void wt_status_print(struct wt_status *s)
>  		if (!prefixcmp(branch_name, "refs/heads/"))
>  			branch_name += 11;
>  		else if (!strcmp(branch_name, "HEAD")) {
> +			unsigned char sha1[20];
>  			branch_status_color = color(WT_STATUS_NOBRANCH, s);
> -			if (state.detached_from) {
> -				unsigned char sha1[20];
> +			if (state.detached_from && !get_sha1("HEAD", sha1) &&
> +				!hashcmp(sha1, state.detached_sha1)) {
>  				branch_name = state.detached_from;
> -				if (!get_sha1("HEAD", sha1) &&
> -				    !hashcmp(sha1, state.detached_sha1))
> -					on_what = _("HEAD detached at ");
> -				else
> -					on_what = _("HEAD detached from ");
> +				on_what = _("HEAD detached at ");
>  			} else {
>  				branch_name = "";
>  				on_what = _("Not currently on any branch.");

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]

I'd just like to point out quickly that I first attempted to write 6/6
without this patch.  It is absolutely impossible, because the
"detached HEAD from/to" messages no longer make any sense when
checkout starts respecting GIT_REFLOG_ACTION.  At that point, I'm was
just monkeying around the trash-directory running describes to somehow
try and make the expected output equal to the actual output.  There
was no method to the madness, and I was literally losing my mind.

This is _the_ patch that makes this series possible.

If you want to be convinced, please attempt to drop this patch and fix
the tests in 6/6.  You will see what I mean.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> At this point, the utility of such a message is in question.
>
> You can question, but I am not convinced the answer is an
> unambiguous "not useful"

I am not arguing for an unambiguous "not useful".  I am arguing for a
practical compromise: this patch locks things up too tightly, and
makes life hell for contributors who want to improve reflog messages.
To be clear: the problem is not the feature, but rather in the
_implementation_ of the feature.

> You were at 1.8.2 but no longer are, so in the following sequence:
>
>     $ git checkout v1.8.2
>     $ git status
>     $ git reset --hard HEAD^
>     $ git status
>
> the former would say "detached at v1.8.2" while the latter should
> *not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
> is too subtle a way to express the state, and is confusing, but I
> would not be surprised if people find it useful to be able to learn
> "v1.8.2" even after you strayed away.

What is wrong with git describe?  Is this cheaper, or am I missing something?

>> Moreover,
>> there are several tests in t/status-help that explicitly rely on rebase
>> writing "checkout: " messages to the reflog.  As evidenced by the
>> failing tests in t/checkout-last, these messages are completely
>> unintended and flaky.
>
> The above only helps to convince me that "rebase should not affect
> what the last checked-out branch was by letting 'checkout' it
> internally calls to write reflog entries for it"  With patches 6, 2,
> and 3, I thought you fixed that issue.

I also thought of ignoring the first line in the actual output ("HEAD
detached from/to"), and comparing the rest to make the tests pass.  At
that point, you start to wonder: what is this fantastic feature that
we are bending over backwards for?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> To be clear: the problem is not the feature, but rather in the
> _implementation_ of the feature.

OK, but are we discussing the same "feature" (see below)?

>> You were at 1.8.2 but no longer are, so in the following sequence:
>>
>>     $ git checkout v1.8.2
>>     $ git status
>>     $ git reset --hard HEAD^
>>     $ git status
>>
>> the former would say "detached at v1.8.2" while the latter should
>> *not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
>> is too subtle a way to express the state, and is confusing, but I
>> would not be surprised if people find it useful to be able to learn
>> "v1.8.2" even after you strayed away.
>
> What is wrong with git describe?  Is this cheaper, or am I missing something?

I think what you are missing is that the "detached from" is not
about your current HEAD after you flipped it around with many resets
and commits.  It is about what tag or what specific commit you
detached your HEAD at originally.

"You can ask the same to describe" is wrong and is not a valid
argument.  Once you replace the HEAD^ with a distant commit
(e.g. HEAD~400) in the third step in the example, "describe" will
not talk about v1.8.2 at all.

Your argument can be that it is not a useful piece of information,
and as you can probably guess from my "I wouldn't be surprised"
above, I am not sure how useful it would be and in what situation
[*1*].

But the original commit thought it was necessary and that was done
for a reason; we need to be careful here.  With a good justification
why it is not necessary (or misleading to the user), I do not think
we cannot change it.

As various tools that use detached "intermediate" states leave
enough clues for "status" to notice what is going on, I actually
think it is a mistake to focus on what happens when we are in such a
detached "intermediate" status with this codepath.  For example:

	$ git rebase master
        ... replays some but stops
        $ git status

currently uses that "HEAD detached from" codepath, but I think that
is a mistake.  We could not just tell the HEAD is detached, but the
reason _why_ the HEAD is detached (i.e. we are in the middle of a
rebase).  The prompt script can do it, "status" should be able to do
the same, and do a lot more sensible thing than unconditionally
showing that "HEAD detached from" and then say "You are currently
rebasing" on a separate line.  Most likely we do not want to even
say "Not currently on any branch" but just say "You are currently
rebasing branch X on top of Y" (and perhaps "N commits remaining to
be replayed").


[Footnote]

*1* One thing I could think of is to start sightseeing or (more
    realistically) manually bisecting at a given release point,
    reset the detached HEAD around several times, and then want to
    be reminded where the session started from.  I do not think it
    is particularly a very good example, though.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>> What is wrong with git describe?  Is this cheaper, or am I missing something?
>
> I think what you are missing is that the "detached from" is not
> about your current HEAD after you flipped it around with many resets
> and commits.  It is about what tag or what specific commit you
> detached your HEAD at originally.

No, it is about what tag of specific commit you detached your HEAD
from, *without using checkout*.  If you used checkout, you'd get the
"detached at" message, and I haven't changed that.

> "You can ask the same to describe" is wrong and is not a valid
> argument.  Once you replace the HEAD^ with a distant commit
> (e.g. HEAD~400) in the third step in the example, "describe" will
> not talk about v1.8.2 at all.

You're missing the point: how do I, as the end-user, detach my HEAD
*without using checkout*?  The hypothetical example you have given is:

  $ git checkout HEAD^
  $ git update-ref HEAD $(git rev-parse HEAD~400)

Which end-user executes that?

> Your argument can be that it is not a useful piece of information,
> and as you can probably guess from my "I wouldn't be surprised"
> above, I am not sure how useful it would be and in what situation
> [*1*].

Precisely.  It is a poorly thought-out feature that locks things up
too tightly, and makes life hell for contributors.  It must therefore
be removed.

> But the original commit thought it was necessary and that was done
> for a reason; we need to be careful here.  With a good justification
> why it is not necessary (or misleading to the user), I do not think
> we cannot change it.

We cannot reverse-engineer intents.  All we can do is look at the
evidence in front of us.  Read the commit message, and look at the
newly added test.  There is absolutely no indication about why this
"detached from" is useful, and where.

>         $ git rebase master
>         ... replays some but stops
>         $ git status
>
> currently uses that "HEAD detached from" codepath, but I think that
> is a mistake.  We could not just tell the HEAD is detached, but the
> reason _why_ the HEAD is detached (i.e. we are in the middle of a
> rebase).  The prompt script can do it, "status" should be able to do
> the same, and do a lot more sensible thing than unconditionally
> showing that "HEAD detached from" and then say "You are currently
> rebasing" on a separate line.  Most likely we do not want to even
> say "Not currently on any branch" but just say "You are currently
> rebasing branch X on top of Y" (and perhaps "N commits remaining to
> be replayed").

That information is available in .git/{rebase-apply,rebase-merge}, and
your suggestion pertains to improving show_rebase_in_progress().  The
first line is about the state of HEAD, and is completely orthogonal to
the issue at hand.

  artagnon|rebase-rev-failure|REBASE-i 2/3:~/src/git$ git status
  # HEAD detached from a7e9fd4
  # You are currently editing a commit while rebasing branch
'rebase-rev-failure' on '9926f66'.
  #
  nothing to commit, working directory clean

That first line about "HEAD detached from ..." is completely useless.
And yes, my prompt is more useful.  No prizes for guessing how often I
use the long form of git status.

> *1* One thing I could think of is to start sightseeing or (more
>     realistically) manually bisecting at a given release point,
>     reset the detached HEAD around several times, and then want to
>     be reminded where the session started from.  I do not think it
>     is particularly a very good example, though.

The example you have given now is:

  $ git checkout @^
  # or whatever bisect command to detach HEAD
  $ git reset @~3
  ...
  $ git reset @^
  ...
  $ git reset @~5
  ....
  # when was HEAD originally detached?

Yes, it is a contrived example where this feature arguably has some
utility.  Is it worth putting the information in the status for such
an esoteric example?  If one really wants this information, they can
open up the reflog and grep for "checkout: ".

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>>> What is wrong with git describe?  Is this cheaper, or am I missing something?
>>
>> I think what you are missing is that the "detached from" is not
>> about your current HEAD after you flipped it around with many resets
>> and commits.  It is about what tag or what specific commit you
>> detached your HEAD at originally.
>
> No, it is about what tag of specific commit you detached your HEAD
> from, *without using checkout*.  If you used checkout, you'd get the
> "detached at" message, and I haven't changed that.

The part you stripped from your quote looked like this:

>> You were at 1.8.2 but no longer are, so in the following sequence:
>>
>>     $ git checkout v1.8.2
>>     $ git status
>>     $ git reset --hard HEAD^
>>     $ git status
>>
>> the former would say "detached at v1.8.2" while the latter should
>> *not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
>> is too subtle a way to express the state, and is confusing, but I
>> would not be surprised if people find it useful to be able to learn
>> "v1.8.2" even after you strayed away.

And your justification to make the latter "git status" to say "Not
on any branch" instead of "detached from" was "what is wrong with
describe".

The user used "checkout" to detach the HEAD, and the user stayed in
that detached state and jumped around.  Where is this "without using
checkout" coming from?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The part you stripped from your quote looked like this:

Apologies for the lack of clarity.

>>> You were at 1.8.2 but no longer are, so in the following sequence:
>>>
>>>     $ git checkout v1.8.2
>>>     $ git status
>>>     $ git reset --hard HEAD^
>>>     $ git status
>>>
>>> the former would say "detached at v1.8.2" while the latter should
>>> *not*, because we are no longer at v1.8.2.  "detached from v1.8.2"
>>> is too subtle a way to express the state, and is confusing, but I
>>> would not be surprised if people find it useful to be able to learn
>>> "v1.8.2" even after you strayed away.
>
> And your justification to make the latter "git status" to say "Not
> on any branch" instead of "detached from" was "what is wrong with
> describe".

In this example, it is inconsequential whether I run:

  $ git checkout v1.8.2^

or:

  $ git checkout v1.8.2
  $ git reset --hard @^

as far as describe is concerned.  It will give me the same good
consistent answer in either case.

> The user used "checkout" to detach the HEAD, and the user stayed in
> that detached state and jumped around.  Where is this "without using
> checkout" coming from?

The point I was trying to make is:

  $ git checkout v1.8.2
  $ git checkout @^

will give a different result once again.  As the end-user, I use
checkout to inspect detached HEAD states, and reset to update
worktree/refs.

Did my response to your examples make sense?  Are you convinced that
this feature should be removed yet?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> In this example, it is inconsequential whether I run:
>
>   $ git checkout v1.8.2^
>
> or:
>
>   $ git checkout v1.8.2
>   $ git reset --hard @^
>
> as far as describe is concerned.  It will give me the same good
> consistent answer in either case.

Yes, "describe HEAD" is about the location in the history of the
commit at the current HEAD.

But the commit shown on "detached at/detached from" is for people
who care about the differences between the initial "checkout" you
did in these two examples: "checkout v1.8.2^" and "checkout v1.8.2".

That is different from the question you ask to "describe", which is
the location your HEAD happens to be in the commit ancestry graph
after you detached the HEAD with "checkout" and possibly jumping
around.  If you say "Not on any branch" instead of saying "detached
from v1.8.2" (or v1.8.2^), you are losing information, aren't you?

As I said (twice), you can argue that that particular piece of
information is not useful (at least to you), but why it is not
useful has to be justified, against the justification given by
b397ea4863a1 (status: show more info than "currently not on any
branch", 2013-03-13) and people who have been using that information
in the status output, no?

>> The user used "checkout" to detach the HEAD, and the user stayed in
>> that detached state and jumped around.  Where is this "without using
>> checkout" coming from?
>
> The point I was trying to make is:
>
>   $ git checkout v1.8.2
>   $ git checkout @^
>
> will give a different result once again.

The last checkout is for HEAD^ while at v1.8.2 in this case, isn't
it?  Don't you want to show it?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> As I said (twice), you can argue that that particular piece of
> information is not useful (at least to you), but why it is not
> useful has to be justified, against the justification given by
> b397ea4863a1 (status: show more info than "currently not on any
> branch", 2013-03-13) and people who have been using that information
> in the status output, no?

Yes, Junio.  I cannot justify that "detached HEAD from" is completely
useless, because it is not.  I argued for a practical compromise, and
have tried to show the huge negative impact against the marginal gain.
 If you argue that we absolutely must not cause an
information-regression against all odds and are unwilling to
compromise, I cannot improve the reflog messages written by various
scripts (and fix git checkout -).  I don't know any other way forward.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> As I said (twice), you can argue that that particular piece of
>> information is not useful (at least to you), but why it is not
>> useful has to be justified, against the justification given by
>> b397ea4863a1 (status: show more info than "currently not on any
>> branch", 2013-03-13) and people who have been using that information
>> in the status output, no?
>
> Yes, Junio.  I cannot justify that "detached HEAD from" is completely
> useless, because it is not.  I argued for a practical compromise, and
> have tried to show the huge negative impact against the marginal gain.

First of all, during a stopped "git rebase", I think "git status"
that says "# HEAD detached at" or "# HEAD detached from" is not
something we want in the end result anyway.  In either case, that
comes from the internal use of "git checkout" by the command to
switch out of a concrete branch, which is *not* user initiated.

If you first update "git checkout" so that it will pay attention to
a custom reflog-action exported by Porcelain scripts that may want
to internally use it to flip branches (and without a custom one, it
will still record "checkout: moving from A to B"), without exporting
custom reflog-action from "rebase" and other Porcelain scripts, that
would not affect any externally visible behaviour.

When you teach "git rebase" to pass custom reflog-action when it
detaches the HEAD at "$onto^0" (because that is not a user-initiated
action) as the next step, nth_prior_checkout and 1st_switch would
stop counting them as "checkout" events.  That fix will give you the
desired result of the topic of this series, i.e. "git checkout @{-1}"
ignores the internal "git checkout" done by "rebase".

That step may change the output from "git status" but I do not think
the output from "git status" in this sequence:

	git rebase ;# stops upon conflict
        git status

is anything sacred.  Tests modified by b397ea4863a1 (status: show
more info than "currently not on any branch", 2013-03-13) do change
the original "# Not currently on any branch" to "detached at", but I
do not think b397ea4863a1 _wanted_ to say where the $onto commit was
during the rebase (or was it?), so if they have to change to
"detached from", I do not think it is a problem at all.  I think it
is very sane to update these selected "detached at" to "detached
from" as needed.

That would be a sensible compromise, because we do not want to see
either "detached at" or "detached from" during "rebase" in the
longer term.  We would rather want to see the message start with
something like

	# Rebasing X on top of Y (23 commits remaining to be replayed)

which is under discussion in a separate thread lately.  The change
to expected output (i.e. "detached at" to "detached from") will have
to be changed again in that separate topic.

The very last test that the b397ea4863a1 added, which wants to see
"detached at atag", should be kept, and also there should be another
test added to reset one step back to make sure we see "detached
from" after it.  It tests what the user sees after a "git checkout"
initiated by the end-user.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> If you first update "git checkout" so that it will pay attention to
> a custom reflog-action exported by Porcelain scripts that may want
> to internally use it to flip branches (and without a custom one, it
> will still record "checkout: moving from A to B"), without exporting
> custom reflog-action from "rebase" and other Porcelain scripts, that
> would not affect any externally visible behaviour.

rebase already sets a custom reflog-action; in fact,
'set_reflog_action rebase' is executed after it sources sh-setup.  I
have to go out of my way to deliberately unset it before every
checkout invocation to avoid breaking all the tests in t/status-help.
Most unnatural way to go about writing a series, if you ask me.

> Tests modified by b397ea4863a1 (status: show
> more info than "currently not on any branch", 2013-03-13) do change
> the original "# Not currently on any branch" to "detached at", but I
> do not think b397ea4863a1 _wanted_ to say where the $onto commit was
> during the rebase (or was it?), so if they have to change to
> "detached from", I do not think it is a problem at all.  I think it
> is very sane to update these selected "detached at" to "detached
> from" as needed.

... and you have missed the point yet again.

I previously wrote:
> I'd just like to point out quickly that I first attempted to write 6/6
> without this patch.  It is absolutely impossible, because the
> "detached HEAD from/to" messages no longer make any sense when
> checkout starts respecting GIT_REFLOG_ACTION.  At that point, I'm was
> just monkeying around the trash-directory running describes to somehow
> try and make the expected output equal to the actual output.  There
> was no method to the madness, and I was literally losing my mind.

Let me try and explain this once again: to determine "HEAD detached
from/to", wt-status must look for a previous "checkout: " message in
the reflog.  Since a rebase no longer executes a checkout, the
"detached HEAD from" message no longer has anything to do with the
rebase itself; it is completely _random_ and _incidental_, based on
what checkout operation was executed last (eg. in a previous test).
To change the "detached HEAD to" messages to "detached HEAD from"
messages, I have to turn myself into a monkey that goes around running
describes on the test-directory.  After enduring hours of this
mind-numbing job, the final result will be a terrible mess; tests can
no longer be added, removed, reordered, or even tweaked.  Everything
depends on the exact ordering of the tests, and the incidental
checkout invocations they contain.

> That would be a sensible compromise

How is it a compromise?  It is most uncompromising.

> because we do not want to see
> either "detached at" or "detached from" during "rebase" in the
> longer term.  We would rather want to see the message start with
> something like
>
>         # Rebasing X on top of Y (23 commits remaining to be replayed)
>
> which is under discussion in a separate thread lately.  The change
> to expected output (i.e. "detached at" to "detached from") will have
> to be changed again in that separate topic.

Orthogonal.  I want checkout-dash now, and I should not have to be
asked to do something as unrelated as redoing 'git status' output to
get what I want.

Since you have made it clear that you will defend every bit of b397ea
against all odds, and since I want checkout-dash now, we have to do
something.  I have a proposal: I will smudge the first line of the
'git status' output, before comparing it to make tests pass.  Never
mind that the line is incidental and Wrong; since I have no say in the
matter, I will just sidestep the issue.  I don't use the long-form
'git status' anyway, and am never going to see this ugliness.  We both
win.  Okay?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> If you first update "git checkout" so that it will pay attention to
>> a custom reflog-action exported by Porcelain scripts that may want
>> to internally use it to flip branches (and without a custom one, it
>> will still record "checkout: moving from A to B"), without exporting
>> custom reflog-action from "rebase" and other Porcelain scripts, that
>> would not affect any externally visible behaviour.
>
> rebase already sets a custom reflog-action; in fact,
> 'set_reflog_action rebase' is executed after it sources sh-setup.

Ah, OK, that is what I missed, so updating the "checkout" itself
will affect the test Duy's commit updated.

But that does not change anything.  The thing is, I fully agree with
this:

> the "detached HEAD from" message no longer has anything to do with
> the rebase itself; it is completely _random_ and _incidental_.

because at the end of this series, we will not be recording the
internal "checkout" as a checkout event in the reflog.
What the top-line of "git status" says does not matter.

If I were doing this series, the first commit before doing anything
else would be to strip that line from the expected output from "git
status" for tests Duy's commit changed, with the justification
perhaps like this:

    b397ea4863a1 (status: show more info than "currently not on any
    branch", 2013-03-13) wanted to make sure that after a checkout
    to detach HEAD, the user can see where the HEAD was originally
    detached from.

    The last test added by that commit to t7512 shows one example,
    immediately after HEAD is detached.  Add another test to show
    "detached HEAD from" form that should be shown when the user
    further resetted to another commit.

    The majority of tests in t7512 that commit updated, however, are
    looking at the status output while in "rebase", and the exact
    content of that first line relies on the fact that "git rebase"
    used "git checkout" to first detach HEAD before doing anything
    else, and allowed that to be recorded in the reflog just like a
    user initiated "checkout".  As far as the user is concerned,
    however, this operation is not a "checkout" that the user would
    want to count when later issuing "git checkout @{-1}", etc.

    We are going to fix "git rebase" not to let reflog record these
    internal events as "checkout"; these first lines will start
    saying different (and useless) things.  Improving them to give
    a message more appropriate during "rebase" (e.g. "# Rebasing
    branch X on top of Y", instead of "# Not currently on any
    branch" or "# HEAD detached from Z") is left to a separate
    future series, but until that happens, stop checking what
    appears on that first line.

and would keep the last one that shows "detached HEAD at".  That is
what that commit wantd to achieve.  This first commit in the series
would be a good place to add the missing one after that to show
"detached HEAD from", doing "reset HEAD^".  In other words, we can
make this tirst step as a pure clean-up.

If stripping the first line to compare is too much, then I think it
is an acceptable compromise to update the "# HEAD detached from
$ONTO" Duy's patch changed from "# Not currently on ..." to whatever
your updated "rebase" happens to produce, with the same (but weaker)
justification as the last sentence in my "perhaps like this" above.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Since you have made it clear that you will defend every bit of b397ea

You are misreading me.  I am not defending every bit at all.

We both agree that what b397ea4863a1 (status: show more info than
"currently not on any branch", 2013-03-13) expects from "git status"
during an interrupted "git rebase" is _not_ sacred, especially when
we are about to fix what "git rebase" records in the reflog.

I am only saying that the last test the commit adds must be kept
unbroken.  I am also saying that, even though that commit did not
add a test for "detached from" case, we should add something like
the attached to protect the behaviour.  These two are sacred.

What happens to be shown during a stopped "git rebase" is not.

I have been assuming the "main" thing Duy wanted to do was the last
test (and the one below), but was this meant as an improvement for
"git status" output during that state?  Showing $ONTO certainly
makes some sense, and from that point of view, the change we are
discussing _will_ be a regression if it just shows a random thing.
If you want to avoid regression, the codepath in wt-status.c should
compensate for the change to "rebase" so that it checks $dotest/onto
and show what is recorded in there.

But in practice, as far as we know other people are working on it in
the near-by thread, I do not think this particular regression (if it
was) matters that much.  At the worst, we can queue that on top of
this topic to fix "rebase ; checkout -" and have them graduate at
the same time to avoid it.

 t/t7512-status-help.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index bf08d4e..e07185e 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -675,6 +675,14 @@ test_expect_success 'status showing detached from a tag' '
 	nothing to commit (use -u to show untracked files)
 	EOF
 	git status --untracked-files=no >actual &&
+	test_i18ncmp expected actual &&
+
+	git reset --hard HEAD^ &&
+	cat >expected <<-\EOF
+	# HEAD detached from atag
+	nothing to commit (use -u to show untracked files)
+	EOF
+	git status --untracked-files=no >actual &&
 	test_i18ncmp expected actual
 '
 

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> I have been assuming the "main" thing Duy wanted to do was the last
> test (and the one below), but was this meant as an improvement for
> "git status" output during that state?  Showing $ONTO certainly
> makes some sense, and from that point of view, the change we are
> discussing _will_ be a regression if it just shows a random thing.
>
> If you want to avoid regression, the codepath in wt-status.c should
> compensate for the change to "rebase" so that it checks $dotest/onto
> and show what is recorded in there.

And it might be just the matter of doing something like this
(totally untested, of course).  Then the people who want to improve
"status" to give more detailed state during "rebase" can further
enhance the logic in this part to give more detailed information.

I have a feeling that without changing anything in t7512 but
applying the change to commit.c in your [6/6], most tests in there
would still pass, and you may even uncover latent *bug* that
expected to show a wrong $ONTO value in "# HEAD detached from/at"
line.

 wt-status.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/wt-status.c b/wt-status.c
index bf84a86..403d48d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1176,7 +1176,11 @@ void wt_status_print(struct wt_status *s)
 			branch_name += 11;
 		else if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.detached_from) {
+
+			if (state.rebase_in_progress) {
+				on_what = _("HEAD detached at ");
+				branch_name = state.onto;
+			} else if (state.detached_from) {
 				unsigned char sha1[20];
 				branch_name = state.detached_from;
 				if (!get_sha1("HEAD", sha1) &&

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
>  wt-status.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/wt-status.c b/wt-status.c
> index bf84a86..403d48d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1176,7 +1176,11 @@ void wt_status_print(struct wt_status *s)
>                         branch_name += 11;
>                 else if (!strcmp(branch_name, "HEAD")) {
>                         branch_status_color = color(WT_STATUS_NOBRANCH, s);
> -                       if (state.detached_from) {
> +
> +                       if (state.rebase_in_progress) {
> +                               on_what = _("HEAD detached at ");
> +                               branch_name = state.onto;
> +                       } else if (state.detached_from) {
>                                 unsigned char sha1[20];
>                                 branch_name = state.detached_from;

Good.  You have proposed a solution to the problem.  However, it is
wrong for the following reasons:

1. It shows a pseudo "HEAD detached at" message.  Everywhere else,
when the user sees a "HEAD detached at $committish" message, git
rev-parse $committish = git rev-parse HEAD.  A rebase-in-progress
seems to be the only exception, and the user has no idea this is
happened.

2. The following no longer updates status:

  # in the middle of a rebase
  $ git reset @~2

The constant "HEAD detached at $onto" message is misleading and Bad.
Besides, wasn't this the primary usecase you wanted?

You previously wrote:
> *1* One thing I could think of is to start sightseeing or (more
>     realistically) manually bisecting at a given release point,
>     reset the detached HEAD around several times, and then want to
>     be reminded where the session started from.  I do not think it
>     is particularly a very good example, though.

Whether the HEAD is detached by bisect or rebase is irrelevant.

3. The problem is not unique to rebase at all; yet you have
special-cased it.  If this isn't a band-aid, what is?  The larger
problem, as I have stated previously, is that 'git status' output
depends on the _implementations_ of various commands (do they write a
"checkout: " message to the reflog or not?).  Therefore, a future
contributor who updates a command to write more sensible reflog
messages will have to apply a similar band-aid.

If you want to take the band-aid approach, I think this is the right
way to do it:

diff --git a/wt-status.c b/wt-status.c
index bf84a86..99c55e3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1182,7 +1182,7 @@ void wt_status_print(struct wt_status *s)
 				if (!get_sha1("HEAD", sha1) &&
 				    !hashcmp(sha1, state.detached_sha1))
 					on_what = _("HEAD detached at ");
-				else
+				else if (!state.rebase_in_progress)
 					on_what = _("HEAD detached from ");
 			} else {
 				branch_name = "";

You have already mentioned that there is a topic cooking to improve
this first-line in the case of rebase, so this regression from a
senseless message isn't a problem.

The problem with this bad-aid, as with all band aids, is that this
will soon explode to become else if(!state.rebase_in_progress &&
!state.bisect_in_progress && ....) when people update those scripts.
If you don't want to go with the band-aid, I have no choice but to
smudge the first-line.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> 2. The following no longer updates status:
>
>   # in the middle of a rebase
>   $ git reset @~2
>
> The constant "HEAD detached at $onto" message is misleading and Bad.
> Besides, wasn't this the primary usecase you wanted?

See the other message.  The first line from status in the middle of
a rebase is secondary.  End-user initiated "checkout" to detach is
the primary thing.

> You previously wrote:
>> *1* One thing I could think of is to start sightseeing or (more
>>     realistically) manually bisecting at a given release point,
>>     reset the detached HEAD around several times, and then want to
>>     be reminded where the session started from.  I do not think it
>>     is particularly a very good example, though.
>
> Whether the HEAD is detached by bisect or rebase is irrelevant.

You read and reacted to "bisect" too literally and missed
"manually".  The scenario I had in mind does _not_ use "git bisect"
command, which may _internally_ run "git checkout" to detach, but
for exactly the same reason why you want to touch "git rebase" in
this series, its use of "checkout" should not count for the next
"checkout -".

The sequence of "manually bisecting" is like

	$ git checkout v1.8.3
        ... test
        $ git status
        $ git log --oneline --first-parent v1.8.2..$detached_from
        ... pick a likely point
        $ git reset --hard $that_point
        ... go back to test further

which I actually do fairly often, as I tend to know more about the
likely place something may have been broken than a mechanical "split
the history into about the same number of commits" bisection.

> 3. The problem is not unique to rebase at all; yet you have
> special-cased it.  If this isn't a band-aid, what is?

It is an illustration for approach (2) in the other message.

In the longer term, you would want richer status output for various
detached state, and that "how about this" patch shows where new code
to support other cases should be added.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> The first line from status in the middle of
> a rebase is secondary.  End-user initiated "checkout" to detach is
> the primary thing.
>
>> 3. The problem is not unique to rebase at all; yet you have
>> special-cased it.  If this isn't a band-aid, what is?
>
> It is an illustration for approach (2) in the other message.
>
> In the longer term, you would want richer status output for various
> detached state, and that "how about this" patch shows where new code
> to support other cases should be added.

Fine.  If that is what you want, we'll have to special-case every
known script-in-progress to stop writing the usual "HEAD detached
from" message.  That'll leave out only the "end-user cases", where you
argue that the message is helpful.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I am only saying that the last test the commit adds must be kept
> unbroken.  I am also saying that, even though that commit did not
> add a test for "detached from" case, we should add something like
> the attached to protect the behaviour.  These two are sacred.
>
> What happens to be shown during a stopped "git rebase" is not.
>
> I have been assuming the "main" thing Duy wanted to do was the last
> test (and the one below), but was this meant as an improvement for
> "git status" output during that state?

On what basis are you making that assumption?  b397ea4 did not add a
test for "detached HEAD from", and there is no evidence of what the
author wanted in the commit message.  What has happened has happened;
the question is: what do _you_ want now?

Read the various "HEAD detached from/to" messages in the rebase tests
in t/status-help and tell me that they make some sense.  It does _not_
show a constant "HEAD detached from $onto", contrary to your
assumption (evidenced by your patch in the later email).  Look at these
hunk added by b397ea4, and tell me that it's not a monkey-patch:

@@ -187,9 +187,10 @@ test_expect_success 'status when rebasing -i in
edit mode' '
        export FAKE_LINES &&
        test_when_finished "git rebase --abort" &&
        ONTO=$(git rev-parse --short HEAD~2) &&
+       TGT=$(git rev-parse --short two_rebase_i) &&
        git rebase -i HEAD~2 &&
        cat >expected <<-EOF &&
-       # Not currently on any branch.
+       # HEAD detached from $TGT
        # You are currently editing a commit while rebasing branch
'\''rebase_i_edit'\'' on '\''$ONTO'\''.
        #   (use "git commit --amend" to amend the current commit)
        #   (use "git rebase --continue" once you are satisfied with
your changes)
@@ -214,8 +215,9 @@ test_expect_success 'status when splitting a commit' '
        ONTO=$(git rev-parse --short HEAD~3) &&
        git rebase -i HEAD~3 &&
        git reset HEAD^ &&
+       TGT=$(git rev-parse --short HEAD) &&
        cat >expected <<-EOF &&
-       # Not currently on any branch.
+       # HEAD detached at $TGT
        # You are currently splitting a commit while rebasing branch
'\''split_commit'\'' on '\''$ONTO'\''.
        #   (Once your working directory is clean, run "git rebase --continue")
        #
@@ -243,10 +245,11 @@ test_expect_success 'status after editing the
last commit with --amend during a
        export FAKE_LINES &&
        test_when_finished "git rebase --abort" &&
        ONTO=$(git rev-parse --short HEAD~3) &&
+       TGT=$(git rev-parse --short three_amend) &&
        git rebase -i HEAD~3 &&
        git commit --amend -m "foo" &&
        cat >expected <<-EOF &&
-       # Not currently on any branch.
+       # HEAD detached from $TGT
        # You are currently editing a commit while rebasing branch
'\''amend_last'\'' on '\''$ONTO'\''.
        #   (use "git commit --amend" to amend the current commit)
        #   (use "git rebase --continue" once you are satisfied with
your changes)


> Showing $ONTO certainly
> makes some sense, and from that point of view, the change we are
> discussing _will_ be a regression if it just shows a random thing.

If showing $ONTO were the intent of the patch, this is no way to go
about it.  Reliable information is readily available in
.git/rebase-apply; no parsing is required, and it is ready for
consumption.

Your argument now is: because the patch _incidentally_ shows $ONTO in
many cases (as evidenced by the hunks above), the objective of the
patch _must_ have been to show $ONTO.  Should I laugh or cry?

> If you want to avoid regression, the codepath in wt-status.c should
> compensate for the change to "rebase" so that it checks $dotest/onto
> and show what is recorded in there.

Now you want to achieve the exact same accidental behavior after
patching rebase/checkout.  This is a never-ending nightmare.

> You are misreading me.  I am not defending every bit at all.

I am not misreading anything.  Despite me having repeatedly shown that
b397ea4 is poorly done with far-reaching unintended consequences, you
are unwilling to admit it.  Instead, you are trying to somehow
preserve this accidental behavior.  And you still haven't been able to
show me how to do that.  As a result, checkout-dash is stalled.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> I am only saying that the last test the commit adds must be kept
>> unbroken.  I am also saying that, even though that commit did not
>> add a test for "detached from" case, we should add something like
>> the attached to protect the behaviour.  These two are sacred.
>>
>> What happens to be shown during a stopped "git rebase" is not.
> ...
> The question is: what do _you_ want now?

It should be fairly clear what _I_ want now.  Re-read the first four
lines you quoted above.

> Read the various "HEAD detached from/to" messages in the rebase tests
> in t/status-help and tell me that they make some sense.

Again, re-read the part you did _not_ quote before those four lines.
I do not think all of them make lot of sense; some of them may.

Unless you are also tackling the topic of the other thread:

  http://thread.gmane.org/gmane.comp.version-control.git/227432/focus=227469

(which I would not suggest to do in this topic), what these lines
say during "rebase" does not really matter, as far as your topic is
concerned.

Two possibilities:

 (1) Assume that the other thread will produce a more reasonable
     semantics when finished; perhaps the first line will go away
     entirely, or maybe it would say something like "# Rebasing;
     head at $commit".

     Your topic does not _care_ what it would say, so you tweak the
     "status" test that is done during "rebase" so that they
     ignore the first lines; or

 (2) Starting from the same assumption as above, but try to minimize
     the semantics change to user-visible behaviour this series
     makes.

     That means that even though the _primary_ thing you want to do
     is to tweak "rebase" and its internal use of "checkout" in such
     a way that reflog will not record the implementation-detail
     checkout (because that will affect the next "checkout -"), make
     sure that "status" while doing "rebase" reports where the
     internal "checkout" of $ONTO detached HEAD from/at.

The "something along this line" patch was an illustration of how
the beginning of the effort for (2) would look like.

As I already said, I could see us going either way.  If we can do
(2), that may be better, but at end it would not matter that much as
long as we will be doing the other topic, too.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> Two possibilities:
>
>  (1) Assume that the other thread will produce a more reasonable
>      semantics when finished; perhaps the first line will go away
>      entirely, or maybe it would say something like "# Rebasing;
>      head at $commit".
>
>      Your topic does not _care_ what it would say, so you tweak the
>      "status" test that is done during "rebase" so that they
>      ignore the first lines; or

You said you didn't want to regress to show senseless information, and
I agreed with that.  What is wrong with the patch I showed in the
previous email?  Smudging is a bad hack, and must only be used as a
last resort: when an another topics updates status to say something
sensible, it will have to unsmudge the tests.

diff --git a/wt-status.c b/wt-status.c
index bf84a86..99c55e3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1182,7 +1182,7 @@ void wt_status_print(struct wt_status *s)
                                if (!get_sha1("HEAD", sha1) &&
                                    !hashcmp(sha1, state.detached_sha1))
                                        on_what = _("HEAD detached at ");
-                               else
+                               else if (!state.rebase_in_progress)
                                        on_what = _("HEAD detached from ");
                        } else {
                                branch_name = "";

>  (2) Starting from the same assumption as above, but try to minimize
>      the semantics change to user-visible behaviour this series
>      makes.

The "try to minimize" is a somewhat admirable goal, but I have shown
that your midway solution is wrong.  Either dedicate a lot of time and
effort towards improving status for rebase, or don't attempt it.

>      That means that even though the _primary_ thing you want to do
>      is to tweak "rebase" and its internal use of "checkout" in such
>      a way that reflog will not record the implementation-detail
>      checkout (because that will affect the next "checkout -"), make
>      sure that "status" while doing "rebase" reports where the
>      internal "checkout" of $ONTO detached HEAD from/at.

Unless we change the first line drastically to say: "rebase in
progress: rebasing onto $ONTO" (or something), I don't think this
makes sense.  And if we were to do that, why not do it properly like
"rebase ($N/$M): onto $ONTO, upstream $UPSTREAM, branch $BRANCH"?
Other people on a different thread are already handling that, and I am
not interested.

So, you have three simple choices now:

1. Accept the simple patch I proposed above.
2. Propose an alternative patch quickly.  *Patch*.  No more English.
3. Reject all patches, and leave me no choice but to smudge.

Which one is it going to be?

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Junio C Hamano wrote:
>> Two possibilities:
>>
>>  (1) Assume that the other thread will produce a more reasonable
>>      semantics when finished; perhaps the first line will go away
>>      entirely, or maybe it would say something like "# Rebasing;
>>      head at $commit".
>>
>>      Your topic does not _care_ what it would say, so you tweak the
>>      "status" test that is done during "rebase" so that they
>>      ignore the first lines; or
>
> You said you didn't want to regress to show senseless information,...

Go back and read it again.

    If you want to avoid regression, the codepath in wt-status.c
    should compensate for the change to "rebase" so that it checks
    $dotest/onto and show what is recorded in there.

> I agreed with that.  What is wrong with the patch I showed in the
> previous email?

Which previous?  My message you are responding to was a response to
your non-patch message, and tangling the In-reply-to backwards will
reach your original patch.

Puzzled....

> Smudging is a bad hack, and must only be used as a
> last resort: when an another topics updates status to say something
> sensible, it will have to unsmudge the tests.

Yes. I just got an impression, from your incessant arguing, that you
are resisting the "ignore the top" as "extra work that is not
interesting to _me_", while I was trying to see what is the best way
to go forward for the _project_.

Honestly, I didn't want to waste my time and was trying to come up
with a compromise, which would be a small regression to the tests
that we will need to fix with the other topic.  Because you already
said that you are not interested in that topic, I was anticipating
that the work to polish the topic would be a lot easier than
anything I would have to do with you in this topic, because others
are a lot easier to collaborate with than you are, and that is where
that suggestion comes from.

> diff --git a/wt-status.c b/wt-status.c
> index bf84a86..99c55e3 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1182,7 +1182,7 @@ void wt_status_print(struct wt_status *s)
>                                 if (!get_sha1("HEAD", sha1) &&
>                                     !hashcmp(sha1, state.detached_sha1))
>                                         on_what = _("HEAD detached at ");
> -                               else
> +                               else if (!state.rebase_in_progress)
>                                         on_what = _("HEAD detached from ");

Is this supposed to be on top of your original patch?

The primary reason I suggested to special case "rebase-in-progress"
in the "how about this" patch was because that way, you can have the
"have builtin/commit.c honor reflog-action" much earlier in the
series, because what this part of the code would say will not be
affected by what is recorded in the reflog.  If the above, together
with the original patch, makes the code match the expectation of the
"rebase stops in the middle and then status runs" tests, that's also
fine.  The other topic needs to redo it in either case anyway.

> Unless we change the first line drastically to say: "rebase in
> progress: rebasing onto $ONTO" (or something), I don't think this
> makes sense.  And if we were to do that, why not do it properly like
> "rebase ($N/$M): onto $ONTO, upstream $UPSTREAM, branch $BRANCH"?
> Other people on a different thread are already handling that,...

That is the thread I was referring to.


>
> So, you have three simple choices now:
>
> 1. Accept the simple patch I proposed above.
> 2. Propose an alternative patch quickly.  *Patch*.  No more English.
> 3. Reject all patches, and leave me no choice but to smudge.
>
> Which one is it going to be?

None of the above.  After going back and forth this long, I won't
want to pick an incremental from the middle of discussion, so (1) is
out.  This is not my itch and I am only helping you in a way that
the result will not hurt the project, so (2) is not it.  But
assuming that "checkout that is done as an implementation detail in
rebase is _not_ checkout from end user's point of view" is where we
want to go, discarding this series is not something we want to see,
either.

A rerolled series that does:

 - Tweak wt-status.c to take information not from reflog, when
   detached during rebase (this may need to tweak existing tests,
   as different "rebase" modes may use 'checkout' liberally in
   different ways);

 - Teach builtin/commit.c to pay attention to reflog-action; thanks
   to the previous step, this will not have to change the tests;

 - Update the reflog message rebase uses, but again this will not
   affect wt-status.c at all.

would be one way to go.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> [...]

I have no desire to argue incessantly.  I just want a solution to the problem!

> A rerolled series that does:
>
>  - Tweak wt-status.c to take information not from reflog, when
>    detached during rebase (this may need to tweak existing tests,
>    as different "rebase" modes may use 'checkout' liberally in
>    different ways);

Please be more specific, and tell me what it should print when a
rebase is in progress.  Would a constant

  # rebase in progress; onto $ONTO

be sufficient?

>  - Teach builtin/commit.c to pay attention to reflog-action; thanks
>    to the previous step, this will not have to change the tests;

Where did builtin/commit.c enter into the equation?  Doesn't it
already respect GIT_REFLOG_ACTION?  See line 1461.

>  - Update the reflog message rebase uses, but again this will not
>    affect wt-status.c at all.

Okay.

Re: [PATCH 5/6] status: do not depend on flaky reflog messages

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

>> I have been assuming the "main" thing Duy wanted to do was the last
>> test (and the one below), but was this meant as an improvement for
>> "git status" output during that state?
>
> On what basis are you making that assumption? 

List archive helps.

http://thread.gmane.org/gmane.comp.version-control.git/217342/focus=217444

Not just that single message, but a few messages in the discussion
that led to the design shows that we wanted both "at" and "from" to
mean something sensible, when a "checkout" initiated by the end-user
detached HEAD and we are not on any branch, even after making
commits or doing resets.

[PATCH 6/6] checkout: respect GIT_REFLOG_ACTION

[Ramkumar Ramachandra]

GIT_REFLOG_ACTION is an environment variable specifying the reflog
message to write after an action is completed.  Several other commands
including merge, reset, and commit respect it.

Fix the failing tests in t/checkout-last by making checkout respect it
too.  You can now expect

  $ git checkout -

to work as expected after any rebase operation.

Also update the tests in t/status-help that rely on rebase writing
"checkout: " messages to the reflog.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/checkout.c       | 11 ++++++++---
 t/t2012-checkout-last.sh |  4 ++--
 t/t7512-status-help.sh   | 13 ++++++-------
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f5b50e5..1e2af85 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -587,7 +587,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				   struct branch_info *new)
 {
 	struct strbuf msg = STRBUF_INIT;
-	const char *old_desc;
+	const char *old_desc, *reflog_msg;
 	if (opts->new_branch) {
 		if (opts->new_orphan_branch) {
 			if (opts->new_branch_log && !log_all_ref_updates) {
@@ -620,8 +620,13 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 	old_desc = old->name;
 	if (!old_desc && old->commit)
 		old_desc = sha1_to_hex(old->commit->object.sha1);
-	strbuf_addf(&msg, "checkout: moving from %s to %s",
-		    old_desc ? old_desc : "(invalid)", new->name);
+
+	reflog_msg = getenv("GIT_REFLOG_ACTION");
+	if (!reflog_msg)
+		strbuf_addf(&msg, "checkout: moving from %s to %s",
+			old_desc ? old_desc : "(invalid)", new->name);
+	else
+		strbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));
 
 	if (!strcmp(new->name, "HEAD") && !new->path && !opts->force_detach) {
 		/* Nothing to do. */
diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index ae6d319..336b6f2 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -116,7 +116,7 @@ test_expect_success 'master...' '
 	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify master^)"
 '
 
-test_expect_failure '"checkout -" works after a rebase' '
+test_expect_success '"checkout -" works after a rebase' '
 	git checkout master &&
 	git checkout other &&
 	git rebase master &&
@@ -124,7 +124,7 @@ test_expect_failure '"checkout -" works after a rebase' '
 	test "z$(git symbolic-ref HEAD)" = "zrefs/heads/master"
 '
 
-test_expect_failure '"checkout -" works after an interactive rebase' '
+test_expect_success '"checkout -" works after an interactive rebase' '
 	git checkout master &&
 	git checkout other &&
 	git rebase -i master &&
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index ed9d57c..f8661e4 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -77,7 +77,7 @@ test_expect_success 'status when rebase in progress before resolving conflicts'
 	ONTO=$(git rev-parse --short HEAD^^) &&
 	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# Not currently on any branch.
 	# You are currently rebasing branch '\''rebase_conflicts'\'' on '\''$ONTO'\''.
 	#   (fix conflicts and then run "git rebase --continue")
 	#   (use "git rebase --skip" to skip this patch)
@@ -104,7 +104,7 @@ test_expect_success 'status when rebase in progress before rebase --continue' '
 	echo three >main.txt &&
 	git add main.txt &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# Not currently on any branch.
 	# You are currently rebasing branch '\''rebase_conflicts'\'' on '\''$ONTO'\''.
 	#   (all conflicts fixed: run "git rebase --continue")
 	#
@@ -136,7 +136,7 @@ test_expect_success 'status during rebase -i when conflicts unresolved' '
 	ONTO=$(git rev-parse --short rebase_i_conflicts) &&
 	test_must_fail git rebase -i rebase_i_conflicts &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# Not currently on any branch.
 	# You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO'\''.
 	#   (fix conflicts and then run "git rebase --continue")
 	#   (use "git rebase --skip" to skip this patch)
@@ -162,7 +162,7 @@ test_expect_success 'status during rebase -i after resolving conflicts' '
 	test_must_fail git rebase -i rebase_i_conflicts &&
 	git add main.txt &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# Not currently on any branch.
 	# You are currently rebasing branch '\''rebase_i_conflicts_second'\'' on '\''$ONTO'\''.
 	#   (all conflicts fixed: run "git rebase --continue")
 	#
@@ -215,9 +215,8 @@ test_expect_success 'status when splitting a commit' '
 	ONTO=$(git rev-parse --short HEAD~3) &&
 	git rebase -i HEAD~3 &&
 	git reset HEAD^ &&
-	TGT=$(git rev-parse --short HEAD) &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $TGT
+	# Not currently on any branch.
 	# You are currently splitting a commit while rebasing branch '\''split_commit'\'' on '\''$ONTO'\''.
 	#   (Once your working directory is clean, run "git rebase --continue")
 	#
@@ -599,7 +598,7 @@ test_expect_success 'status when rebase conflicts with statushints disabled' '
 	ONTO=$(git rev-parse --short HEAD^^) &&
 	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
 	cat >expected <<-EOF &&
-	# HEAD detached at $ONTO
+	# Not currently on any branch.
 	# You are currently rebasing branch '\''statushints_disabled'\'' on '\''$ONTO'\''.
 	#
 	# Unmerged paths:
-- 
1.8.3.1.384.g7cec0b4