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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2013, 16:15 UTC
Message-ID: <7vip1b75uc.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CALkWK0n2AvL49DNp1t7ty0Dneprd7r_V2p=brS0qMKz=wdZz6A@mail.gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 12 quoted lines

> Junio C Hamano wrote:
>> 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.