Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > On Mon, Aug 5, 2024 at 8:55 PM Junio C Hamano <gitster@xxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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 || :" &&
