Re: [PATCH] t3206: test_when_finished before dirtying operations, not after

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Aug 5, 2024 at 8:55 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Many existing tests in this script perform operation(s) and then use
>> test_when_finished to define how to undo the effect of the
>> operation(s).
>>
>> This is backwards.  When your operation(s) fail before you manage to
>> successfully call test_when_finished (remember, that these commands
>> must be all &&-chained, so a failure of an earlier operation mean
>> your test_when_finished may not be executed at all).  You must
>> establish how to clean up your mess with test_when_finished before
>> you create the mess to be cleaned up.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>> ---
>>  t/t3206-range-diff.sh | 52 +++++++++++++++++++++----------------------
>> diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
>> @@ -533,9 +533,9 @@ test_expect_success 'dual-coloring' '
>>  for prev in topic main..topic
>>  do
>>         test_expect_success "format-patch --range-diff=$prev" '
>> +               test_when_finished "rm 000?-*" &&
>>                 git format-patch --cover-letter --range-diff=$prev \
>>                         main..unmodified >actual &&
>> -               test_when_finished "rm 000?-*" &&
>
> Do we care whether the action invoked by `test_when_finished` itself
> succeeds or fails? In particular, should this be using `rm -f` rather
> than `rm`?

Thanks for good eyes.  The original avoids that issue by making sure
it only installs the clean-up after the operation to create crufts
successfully completes ;-)  Of course, if it fails in the middle,
then the crufts are left behind X-<.

>> @@ -606,9 +606,9 @@ test_expect_success 'basic with modified format.pretty without "commit "' '
>>  test_expect_success 'range-diff compares notes by default' '
>> +       test_when_finished git notes remove topic unmodified &&
>>         git notes add -m "topic note" topic &&
>>         git notes add -m "unmodified note" unmodified &&
>> -       test_when_finished git notes remove topic unmodified &&
>
> Similarly, should this be using `|| :`?

Ah, I forgot that "notes remove" would barf when there is no note to
remove, instead of being idempotent no-op.  Yes, you'd need ||: there.

>     test_when_finished "git notes remove topic unmodified || :" &&