Ramkumar Ramachandra <artag...@gmail.com> writes:

> 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.
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to