patch, 3 partsFix git checkout - (early preview)

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

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(+)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

Junio C Hamano wrote:

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.

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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.

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(+)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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

I am curious what breaks, though.

Junio C Hamano wrote:

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

Ramkumar Ramachandra wrote:

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.

Ramkumar Ramachandra <artagnon@gmail.com> writes:

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?

Junio C Hamano wrote:

See the series I just posted.