Subject: Re: [PATCH v9 2/8] t2400: print captured git output when finished

From: Junio C Hamano <[email-masked]>
Date: Tue, 18 Apr 2023 09:34:15 -0700

Jacob Abel <[email-masked]> writes:

>> So "sh t2400-*.sh -x -i" already shows "cat actual" output.  Try
>> the attached patch on top of this one and running it would show
>> the above message shown by test_must_be_empty and the contents of
>> the file 'actual'.
>
> That should not be the case.

Here is how it fails and test_must_be_empty complains that the
"actual" file is not empty, if you run the script with "-x -i" after
applying the patch in the message you are responding to on top of
this step.

    expecting success of 2400.37 'add --quiet':
            test_when_finished "git worktree remove -f -f another-worktree" &&
            test_when_finished cat actual >&2 &&
            git worktree add --quiet another-worktree main 2>actual &&
    echo foo >>actual &&
            test_must_be_empty actual

    ++ test_when_finished 'git worktree remove -f -f another-worktree'
    ++ test 0 = 0
    ++ test_cleanup='{ git worktree remove -f -f another-worktree
                    } && (exit "$eval_ret"); eval_ret=$?; :'
    ++ test_when_finished cat actual
    ++ test 0 = 0
    ++ test_cleanup='{ cat actual
                    } && (exit "$eval_ret"); eval_ret=$?; { git worktree remove -f -f another-worktree
                    } && (exit "$eval_ret"); eval_ret=$?; :'
    ++ git worktree add --quiet another-worktree main
    ++ echo foo
    ++ test_must_be_empty actual
    ++ test 1 -ne 1
    ++ test_path_is_file actual
    ++ test 1 -ne 1
    ++ test -f actual
    ++ test -s actual
    ++ echo ''\''actual'\'' is not empty, it contains:'
    'actual' is not empty, it contains:
    ++ cat actual
    foo
    ++ return 1
    error: last command exited with $?=1
    not ok 37 - add --quiet

Observe what test_must_be_empty does in the last part of the
transcript above.  If you run it without "-i", then the trace will
show "cat actual" twice (one from test_must_be_empty above, then a
redundant one from the test_when_finished).

Another reason why we shouldn't add this to test_when_finished is
because it would not help those who run the tests with "-i" option.
The test_cleanup handlers are meant to be "clean-up" routines, and
they are not run when the user uses "-i", intending to go into the
test directory after seeing the test fail and inspect what is left
there.