Re: [Outreachy][PATCH v2] t6050: avoid pipes with downstream Git commands

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Chizoba

On 10/10/2024 07:39, chizobajames21@gmail.com wrote:
> From: Chizoba ODINAKA <chizobajames21@gmail.com>
> 
> In pipes, the exit code of a chain of commands is determined by
> the downstream command.

I would perhaps say "final command" rather than "downstream command" as 
in a pipeline "cmd1 | cmd2 | cmd3" cmd2 and cmd3 are downstream of cmd1 
but it is the exit code of cmd3 that will be used

> In order not to loss the entire result code of tests,
> write output of upstreams into a file.

We're interested in checking the exit code of git, but not other 
commands so it would be helpful to make that clear. Usman's patch [1] 
has a good explanation of this.

This patch also changes instances of "grep" to "test_grep" so the commit 
message needs to explain the reason for that change which is that it 
gives a better debugging experience if the test fails.

The patch is looking pretty good, most of the conversions look correct. 
I've left a few comments below


[1] 
https://lore.kernel.org/git/bfff7937cd20737bb5a8791dc7492700b1d7881f.1728315124.git.gitgitgadget@gmail.com

>   test_expect_success 'replace the author' '
> -	git cat-file commit $HASH2 | grep "author A U Thor" &&
> -	R=$(git cat-file commit $HASH2 | sed -e "s/A U/O/" | git hash-object -t commit --stdin -w) &&
> -	git cat-file commit $R | grep "author O Thor" &&
> +	git cat-file commit $HASH2 >actual &&
> +	test_grep "author A U Thor" actual &&
> +	git cat-file commit $HASH2 >actual &&

You don't need to repeat this command now that we are saving the output 
of "git cat-file commit $HASH2"

> +	R=$(sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
> +	git cat-file commit $R >actual &&
> +	test_grep "author O Thor" actual &&

>   test_expect_success 'push branch with replacement' '
> -	git cat-file commit $PARA3 | grep "author A U Thor" &&
> -	S=$(git cat-file commit $PARA3 | sed -e "s/A U/O/" | git hash-object -t commit --stdin -w) &&
> -	git cat-file commit $S | grep "author O Thor" &&
> +	git cat-file commit $PARA3 >actual &&
> +	test_grep "author A U Thor" actual &&
> +	git cat-file commit $PARA3 >actual &&

We can drop this line for the same reason as above

> +	S=$(sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
> +	git cat-file commit $S >actual &&
> +	test_grep "author O Thor" actual &&

> @@ -260,14 +291,14 @@ test_expect_success 'fetch branch with replacement' '
>   		cd clone_dir &&
>   		git fetch origin refs/heads/tofetch:refs/heads/parallel3 &&
>   		git log --pretty=oneline parallel3 >output.txt &&
> -		! grep $PARA3 output.txt &&
> +		! test_grep $PARA3 output.txt &&

test_grep will print an error message the pattern does not match. In 
this case we expect it not to match so we want to print an error if it 
does match. We can do that with

	test_grep ! $PARA3 output.txt &&

>   test_expect_success 'index-pack and replacements' '
> -	git --no-replace-objects rev-list --objects HEAD |
> +	git --no-replace-objects rev-list --objects HEAD >actual &&
>   	git --no-replace-objects pack-objects test- &&

This command has lost its input - you need to use '<actual' to get it to 
read output from "git rev-list". This test itself could probably do a 
better job of checking that index-pack does what we expect but that is 
outside the scope of this patch.

>   	git index-pack test-*.pack
>   '

Everything that I've not commented on looks correct to me.

Best Wishes

Phillip