[Outreachy] t6050: avoid pipes in git related commands

Commit Message

Chizoba ODINAKA Oct. 8, 2024, 4:21 p.m. UTC

From: Chizoba ODINAKA <[e-mail]>

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.

Signed-off-by: Chizoba ODINAKA <[e-mail]>
---
 t/t6050-replace.sh | 86 +++++++++++++++++++++++-----------------------
 1 file changed, 43 insertions(+), 43 deletions(-)

Comments

Patrick Steinhardt Oct. 9, 2024, 7:28 a.m. UTC | #1

On Tue, Oct 08, 2024 at 05:21:17PM +0100, [e-mail] wrote:
> From: Chizoba ODINAKA <[e-mail]>
> 
> 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.

> Signed-off-by: Chizoba ODINAKA <[e-mail]>
> ---
>  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

Chizoba ODINAKA Oct. 10, 2024, 7:26 a.m. UTC | #2

On Wed, 9 Oct 2024 at 08:28, Patrick Steinhardt <[e-mail]> wrote:
>
> On Tue, Oct 08, 2024 at 05:21:17PM +0100, [e-mail] wrote:
> > From: Chizoba ODINAKA <[e-mail]>
> >
> > 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.
>
Thanks Patrick for the review, and for pointing this out, I totally
agree with you.
> 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.
>
And I will effect this proposal the next change.

> > Signed-off-by: Chizoba ODINAKA <[e-mail]>
> > ---
> >  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.
>
I made these recommended changes.
> Thanks!
>
> Patrick

Thank you.
Chizoba

Patch

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
 '
 
 test_expect_success 'test --no-replace-objects option' '
-	git cat-file commit $HASH2 | grep "author O Thor" &&
-	git --no-replace-objects cat-file commit $HASH2 | grep "author A U Thor" &&
-	git show $HASH2 | grep "O Thor" &&
-	git --no-replace-objects show $HASH2 | grep "A U Thor"
+	git cat-file commit $HASH2 >actual && grep "author O Thor" actual &&
+	git --no-replace-objects cat-file commit $HASH2 >actual && grep "author A U Thor" actual &&
+	git show $HASH2 >actual && grep "O Thor" actual &&
+	git --no-replace-objects show $HASH2 >actual && grep "A U Thor" actual
 '
 
 test_expect_success 'test GIT_NO_REPLACE_OBJECTS env variable' '
-	GIT_NO_REPLACE_OBJECTS=1 git cat-file commit $HASH2 | grep "author A U Thor" &&
-	GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 | grep "A U Thor"
+	GIT_NO_REPLACE_OBJECTS=1 git cat-file commit $HASH2 >actual && grep "author A U Thor"  actual &&
+	GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 >actual && grep "A U Thor" actual
 '
 
 test_expect_success 'test core.usereplacerefs config option' '
 	test_config core.usereplacerefs false &&
-	git cat-file commit $HASH2 | grep "author A U Thor" &&
-	git show $HASH2 | grep "A U Thor"
+	git cat-file commit $HASH2 >actual && grep "author A U Thor" actual &&
+	git show $HASH2 >actual && grep "A U Thor" actual
 '
 
 cat >tag.sig <<EOF
@@ -148,14 +148,14 @@  test_expect_success 'repack, clone and fetch work' '
 	git clone --no-hardlinks . clone_dir &&
 	(
 		cd clone_dir &&
-		git show HEAD~5 | grep "A U Thor" &&
-		git show $HASH2 | grep "A U Thor" &&
+		git show HEAD~5 >actual && grep "A U Thor" actual &&
+		git show $HASH2 >actual && grep "A U Thor" actual &&
 		git cat-file commit $R &&
 		git repack -a -d &&
 		test_must_fail git cat-file commit $R &&
 		git fetch ../ "refs/replace/*:refs/replace/*" &&
-		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 &&
 		git cat-file commit $R
 	)
 '
@@ -169,13 +169,13 @@  test_expect_success '"git replace" listing and deleting' '
 	test_must_fail git replace --delete &&
 	test_must_fail git replace -l -d $HASH2 &&
 	git replace -d $HASH2 &&
-	git show $HASH2 | grep "A U Thor" &&
+	git show $HASH2 >actual && grep "A U Thor" actual &&
 	test -z "$(git replace -l)"
 '
 
 test_expect_success '"git replace" replacing' '
 	git replace $HASH2 $R &&
-	git show $HASH2 | grep "O Thor" &&
+	git show $HASH2 >actual && grep "O Thor" actual &&
 	test_must_fail git replace $HASH2 $R &&
 	git replace -f $HASH2 $R &&
 	test_must_fail git replace -f &&
@@ -186,7 +186,7 @@  test_expect_success '"git replace" resolves sha1' '
 	SHORTHASH2=$(git rev-parse --short=8 $HASH2) &&
 	git replace -d $SHORTHASH2 &&
 	git replace $SHORTHASH2 $R &&
-	git show $HASH2 | grep "O Thor" &&
+	git show $HASH2 >actual && grep "O Thor" actual &&
 	test_must_fail git replace $HASH2 $R &&
 	git replace -f $HASH2 $R &&
 	test_must_fail git replace --force &&
@@ -209,10 +209,10 @@  test_expect_success '"git replace" resolves sha1' '
 #
 test_expect_success 'create parallel branch without the bug' '
 	git replace -d $HASH2 &&
-	git show $HASH2 | grep "A U Thor" &&
+	git show $HASH2 >actual && grep "A U Thor" actual &&
 	git checkout $HASH1 &&
 	git cherry-pick $HASH2 &&
-	git show $HASH5 | git apply &&
+	git show $HASH5 >actual && git apply actual &&
 	git commit --amend -m "hello: 4 more lines WITHOUT the bug" hello &&
 	PARA2=$(git rev-parse --verify HEAD) &&
 	git cherry-pick $HASH3 &&
@@ -225,7 +225,7 @@  test_expect_success 'create parallel branch without the bug' '
 	git checkout main &&
 	cur=$(git rev-parse --verify HEAD) &&
 	test "$cur" = "$HASH7" &&
-	git log --pretty=oneline | grep $PARA2 &&
+	git log --pretty=oneline >actual && grep $PARA2 actual &&
 	git remote add cloned ./clone_dir
 '
 
@@ -234,23 +234,23 @@  test_expect_success 'push to cloned repo' '
 	(
 		cd clone_dir &&
 		git checkout parallel &&
-		git log --pretty=oneline | grep $PARA2
+		git log --pretty=oneline >actual && grep $PARA2 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 && grep "author A U Thor" actual &&
+	S=$(git cat-file commit $PARA3 >actual && sed -e "s/A U/O/" actual | git hash-object -t commit --stdin -w) &&
+	git cat-file commit $S >actual && grep "author O Thor" actual &&
 	git replace $PARA3 $S &&
-	git show $HASH6~2 | grep "O Thor" &&
-	git show $PARA3 | grep "O Thor" &&
+	git show $HASH6~2 >actual && grep "O Thor" actual &&
+	git show $PARA3 >actual && grep "O Thor" actual &&
 	git push cloned $HASH6^:refs/heads/parallel2 &&
 	(
 		cd clone_dir &&
 		git checkout parallel2 &&
-		git log --pretty=oneline | grep $PARA3 &&
-		git show $PARA3 | grep "A U Thor"
+		git log --pretty=oneline >actual && grep $PARA3 actual &&
+		git show $PARA3 >actual && grep "A U Thor" actual
 	)
 '
 
@@ -284,8 +284,8 @@  test_expect_success 'bisect and replacements' '
 '
 
 test_expect_success 'index-pack and replacements' '
-	git --no-replace-objects rev-list --objects HEAD |
-	git --no-replace-objects pack-objects test- &&
+	git --no-replace-objects rev-list --objects HEAD >actual &&
+	git --no-replace-objects pack-objects test- <actual &&
 	git index-pack test-*.pack
 '
 
@@ -319,7 +319,7 @@  test_expect_success '-f option bypasses the type check' '
 '
 
 test_expect_success 'git cat-file --batch works on replace objects' '
-	git replace | grep $PARA3 &&
+	git replace >actual && grep $PARA3 actual &&
 	echo $PARA3 | git cat-file --batch
 '
 
@@ -344,7 +344,7 @@  test_expect_success 'test --format medium' '
 		echo "$PARA3 -> $S" &&
 		echo "$MYTAG -> $HASH1"
 	} | sort >expected &&
-	git replace -l --format medium | sort >actual &&
+	git replace -l --format medium >actual && sort actual &&
 	test_cmp expected actual
 '
 
@@ -356,7 +356,7 @@  test_expect_success 'test --format long' '
 		echo "$PARA3 (commit) -> $S (commit)" &&
 		echo "$MYTAG (tag) -> $HASH1 (commit)"
 	} | sort >expected &&
-	git replace --format=long | sort >actual &&
+	git replace --format=long >actual && sort actual &&
 	test_cmp expected actual
 '
 
@@ -374,12 +374,12 @@  test_expect_success 'setup fake editors' '
 test_expect_success '--edit with and without already replaced object' '
 	test_must_fail env GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
 	GIT_EDITOR=./fakeeditor git replace --force --edit "$PARA3" &&
-	git replace -l | grep "$PARA3" &&
-	git cat-file commit "$PARA3" | grep "A fake Thor" &&
+	git replace -l >actual && grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual && grep "A fake Thor" actual &&
 	git replace -d "$PARA3" &&
 	GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
-	git replace -l | grep "$PARA3" &&
-	git cat-file commit "$PARA3" | grep "A fake Thor"
+	git replace -l >actual && grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual && grep "A fake Thor" actual
 '
 
 test_expect_success '--edit and change nothing or command failed' '
@@ -387,8 +387,8 @@  test_expect_success '--edit and change nothing or command failed' '
 	test_must_fail env GIT_EDITOR=true git replace --edit "$PARA3" &&
 	test_must_fail env GIT_EDITOR="./failingfakeeditor" git replace --edit "$PARA3" &&
 	GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
-	git replace -l | grep "$PARA3" &&
-	git cat-file commit "$PARA3" | grep "A fake Thor"
+	git replace -l >actual && grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual && grep "A fake Thor" actual
 '
 
 test_expect_success 'replace ref cleanup' '
@@ -468,7 +468,7 @@  test_expect_success GPG 'set up a merge commit with a mergetag' '
 	git checkout main &&
 	git merge -s ours test_tag &&
 	HASH10=$(git rev-parse --verify HEAD) &&
-	git cat-file commit $HASH10 | grep "^mergetag object"
+	git cat-file commit $HASH10 >actual && grep "^mergetag object" actual
 '
 
 test_expect_success GPG '--graft on a commit with a mergetag' '