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

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 7, 2024, 06:04 UTC
Message-ID: <ZwN6BNPHiXyKGd1t@pks.im>
In-Reply-To: <c9a0cca179bfad129017c7f1085ff87201afb417.1728230769.git.gitgitgadget@gmail.com>

On Sun, Oct 06, 2024 at 04:06:08PM +0000, Usman Akinyemi via GitGitGadget wrote:

Show 8 quoted lines

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

Show 17 quoted lines

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

Show 11 quoted lines

> @@ -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.

Show 15 quoted lines

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