Re: [PATCH v2 0/6] Fix checkout-dash to work with rebase

Ramkumar Ramachandra [off-list ref] writes:

Junio C Hamano wrote:

quoted

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index a58406d..c175ef1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh

@@ -934,6 +934,21 @@ test_expect_success 'rebase --edit-todo can be used to modify todo' '
        test L = $(git cat-file commit HEAD | sed -ne \$p)
 '

+test_expect_success 'rebase -i produces readable reflog' '

I don't know if this test is a good idea at all.

It is.

This catches "a change to GIT_REFLOG_ACTION that is meant to only
apply to checkout done internally in git-rebase leaks to the later
codepath and affects the reflog message left by rebase--interactive".

Apply the test change without the "do not leak" part in the fix-up
(queued as a single "SQUASH???" commit on 'pu') to what you posted
earlier and see how it breaks.

    # rr/rebase-checkout-reflog with fix-up
    $ git checkout 33b1cdeb 
    # revert the fix but keep the test
    $ git checkout HEAD^ -- git-rebase--interactive.sh git-rebase.sh
    # run the test
    $ make && cd t && sh ./t3404-*.sh -v -i

The test fails with this:

    --- expect      2013-06-18 16:09:21.000000000 +0000
    +++ actual      2013-06-18 16:09:21.000000000 +0000
    @@ -1,4 +1,4 @@
    -rebase -i (start): checkout I
    +rebase -i (start): checkout branch-reflog-test: checkout I
     rebase -i (pick): G
     rebase -i (pick): H
     rebase -i (finish): returning to refs/heads/branch-reflog-test

"checkout branch-reflog-test" part is leaking from your setting of
GIT_REFLOG_ACTION for "git checkout" in "git rebase"; it is only
necessary to affect that "checkout" and it should not affect later
commands that write reflog entries, but we see the breakage.

And that is why I did the fix-up to make sure your changes only
apply to "git checkout".

If you apply the test part of that fixup to what you posted today,
what would you see?  I didn't look at the patches very closely, but
I wouldn't be surprised if they are still leaking the change meant
only to "checkout" to the later stages and breaking that test (the
reason why I would not be surprised is because the impression I am
getting from reading your responses is that you do not understand
why it is bad not to limit the setting only to checkout).

Why make an exception in the case of rebase -i?

Because the whole point of last two patches are "word reflog
messages better for rebase", the improvements made by these two
patches are better protected from future changes; it also makes sure
that deliberate improvements will show up as changes to the tests.