[PATCH 0/3] Fix git checkout - (early preview)

[PATCH 0/3] Fix git checkout - (early preview)

[Ramkumar Ramachandra]

Hi,

So, this 'git checkout -' not working after a 'rebase -i' has annoyed
me to no end.  This is the fix.

Unfortunately, some tests fail and I'm still tracking down what
exactly is going on.

Thanks.

Ramkumar Ramachandra (3):
  t/checkout-last: checkout - doesn't work after rebase -i
  checkout: respect GIT_REFLOG_ACTION
  rebase -i: write better reflog messages for start

 builtin/checkout.c         | 11 ++++++++---
 git-rebase--interactive.sh |  2 ++
 t/t2012-checkout-last.sh   |  8 ++++++++
 3 files changed, 18 insertions(+), 3 deletions(-)

-- 
1.8.3.254.g60f9e5b

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

[Ramkumar Ramachandra]

The following command

  $ git checkout -

does not work as expected after a 'git rebase -i'.

Add a failing test documenting this bug.

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

diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
index b44de9d..5729487 100755
--- a/t/t2012-checkout-last.sh
+++ b/t/t2012-checkout-last.sh
@@ -116,4 +116,12 @@ test_expect_success 'master...' '
 	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify 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.254.g60f9e5b

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> The following command
>
>   $ git checkout -
>
> does not work as expected after a 'git rebase -i'.
>
> Add a failing test documenting this bug.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  t/t2012-checkout-last.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t2012-checkout-last.sh b/t/t2012-checkout-last.sh
> index b44de9d..5729487 100755
> --- a/t/t2012-checkout-last.sh
> +++ b/t/t2012-checkout-last.sh
> @@ -116,4 +116,12 @@ test_expect_success 'master...' '
>  	test "z$(git rev-parse --verify HEAD)" = "z$(git rev-parse --verify 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"
> +'

Hmph, you were on "other" and then ran "rebase -i" to rebase it.
When you are done, you are on "other".  You want to go back to the
one before you checked out the branch you started your "rebase -i"
on, which is "master".

OK, the expectation makes sense to me.

These four are all valid ways to spell the "rebase -i master" step.

	git rebase -i master
        git rebase -i --onto master master
        git rebase -i master other
        git rebase -i --onto master master other

and I think it is sensible to expect

 (1) they all behave the same way; or

 (2) the first two behave the same, but the latter two explicitly
     checks out 'other' by giving the last argument.  If you are not
     on 'other' when you run the "rebase -i", "checkout -" should
     come back to the branch before 'other', i.e. the branch you
     started your "rebase -i" on.

In other words, (2) would mean:

	git checkout master &&
        git checkout -b naster &&
        git rebase -i master other &&
	# ends up on other
        test_string_equal "$(git symbolic-ref HEAD)" refs/heads/other &&
        git checkout - &&
	# we were on naster before we rebased 'other'
        test_string_equal "$(git symbolic-ref HEAD)" refs/heads/naster

It is a bit unclear what the following should expect.

	git checkout master &&
        git checkout other &&
        git rebase -i master other &&
	# ends up on other
        test_string_equal "$(git symbolic-ref HEAD)" refs/heads/other &&
        git checkout - &&
	# we are on ??? before we rebased other
        test_string_equal "$(git symbolic-ref HEAD)" refs/heads/???

One could argue that the "first checkout other and then rebase" done
by rebase becomes a no-op and the user should be aware of that
because rebase is started while on that other branch, in which case
we should come back to 'master'.  From consistency point of view,
one could instead argue that we were on 'other' before we rebased
it, in which case it may not be unreasonable for "checkout -" to
come back to 'other'.  I tend to prefer the former (i.e. go back to
'master') over the latter but not by a large margin.

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

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> These four are all valid ways to spell the "rebase -i master" step.
>
> and I think it is sensible to expect
>
>  (1) they all behave the same way; or

Yes.  My reasoning is very simple: a rebase is a rebase; it should not
write "checkout: " messages to the reflog.  Therefore, the @{-<N>}
will ignore it; for the purposes of checkout -, the rebase event is
inconsequential.

[PATCH 2/3] 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.  Other commands including
merge, reset, and commit respect it.

This incidentally fixes a bug in t/checkout-last.  You can now expect

  $ git checkout -

to work fine after an interactive rebase.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/checkout.c       | 11 ++++++++---
 t/t2012-checkout-last.sh |  2 +-
 2 files changed, 9 insertions(+), 4 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 5729487..ab80da7 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 an interactive rebase' '
+test_expect_success '"checkout -" works after an interactive rebase' '
 	git checkout master &&
 	git checkout other &&
 	git rebase -i master &&
-- 
1.8.3.254.g60f9e5b

Re: [PATCH 2/3] checkout: respect GIT_REFLOG_ACTION

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> GIT_REFLOG_ACTION is an environment variable specifying the reflog
> message to write after an action is completed.  Other commands including
> merge, reset, and commit respect it.
>
> This incidentally fixes a bug in t/checkout-last.  You can now expect
>
>   $ git checkout -
>
> to work fine after an interactive rebase.
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
>  builtin/checkout.c       | 11 ++++++++---
>  t/t2012-checkout-last.sh |  2 +-
>  2 files changed, 9 insertions(+), 4 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));

Looks very sensible; we may need to audit programs that set and
export REFLOG_ACTION to make sure they do not do so incorrectly,
which wouldn't have mattered if they called "checkout" but now it
would with this fix, though.



>  	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 5729487..ab80da7 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 an interactive rebase' '
> +test_expect_success '"checkout -" works after an interactive rebase' '
>  	git checkout master &&
>  	git checkout other &&
>  	git rebase -i master &&

[PATCH 3/3] rebase -i: write better reflog messages for start

[Ramkumar Ramachandra]

Invoking 'git rebase -i' writes the following line to the reflog at the
start of the operation:

  rebase -i (start)

This is not very useful.  Make it more informative like:

  rebase -i (start): checkout master

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 5822b2c..a05a6e4 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -837,6 +837,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
@@ -980,6 +981,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.254.g60f9e5b

Re: [PATCH 3/3] rebase -i: write better reflog messages for start

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Invoking 'git rebase -i' writes the following line to the reflog at the
> start of the operation:
>
>   rebase -i (start)
>
> This is not very useful.  Make it more informative like:
>
>   rebase -i (start): checkout master

Makes sense to me, at least within the scope of the patch context.

I am curious what breaks, though.

> 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 5822b2c..a05a6e4 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -837,6 +837,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
> @@ -980,6 +981,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

Re: [PATCH 3/3] rebase -i: write better reflog messages for start

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> I am curious what breaks, though.

t/status-help.  Looks seriously unrelated, and I'm breaking my head
over it.  Any clues?

--- expected    2013-06-10 17:16:42.276356867 +0000
+++ actual      2013-06-10 17:16:42.279690201 +0000
@@ -1,4 +1,4 @@
-# HEAD detached at 000106f
+# HEAD detached from 88a81b6
 # You are currently rebasing branch 'rebase_conflicts' on '000106f'.
 #   (fix conflicts and then run "git rebase --continue")
 #   (use "git rebase --skip" to skip this patch)
not ok 5 - status when rebase in progress before resolving conflicts

Re: [PATCH 3/3] rebase -i: write better reflog messages for start

[Ramkumar Ramachandra]

Ramkumar Ramachandra wrote:
> t/status-help.  Looks seriously unrelated, and I'm breaking my head
> over it.  Any clues?

Damn it!  A recent commit is responsible for this avalanche in test
breakages: b397ea (status: show more info than "currently not on any
branch", 2013-03-13).  It re-implements a backward version of
grab_nth_branch_switch(): grab_1st_switch() essentially _relies_ on
the random unintended pollution that rebase writes to the reflog to
print a more useful (?) status :/

I have no choice but to completely redo this bit, and update all the
tests.  Let me know if there is some easy way to work around this that
I'm missing.

Thanks.

Re: [PATCH 3/3] rebase -i: write better reflog messages for start

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> Ramkumar Ramachandra wrote:
>> t/status-help.  Looks seriously unrelated, and I'm breaking my head
>> over it.  Any clues?
>
> Damn it!  A recent commit is responsible for this avalanche in test
> breakages: b397ea (status: show more info than "currently not on any
> branch", 2013-03-13).  It re-implements a backward version of
> grab_nth_branch_switch(): grab_1st_switch() essentially _relies_ on
> the random unintended pollution that rebase writes to the reflog to
> print a more useful (?) status :/

After "git checkout v1.3.0", it is reasonable to expect that you can
tell what you checked out and what state you are in.  If you then
made a few commits or resetted to some other commit, it is debatable
if "detached from v1.3.0" is useful or the subtle difference between
"detached at" vs "detached from" is confusing.

But what does it have to do with rebase polluting the reflog?

Re: [PATCH 3/3] rebase -i: write better reflog messages for start

[Ramkumar Ramachandra]

Junio C Hamano wrote:
> But what does it have to do with rebase polluting the reflog?

See the series I just posted.