Re: [PATCH 5/7] t5520: test --rebase with multiple branches

[Paul Tan <pyokagan@gmail.com>]

Hi,

On Tue, May 5, 2015 at 1:09 AM, Stefan Beller <sbeller@google.com> wrote:
> On Sat, May 2, 2015 at 8:37 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index 25d519d..17c63ff 100755
>> --- a/t/t5520-pull.sh
>> +++ b/t/t5520-pull.sh
>> @@ -223,6 +223,14 @@ test_expect_success '--rebase' '
>>         test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
>>         test new = $(git show HEAD:file2)
>>  '
>> +
>> +test_expect_success '--rebase fails with multiple branches' '
>> +       git reset --hard before-rebase &&
>> +       test_must_fail git pull --rebase . copy master 2>out &&
>> +       test_when_finished "rm -f out" &&
>
> I think it would make sense to switch the previous 2 lines, because the
> test_when_finished should also be run if the test actually fails.
> The actual tested part may segfault, which would interrupt the && chain
> from reaching the test_when_finished line. And here, the line to test
> is meant to be the "git pull --rebase" line, so we'd assume it may be
> stopping there due to failures in the tested program.
>
> If you grep through the code base, you'll find lots of test_when_finished
> commands as the very first command (or the earliest spot where it makes
> sense).

Hmm, thinking about it again, I actually don't really think there's
that much of a need to "rm -f out" in this case ;-)

But yes, I should go review the placements of the test_when_finished calls :-).

Thanks.