Re: [Outreachy][PATCH] t6050: avoid pipes in git related commands

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 9, 2024, 07:28 UTC
Message-ID: <ZwYwm2-ixmyYVqo8@pks.im>
In-Reply-To: <20241008162117.6452-1-chizobajames21@gmail.com>

On Tue, Oct 08, 2024 at 05:21:17PM +0100, chizobajames21@gmail.com wrote:

Show 5 quoted lines

> From: Chizoba ODINAKA <chizobajames21@gmail.com>
> 
> In pipes, the exit code of a chain of commands is determined by
> the downstream command. For more accurate info on exit code tests,
> write output of upstreams into a file.

Nit: it isn't really about accuracy, but rather about losing the return
code entirely. I'd also mention as part of your observation that t6050
still contains this pattern, which isn't currently obvious from just
reading the commit message standalone.

I'd also propose the following subject: "t6050: avoid pipes with downstream Git commands", which reflects the fact that Git commands can be at the end of the pipe without much of an issue.

Show 26 quoted lines

> Signed-off-by: Chizoba ODINAKA <chizobajames21@gmail.com>
> ---
>  t/t6050-replace.sh | 86 +++++++++++++++++++++++-----------------------
>  1 file changed, 43 insertions(+), 43 deletions(-)
> 
> diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
> index d7702fc756..6b9811ed67 100755
> --- a/t/t6050-replace.sh
> +++ b/t/t6050-replace.sh
> @@ -98,30 +98,30 @@ test_expect_success 'set up buggy branch' '
>  '
>  
>  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 && grep "author A U Thor" actual &&
> +	R=$(git cat-file commit $HASH2 >actual && sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
> +	git cat-file commit $R >actual && grep "author O Thor" actual &&
> 
>  	git update-ref refs/replace/$HASH2 $R &&
> -	git show HEAD~5 | grep "O Thor" &&
> -	git show $HASH2 | grep "O Thor"
> +	git show HEAD~5 >actual && grep "O Thor" actual &&
> +	git show $HASH2 >actual && grep "O Thor" actual
>  '

We don't typically chain multiple commands on the same line, as it becomes hard to read very fast. So these should all be split across multiple lines. The same is true for other tests you have converted.

Furthermore, I'd recommend to replace "grep" with "test_grep", which is a convenience wrapper that provides more context in case the grep might have failed. It would for example output the contents of "actual", which helps quite a lot in the context of failing CI jobs.

Thanks!

Patrick