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

From: Jacob Abel <jacobabel@nullpo.dev>
Date: Apr 18, 2023, 03:53 UTC
Message-ID: <olztmib77r35mx33a655obqpxui6coj74hfxoxfvcudnkpbqns@ixerneqaai45>
In-Reply-To: <xmqq8reqkyfz.fsf@gitster.g>

On 23/04/17 02:09PM, Junio C Hamano wrote:

Show 12 quoted lines

> Jacob Abel <jacobabel@nullpo.dev> writes:
>
> >  test_expect_success 'add --quiet' '
> > +	test_when_finished "git worktree remove -f -f another-worktree" &&
> > +	test_when_finished cat actual >&2 &&
>
> I doubt that this redirection does anything you expect it do.
> Doesn't it redirect the standard output that is emitted by the
> test_when_finished shell function when it registers another
> test_cleanup scriptlet to the standard error, and when test_cleanup
> is indeed run, wouldn't "cat actual" send its output to the standard
> output?

Yes that's correct. I figured "grab from stderr, cat to stderr" but yes this isn't necessarily what we want here. Dropping the `>&2` causes it to work as expected.

Show 31 quoted lines

>
> No, I am not suggesting to write the line as:
>
> 	test_when_finished "cat >&2 actual" &&
>
> >  	git worktree add --quiet another-worktree main 2>actual &&
> >  	test_must_be_empty actual
>
> The reason why I do not suggest "fixing" the above is because
> test_must_be_empty, when fails, does this:
>
>         test_must_be_empty () {
>                 test "$#" -ne 1 && BUG "1 param"
>                 test_path_is_file "$1" &&
>                 if test -s "$1"
>                 then
>                         echo "'$1' is not empty, it contains:"
>                         cat "$1"
>                         return 1
>                 fi
>         }
>
> i.e. it sends the contents of "actual" to the standard output
> already.  When it succeeds, of course "actual" is empty, and there
> is no point in showing its contents.
>
> 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'.  "git worktree remove" fails and your "cat" in
> the test_cleanup does not even trigger, by the way.

That should not be the case. From what I've seen, the test cleanup is executed in reverse order from the order they are declared with `test_when_finished`. So as long as `cat` is the last command added to test cleanup it should always execute immediately after the first command in the script fails. And as long as the `cat` is added immediately before the `git worktree add`, that means it should be the most recently added in the event that command fails.

Show 6 quoted lines

>
> There may be cases where having something like this might help, but
> running the test with "-x" is not it---that case is already covered
> by what test_must_be_empty gives us, I think.
>
> [...]

I attached an example below to try to illustrate the issue I was attempting to solve. If `git worktree add ... 2>actual` fails, redirecting stderr to actual eats the output that would normally show w/ `-x`. Then because a command fails, it never reaches the `test_must_be_empty`.

Test results of running `sh t2400-*.sh -x` for this test when `git worktree add` fails (caused in this case by adding `--bad-arg` to the command):

    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 --bad-arg another-worktree main 2>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 --bad-arg another-worktree main
    error: last command exited with $?=129
    ++ cat actual
    error: unknown option `bad-arg'
    usage: git worktree add [-f] [--detach] [--checkout] [--lock [--reason <string>]]
                            [(-b | -B) <new-branch>] <path> [<commit-ish>]

        -f, --force           checkout <branch> even if already checked out in other worktree
        -b <branch>           create a new branch
        -B <branch>           create or reset a branch
        -d, --detach          detach HEAD at named commit
        --checkout            populate the new working tree
        --lock                keep the new working tree locked
        --reason <string>     reason for locking
        -q, --quiet           suppress progress reporting
        --track               set up tracking mode (see git-branch(1))
        --guess-remote        try to match the new branch name with a remote-tracking branch

    ++ exit 129
    ++ eval_ret=129
    ++ git worktree remove -f -f another-worktree
    fatal: 'another-worktree' is not a working tree
    ++ eval_ret=128
    ++ :
    not ok 37 - add --quiet

The same test but with the `test_when_finished cat actual` removed:

    expecting success of 2400.37 'add --quiet':
            test_when_finished "git worktree remove -f -f another-worktree" &&
            git worktree add --quiet --bad-arg another-worktree main 2>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=$?; :'
    ++ git worktree add --quiet --bad-arg another-worktree main
    error: last command exited with $?=129
    ++ git worktree remove -f -f another-worktree
    fatal: 'another-worktree' is not a working tree
    ++ eval_ret=128
    ++ :
    not ok 37 - add --quiet