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

[Patrick Steinhardt <ps@pks.im>]

On Sun, Oct 06, 2024 at 04:06:08PM +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 some of the instances
> which include `git show` and `git cat-files`.

Well-described.

> 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
>  '

One thing to note is that it would be preferable to use `test_grep`
instead of `grep` here. `test_grep` brings in some additional benefits
over plain `grep` like better diagnosis of issues, and it would also
print the file if things didn't match. That makes it way easier e.g. in
our CI to see what the actual output was.

There is no need to reroll this patch series just because of that
though, as your changes are a strict improvement by themselves already.
But if you do end up rerolling it would be nice to incorporate this.

> @@ -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
>  '
>  
>  test_expect_success 'multi-fixup does not fire up editor' '

I was wondering whether the following might be nicer:

    git show >output &&
    grep ONCE output >output.filtered &&
    test_line_count = 1 output.filtered

But after seeing this I don't strongly lean into one or another
direction. So please feel free to use either your current or my proposed
style, I'm fine with either.

> @@ -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 &&
> +	git cat-file commit HEAD@{3} >actual &&
> +	grep "^# This is a combination of 2 commits\." actual  &&
>  	git checkout @{-1} &&
>  	git branch -D squash-fixup
>  '

Nice cleanups while at it.

Patrick