Re: [PATCH 1/3] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 12, 2024 at 11:09:32PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> The exit code of the preceding command in a pipe is disregarded. So
> if that preceding command is a Git command that fails, the test would
> not fail. Instead, by saving the output of that Git command to a file,
> and removing the pipe, we make sure the test will fail if that Git
> command fails. This particular patch focuses on all `git show` and
> some instances of `git cat-file`.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index f171af3061d..96a65783c47 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -319,7 +319,8 @@ test_expect_success 'retain authorship' '
>  	GIT_AUTHOR_NAME="Twerp Snog" git commit -m "different author" &&
>  	git tag twerp &&
>  	git rebase -i --onto primary HEAD^ &&
> -	git show HEAD | grep "^Author: Twerp Snog"
> +	git show HEAD >actual &&
> +	grep "^Author: Twerp Snog" actual
>  '

Good.

> @@ -397,7 +400,9 @@ test_expect_success 'multi-squash only fires up editor once' '
>  			git rebase -i $base
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
> -	test 1 = $(git show | grep ONCE | wc -l)
> +	git show >output &&
> +	count=$(grep ONCE output | wc -l) &&
> +	test 1 = $count
>  '

I think moving 'git show' out of the pipeline is a good step here, but I
don't think we need to store $count in a separate variable. It would be
fine to write:

    git show >output &&
    test 1 = $(grep ONCE output | wc -l)

or even to replace the subshell with 'grep -c' instead of piping 'grep'
to 'wc -l'.

>  test_expect_success 'multi-fixup does not fire up editor' '
> @@ -410,7 +415,9 @@ test_expect_success 'multi-fixup does not fire up editor' '
>  			git rebase -i $base
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
> -	test 0 = $(git show | grep NEVER | wc -l) &&
> +	git show >output &&
> +	count=$(grep NEVER output | wc -l) &&
> +	test 0 = $count &&
>  	git checkout @{-1} &&
>  	git branch -D multi-fixup
>  '

Same notes from above here and the next two tests (elided from my
response) below.

> @@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
>  	) &&
>  	git cat-file commit HEAD | sed -e 1,/^\$/d > actual-squash-fixup &&
>  	test_cmp expect-squash-fixup actual-squash-fixup &&
> -	git cat-file commit HEAD@{2} |
> -		grep "^# This is a combination of 3 commits\."  &&
> -	git cat-file commit HEAD@{3} |
> -		grep "^# This is a combination of 2 commits\."  &&
> +	git cat-file commit HEAD@{2} >actual &&
> +	grep "^# This is a combination of 3 commits\." actual &&

Is there a more descriptive name for the output here than just 'actual'?

Thanks,
Taylor