[v2] t3206: test_when_finished before dirtying operations, not after

Commit Message

Junio C Hamano Aug. 6, 2024, 5:04 p.m. UTC

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.

Also make sure that the body of test_when_finished deals with case
where the cruft it wants to remove failed to be created, by using
"rm -f" (instead of "rm") to remove potential cruft files, and
having "|| :" after "git notes remove" to remove potential cruft
notes---both of these by default fail when asked to remove something
that does not exist, instead of being silently idempotent no-ops.

Helped-by: Eric Sunshine <[e-mail]>
Signed-off-by: Junio C Hamano <[e-mail]>
---
 * a range-diff appears at the end.  The last paragraph of the
   proposed log message is new, and the differences in patch text
   are about making sure test_when_finished commands are prepared
   for their body failing.

 t/t3206-range-diff.sh | 52 +++++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 26 deletions(-)


Range-diff:
1:  591c4d512f ! 1:  3a3c1a1a44 t3206: test_when_finished before dirtying operations, not after
    @@ Commit message
         establish how to clean up your mess with test_when_finished before
         you create the mess to be cleaned up.
     
    +    Also make sure that the body of test_when_finished deals with case
    +    where the cruft it wants to remove failed to be created, by using
    +    "rm -f" (instead of "rm") to remove potential cruft files, and
    +    having "|| :" after "git notes remove" to remove potential cruft
    +    notes---both of these by default fail when asked to remove something
    +    that does not exist, instead of being silently idempotent no-ops.
    +
    +    Helped-by: Eric Sunshine <[e-mail]>
         Signed-off-by: Junio C Hamano <[e-mail]>
     
      ## t/t3206-range-diff.sh ##
    @@ t/t3206-range-diff.sh: 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?-*" &&
    ++		test_when_finished "rm -f 000?-*" &&
      		git format-patch --cover-letter --range-diff=$prev \
      			main..unmodified >actual &&
     -		test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success "explicit --no-cover-letter defeats i
      '
      
      test_expect_success 'format-patch --range-diff as commentary' '
    --	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
    - 	test_when_finished "rm 0001-*" &&
    -+	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
    ++	test_when_finished "rm -f 0001-*" &&
    + 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
    +-	test_when_finished "rm 0001-*" &&
      	test_line_count = 1 actual &&
      	test_grep "^Range-diff:$" 0001-* &&
      	grep "> 1: .* new message" 0001-*
      '
      
      test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
    --	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
    - 	test_when_finished "rm v2.9-0001-*" &&
    -+	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
    ++	test_when_finished "rm -f v2.9-0001-*" &&
    + 	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
    +-	test_when_finished "rm v2.9-0001-*" &&
      	test_line_count = 1 actual &&
      	test_grep "^Range-diff:$" v2.9-0001-* &&
      	grep "> 1: .* new message" v2.9-0001-*
      '
      
      test_expect_success 'format-patch --range-diff reroll-count with a integer' '
    --	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
    - 	test_when_finished "rm v2-0001-*" &&
    -+	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
    ++	test_when_finished "rm -f v2-0001-*" &&
    + 	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
    +-	test_when_finished "rm v2-0001-*" &&
      	test_line_count = 1 actual &&
      	test_grep "^Range-diff ..* v1:$" v2-0001-* &&
      	grep "> 1: .* new message" v2-0001-*
      '
      
      test_expect_success 'format-patch --range-diff with v0' '
    --	git format-patch --range-diff=HEAD~1 -v0 HEAD~1 >actual &&
    - 	test_when_finished "rm v0-0001-*" &&
    -+	git format-patch --range-diff=HEAD~1 -v0 HEAD~1 >actual &&
    ++	test_when_finished "rm -f v0-0001-*" &&
    + 	git format-patch --range-diff=HEAD~1 -v0 HEAD~1 >actual &&
    +-	test_when_finished "rm v0-0001-*" &&
      	test_line_count = 1 actual &&
      	test_grep "^Range-diff:$" v0-0001-* &&
      	grep "> 1: .* new message" v0-0001-*
    @@ t/t3206-range-diff.sh: test_expect_success 'basic with modified format.pretty wi
      '
      
      test_expect_success 'range-diff compares notes by default' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
    @@ t/t3206-range-diff.sh: test_expect_success 'range-diff compares notes by default
      '
      
      test_expect_success 'range-diff with --no-notes' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
    @@ t/t3206-range-diff.sh: test_expect_success 'range-diff with --no-notes' '
      '
      
      test_expect_success 'range-diff with multiple --notes' '
    -+	test_when_finished git notes --ref=note1 remove topic unmodified &&
    ++	test_when_finished "git notes --ref=note1 remove topic unmodified || :" &&
      	git notes --ref=note1 add -m "topic note1" topic &&
      	git notes --ref=note1 add -m "unmodified note1" unmodified &&
     -	test_when_finished git notes --ref=note1 remove topic unmodified &&
    -+	test_when_finished git notes --ref=note2 remove topic unmodified &&
    ++	test_when_finished "git notes --ref=note2 remove topic unmodified || :" &&
      	git notes --ref=note2 add -m "topic note2" topic &&
      	git notes --ref=note2 add -m "unmodified note2" unmodified &&
     -	test_when_finished git notes --ref=note2 remove topic unmodified &&
    @@ t/t3206-range-diff.sh: test_expect_success 'range-diff with multiple --notes' '
      
      # `range-diff` should act like `log` with regards to notes
      test_expect_success 'range-diff with --notes=custom does not show default notes' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 --ref=custom remove topic unmodified &&
    ++	test_when_finished "git notes --ref=custom remove topic unmodified || :" &&
      	git notes --ref=custom add -m "topic note" topic &&
      	git notes --ref=custom add -m "unmodified note" unmodified &&
     -	test_when_finished git notes remove topic unmodified &&
    @@ t/t3206-range-diff.sh: test_expect_success 'range-diff with --notes=custom does
      '
      
      test_expect_success 'format-patch --range-diff does not compare notes by default' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
    -+	test_when_finished "rm 000?-*" &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff does not c
      '
      
      test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	test_when_finished "git notes remove topic unmodified || :" &&
      	git notes add -m "topic note" topic &&
     -	git notes --ref=custom add -m "topic note (custom)" topic &&
      	git notes add -m "unmodified note" unmodified &&
    --	git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
    --	test_when_finished git notes remove topic unmodified &&
    - 	test_when_finished git notes --ref=custom remove topic unmodified &&
    ++	test_when_finished "git notes --ref=custom remove topic unmodified || :" &&
     +	git notes --ref=custom add -m "topic note (custom)" topic &&
    -+	git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
    -+	test_when_finished "rm 000?-*" &&
    + 	git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
    +-	test_when_finished git notes remove topic unmodified &&
    +-	test_when_finished git notes --ref=custom remove topic unmodified &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --notes=custom --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff does not c
      '
      
      test_expect_success 'format-patch --range-diff with --no-notes' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
    -+	test_when_finished "rm 000?-*" &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --no-notes --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff with --no-
      '
      
      test_expect_success 'format-patch --range-diff with --notes' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
    -+	test_when_finished "rm 000?-*" &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --notes --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff with --not
      '
      
      test_expect_success 'format-patch --range-diff with format.notes config' '
    -+	test_when_finished git notes remove topic unmodified &&
    ++	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 &&
      	test_config format.notes true &&
    -+	test_when_finished "rm 000?-*" &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&
    @@ t/t3206-range-diff.sh: test_expect_success 'format-patch --range-diff with forma
      '
      
      test_expect_success 'format-patch --range-diff with multiple notes' '
    -+	test_when_finished git notes --ref=note1 remove topic unmodified &&
    ++	test_when_finished "git notes --ref=note1 remove topic unmodified || :" &&
      	git notes --ref=note1 add -m "topic note1" topic &&
      	git notes --ref=note1 add -m "unmodified note1" unmodified &&
     -	test_when_finished git notes --ref=note1 remove topic unmodified &&
    -+	test_when_finished git notes --ref=note2 remove topic unmodified &&
    ++	test_when_finished "git notes --ref=note2 remove topic unmodified || :" &&
      	git notes --ref=note2 add -m "topic note2" topic &&
      	git notes --ref=note2 add -m "unmodified note2" unmodified &&
     -	test_when_finished git notes --ref=note2 remove topic unmodified &&
    -+	test_when_finished "rm 000?-*" &&
    ++	test_when_finished "rm -f 000?-*" &&
      	git format-patch --notes=note1 --notes=note2 --cover-letter --range-diff=$prev \
      		main..unmodified >actual &&
     -	test_when_finished "rm 000?-*" &&

Patch

diff --git a/t/t3206-range-diff.sh b/t/t3206-range-diff.sh
index a767c3520e..d2ca43d6aa 100755
--- 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 -f 000?-*" &&
 		git format-patch --cover-letter --range-diff=$prev \
 			main..unmodified >actual &&
-		test_when_finished "rm 000?-*" &&
 		test_line_count = 5 actual &&
 		test_grep "^Range-diff:$" 0000-* &&
 		grep "= 1: .* s/5/A" 0000-* &&
@@ -560,32 +560,32 @@  test_expect_success "explicit --no-cover-letter defeats implied --cover-letter"
 '
 
 test_expect_success 'format-patch --range-diff as commentary' '
+	test_when_finished "rm -f 0001-*" &&
 	git format-patch --range-diff=HEAD~1 HEAD~1 >actual &&
-	test_when_finished "rm 0001-*" &&
 	test_line_count = 1 actual &&
 	test_grep "^Range-diff:$" 0001-* &&
 	grep "> 1: .* new message" 0001-*
 '
 
 test_expect_success 'format-patch --range-diff reroll-count with a non-integer' '
+	test_when_finished "rm -f v2.9-0001-*" &&
 	git format-patch --range-diff=HEAD~1 -v2.9 HEAD~1 >actual &&
-	test_when_finished "rm v2.9-0001-*" &&
 	test_line_count = 1 actual &&
 	test_grep "^Range-diff:$" v2.9-0001-* &&
 	grep "> 1: .* new message" v2.9-0001-*
 '
 
 test_expect_success 'format-patch --range-diff reroll-count with a integer' '
+	test_when_finished "rm -f v2-0001-*" &&
 	git format-patch --range-diff=HEAD~1 -v2 HEAD~1 >actual &&
-	test_when_finished "rm v2-0001-*" &&
 	test_line_count = 1 actual &&
 	test_grep "^Range-diff ..* v1:$" v2-0001-* &&
 	grep "> 1: .* new message" v2-0001-*
 '
 
 test_expect_success 'format-patch --range-diff with v0' '
+	test_when_finished "rm -f v0-0001-*" &&
 	git format-patch --range-diff=HEAD~1 -v0 HEAD~1 >actual &&
-	test_when_finished "rm v0-0001-*" &&
 	test_line_count = 1 actual &&
 	test_grep "^Range-diff:$" v0-0001-* &&
 	grep "> 1: .* new message" v0-0001-*
@@ -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 &&
 	git range-diff --no-color main..topic main..unmodified \
 		>actual &&
 	sed s/Z/\ /g >expect <<-EOF &&
@@ -630,9 +630,9 @@  test_expect_success 'range-diff compares notes by default' '
 '
 
 test_expect_success 'range-diff with --no-notes' '
+	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 &&
 	git range-diff --no-color --no-notes main..topic main..unmodified \
 		>actual &&
 	cat >expect <<-EOF &&
@@ -645,12 +645,12 @@  test_expect_success 'range-diff with --no-notes' '
 '
 
 test_expect_success 'range-diff with multiple --notes' '
+	test_when_finished "git notes --ref=note1 remove topic unmodified || :" &&
 	git notes --ref=note1 add -m "topic note1" topic &&
 	git notes --ref=note1 add -m "unmodified note1" unmodified &&
-	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	test_when_finished "git notes --ref=note2 remove topic unmodified || :" &&
 	git notes --ref=note2 add -m "topic note2" topic &&
 	git notes --ref=note2 add -m "unmodified note2" unmodified &&
-	test_when_finished git notes --ref=note2 remove topic unmodified &&
 	git range-diff --no-color --notes=note1 --notes=note2 main..topic main..unmodified \
 		>actual &&
 	sed s/Z/\ /g >expect <<-EOF &&
@@ -678,12 +678,12 @@  test_expect_success 'range-diff with multiple --notes' '
 
 # `range-diff` should act like `log` with regards to notes
 test_expect_success 'range-diff with --notes=custom does not show default notes' '
+	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 --ref=custom remove topic unmodified || :" &&
 	git notes --ref=custom add -m "topic note" topic &&
 	git notes --ref=custom add -m "unmodified note" unmodified &&
-	test_when_finished git notes remove topic unmodified &&
-	test_when_finished git notes --ref=custom remove topic unmodified &&
 	git range-diff --notes=custom main..topic main..unmodified \
 		>actual &&
 	! grep "## Notes ##" actual &&
@@ -691,12 +691,12 @@  test_expect_success 'range-diff with --notes=custom does not show default notes'
 '
 
 test_expect_success 'format-patch --range-diff does not compare 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 &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	test_line_count = 5 actual &&
 	test_grep "^Range-diff:$" 0000-* &&
 	grep "= 1: .* s/5/A" 0000-* &&
@@ -708,26 +708,26 @@  test_expect_success 'format-patch --range-diff does not compare notes by default
 '
 
 test_expect_success 'format-patch --notes=custom --range-diff only compares custom notes' '
+	test_when_finished "git notes remove topic unmodified || :" &&
 	git notes add -m "topic note" topic &&
-	git notes --ref=custom add -m "topic note (custom)" topic &&
 	git notes add -m "unmodified note" unmodified &&
+	test_when_finished "git notes --ref=custom remove topic unmodified || :" &&
+	git notes --ref=custom add -m "topic note (custom)" topic &&
 	git notes --ref=custom add -m "unmodified note (custom)" unmodified &&
-	test_when_finished git notes remove topic unmodified &&
-	test_when_finished git notes --ref=custom remove topic unmodified &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --notes=custom --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	grep "## Notes (custom) ##" 0000-* &&
 	! grep "## Notes ##" 0000-*
 '
 
 test_expect_success 'format-patch --range-diff with --no-notes' '
+	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 &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --no-notes --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	test_line_count = 5 actual &&
 	test_grep "^Range-diff:$" 0000-* &&
 	grep "= 1: .* s/5/A" 0000-* &&
@@ -739,12 +739,12 @@  test_expect_success 'format-patch --range-diff with --no-notes' '
 '
 
 test_expect_success 'format-patch --range-diff with --notes' '
+	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 &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --notes --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	test_line_count = 5 actual &&
 	test_grep "^Range-diff:$" 0000-* &&
 	grep "= 1: .* s/5/A" 0000-* &&
@@ -767,13 +767,13 @@  test_expect_success 'format-patch --range-diff with --notes' '
 '
 
 test_expect_success 'format-patch --range-diff with format.notes config' '
+	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 &&
 	test_config format.notes true &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	test_line_count = 5 actual &&
 	test_grep "^Range-diff:$" 0000-* &&
 	grep "= 1: .* s/5/A" 0000-* &&
@@ -796,15 +796,15 @@  test_expect_success 'format-patch --range-diff with format.notes config' '
 '
 
 test_expect_success 'format-patch --range-diff with multiple notes' '
+	test_when_finished "git notes --ref=note1 remove topic unmodified || :" &&
 	git notes --ref=note1 add -m "topic note1" topic &&
 	git notes --ref=note1 add -m "unmodified note1" unmodified &&
-	test_when_finished git notes --ref=note1 remove topic unmodified &&
+	test_when_finished "git notes --ref=note2 remove topic unmodified || :" &&
 	git notes --ref=note2 add -m "topic note2" topic &&
 	git notes --ref=note2 add -m "unmodified note2" unmodified &&
-	test_when_finished git notes --ref=note2 remove topic unmodified &&
+	test_when_finished "rm -f 000?-*" &&
 	git format-patch --notes=note1 --notes=note2 --cover-letter --range-diff=$prev \
 		main..unmodified >actual &&
-	test_when_finished "rm 000?-*" &&
 	test_line_count = 5 actual &&
 	test_grep "^Range-diff:$" 0000-* &&
 	grep "= 1: .* s/5/A" 0000-* &&