Re: [PATCH v4 1/2] t4014: cleanups in a few tests

["Rubén Justo" <rjusto@gmail.com>]

On Fri, Jun 07, 2024 at 10:14:10AM -0700, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > Arrange things we are going to create to be removed at end, and then
> > start creating them.  That way, we will clean them up even if we fail
> > after creating some but before the end of the command.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  t/t4014-format-patch.sh | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> > index e37a1411ee..5fb5250df4 100755
> > --- a/t/t4014-format-patch.sh
> > +++ b/t/t4014-format-patch.sh
> > @@ -820,8 +820,8 @@ test_expect_success 'format-patch --notes --signoff' '
> >  '
> >  
> >  test_expect_success 'format-patch notes output control' '
> > +	test_when_finished "git notes remove HEAD" &&
> >  	git notes add -m "notes config message" HEAD &&
> > -	test_when_finished git notes remove HEAD &&
> 
> If "notes add" fails to create a note for HEAD, test_when_finished
> would notice that it cannot remove a note from HEAD, wouldn't it?

Yep.  Something like this, no?

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index de9e8455b3..1088c435e0 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -820,7 +820,7 @@ test_expect_success 'format-patch --notes --signoff' '
 '
 
 test_expect_success 'format-patch notes output control' '
-       test_when_finished "git notes remove HEAD" &&
+       test_when_finished "git notes remove HEAD || :" &&
        git notes add -m "notes config message" HEAD &&
 
        git format-patch -1 --stdout >out &&
@@ -849,7 +849,7 @@ test_expect_success 'format-patch notes output control' '
 
 test_expect_success 'format-patch with multiple notes refs' '
        test_when_finished "git notes --ref note1 remove HEAD;
-                           git notes --ref note2 remove HEAD" &&
+                           git notes --ref note2 remove HEAD || :" &&
        git notes --ref note1 add -m "this is note 1" HEAD &&
        git notes --ref note2 add -m "this is note 2" HEAD &&
 
@@ -893,7 +893,7 @@ test_expect_success 'format-patch with multiple notes refs in config' '
        test_when_finished "test_unconfig format.notes" &&
 
        test_when_finished "git notes --ref note1 remove HEAD;
-                           git notes --ref note2 remove HEAD" &&
+                           git notes --ref note2 remove HEAD || :" &&
        git notes --ref note1 add -m "this is note 1" HEAD &&
        git notes --ref note2 add -m "this is note 2" HEAD &&

> If you do
> 
>                 ! grep "notes config message" out &&
>                 git format-patch -1 --stdout --no-notes --notes >out &&
>         -	grep "notes config message" out
>         +	grep "notes config message" out &&
>         +	git notes remove HEAD
>          '
> 
> at the end of this passing test to remove the note from HEAD (so
> that when-finished handler has nothing to remove), and run "sh
> t4014-format-patch.sh -i -v", this test piece 4014.70 fails with
> 
> 	...
>             notes config message
>         Removing note for object HEAD
>         Object HEAD has no note
>         not ok 70 - format-patch notes output control
> 
> A failure in the when-finished handler is noticed (which we might
> argue is a misfeature)

Dropping it doesn't seem like something to be strongly opposed to :-)

> , and that is why it is a good idea to write
> 
> 	test_when_finished 'rm -f cruft-that-may-be-created' &&
> 	do what might create cruft-that-may-be-created
> 
> with "-f".
> 
> A standard trick can be found in the output of
> 
> 	$ git grep 'finished.*|| *:' t/
> 
> Thanks.

Thank you.