Re: [PATCH 1/3] t3404: preserve test_tick state across short SHA-1 collision test

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 25, 2013, 07:53 UTC
Message-ID: <CAPig+cSns+_mOMRVRv2Kqb3vHNQGoE4NGZ3ELu7y3pQ57VqPXw@mail.gmail.com>
In-Reply-To: <20130825055550.GN2882@elie.Belkin>

On Sun, Aug 25, 2013 at 1:55 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 30 quoted lines

> Hi,
>
> Eric Sunshine wrote:
>
>> The short SHA-1 collision test requires carefully crafted commits in
>> order to ensure a collision at rebase time.
>
> Yeah, this breaks the usual rule that tests should be independent
> of hashing function.  But it's the best we can do, I think.
>
> [...]
>> --- a/t/t3404-rebase-interactive.sh
>> +++ b/t/t3404-rebase-interactive.sh
>> @@ -994,17 +994,23 @@ test_expect_success 'short SHA-1 setup' '
>>       test_when_finished "git checkout master" &&
>>       git checkout --orphan collide &&
>>       git rm -rf . &&
>> +     (
>>       unset test_tick &&
>>       test_commit collide1 collide &&
>>       test_commit --notick collide2 collide &&
>>       test_commit --notick "collide3 115158b5" collide collide3 collide3
>> +     )
>
> Would be clearer if the code in a subshell were indented:
>
>         (
>                 unset test_tick &&
>                 test_commit ...
>         )

I considered it, but decided against it for a couple reasons:

* In this script, there already is a mix between the two styles:
indented vs. unindented.

* In this particular patch, the test_commit line creating commit3
wrapped beyond 80 columns when indented.

In v2 of the series (for which you also made the same observation), the collide3 test_commit line is shorter, so I could have indented, however, I left it alone since nobody complained about it (and because there already is the mix of styles). Should this be worth a re-roll?

Show 15 quoted lines

> [...]
>>  test_expect_success 'short SHA-1 collide' '
>>       test_when_finished "reset_rebase && git checkout master" &&
>>       git checkout collide &&
>> +     (
>> +     unset test_tick &&
>> +     test_tick &&
>>       FAKE_COMMIT_MESSAGE="collide2 815200e" \
>>       FAKE_LINES="reword 1 2" git rebase -i HEAD~2
>> +     )
>
> Likewise.
>
> Hope that helps,
> Jonathan