diff mbox series

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

Message ID 20241010063906.51767-1-chizobajames21@gmail.com (mailing list archive)
State Superseded
Headers show
Series [Outreachy,v2] t6050: avoid pipes with downstream Git commands | expand

Commit Message

Chizoba ODINAKA Oct. 10, 2024, 6:39 a.m. UTC 
From: Chizoba ODINAKA <chizobajames21@gmail.com>

In pipes, the exit code of a chain of commands is determined by
the downstream command. In order not to loss the entire result code of tests,
write output of upstreams into a file.

Signed-off-by: Chizoba ODINAKA <chizobajames21@gmail.com>
---
Changes in v2:
- split multiple commands chain on the same line across multiple line,
  for easier readability
- replace "grep" with "test_grep", for more context in case of a "grep"
  failure

 t/t6050-replace.sh | 133 +++++++++++++++++++++++++++++----------------
 1 file changed, 87 insertions(+), 46 deletions(-)

Comments

Phillip Wood Oct. 10, 2024, 2:08 p.m. UTC | #1 
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
Chizoba ODINAKA Oct. 10, 2024, 6:51 p.m. UTC | #2 
Hi Philip,
On Thu, 10 Oct 2024 at 15:08, Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> 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
>
Yes, final command is a more appropriate explain.
> > 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.
>
I just read that sentence again, it obviously needs some clarity. "In order not
to miss the exit code of any Git command, avoid using pipe and write
output into a file"
has more clarity. I will look up on Usman's patch [1], before my next changes.

> 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.
>
I had included that in my "Changes in v2", appended beneath my "Sign-off-by".
"Changes in v2:
- split multiple commands chain on the same line across multiple line,
  for easier readability
- replace "grep" with "test_grep", for more context in case of a "grep"
  failure"
Maybe it was not so obvious that you didn't notice, or it is not the
traditional way of including it.

> 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.
>
Thanks Philip for the review, I will make the needed changes in my
next patch. And look into
the index-pack proposal in a new patch, since it is outside this scope.
> Best Wishes
>
> Phillip

Thanks
Chizoba
Phillip Wood Oct. 11, 2024, 9:17 a.m. UTC | #3 
Hi Chizoba

On 10/10/2024 19:51, Chizoba ODINAKA wrote:
> On Thu, 10 Oct 2024 at 15:08, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 10/10/2024 07:39, chizobajames21@gmail.com wrote:
>>> From: Chizoba ODINAKA <chizobajames21@gmail.com>
>>>
>>> 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.
>>
> I just read that sentence again, it obviously needs some clarity. "In order not
> to miss the exit code of any Git command, avoid using pipe and write
> output into a file"
> has more clarity. I will look up on Usman's patch [1], before my next changes.
> 
>> 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.
>>
> I had included that in my "Changes in v2", appended beneath my "Sign-off-by".
> "Changes in v2:
> - split multiple commands chain on the same line across multiple line,
>    for easier readability
> - replace "grep" with "test_grep", for more context in case of a "grep"
>    failure"
> Maybe it was not so obvious that you didn't notice, or it is not the
> traditional way of including it.

It's great that you listed the changes between versions below the "---" 
line - that is really helpful for reviewers. However those comments are 
not part of the commit message when the patch is applied. It is 
important for the commit message to explain the reasons for all the 
changes in a patch so that someone reading it later can understand why 
the change was made. Therefore the grep->test_grep change should be 
explained in the commit message. In general one should avoid making 
unrelated changes in the same commit. In this case I think one can argue 
that the changes are small enough that combining them is fine.

> Thanks Philip for the review, I will make the needed changes in my
> next patch.

That's great, I look forward to reviewing it

> And look into
> the index-pack proposal in a new patch, since it is outside this scope.

Let's finish this patch first. I'm not sure what the best way to improve 
that test is at the moment.

Best Wishes

Phillip
diff mbox series

Patch

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index d7702fc756..599b6bc66c 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -98,30 +98,43 @@  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 &&
+	test_grep "author A U Thor" actual &&
+	git cat-file commit $HASH2 >actual &&
+	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 &&
 	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 &&
+	test_grep "O Thor" actual &&
+	git show $HASH2 >actual &&
+	test_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 &&
+	test_grep "author O Thor" actual &&
+	git --no-replace-objects cat-file commit $HASH2 >actual &&
+	test_grep "author A U Thor" actual &&
+	git show $HASH2 >actual &&
+	test_grep "O Thor" actual &&
+	git --no-replace-objects show $HASH2 >actual &&
+	test_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 &&
+	test_grep "author A U Thor"  actual &&
+	GIT_NO_REPLACE_OBJECTS=1 git show $HASH2 >actual &&
+	test_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 &&
+	test_grep "author A U Thor" actual &&
+	git show $HASH2 >actual &&
+	test_grep "A U Thor" actual
 '
 
 cat >tag.sig <<EOF
@@ -148,14 +161,18 @@  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 &&
+		test_grep "A U Thor" actual &&
+		git show $HASH2 >actual &&
+		test_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 &&
+		test_grep "O Thor" actual &&
+		git show $HASH2 >actual &&
+		test_grep "O Thor" actual &&
 		git cat-file commit $R
 	)
 '
@@ -169,13 +186,15 @@  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 &&
+	test_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 &&
+	test_grep "O Thor" actual &&
 	test_must_fail git replace $HASH2 $R &&
 	git replace -f $HASH2 $R &&
 	test_must_fail git replace -f &&
@@ -186,7 +205,8 @@  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 &&
+	test_grep "O Thor" actual &&
 	test_must_fail git replace $HASH2 $R &&
 	git replace -f $HASH2 $R &&
 	test_must_fail git replace --force &&
@@ -209,10 +229,12 @@  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 &&
+	test_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 +247,8 @@  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 &&
+	test_grep $PARA2 actual &&
 	git remote add cloned ./clone_dir
 '
 
@@ -234,23 +257,31 @@  test_expect_success 'push to cloned repo' '
 	(
 		cd clone_dir &&
 		git checkout parallel &&
-		git log --pretty=oneline | grep $PARA2
+		git log --pretty=oneline >actual &&
+		test_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 &&
+	test_grep "author A U Thor" actual &&
+	git cat-file commit $PARA3 >actual &&
+	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 &&
 	git replace $PARA3 $S &&
-	git show $HASH6~2 | grep "O Thor" &&
-	git show $PARA3 | grep "O Thor" &&
+	git show $HASH6~2 >actual &&
+	test_grep "O Thor" actual &&
+	git show $PARA3 >actual &&
+	test_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 &&
+		test_grep $PARA3 actual &&
+		git show $PARA3 >actual &&
+		test_grep "A U 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 &&
 		git show $PARA3 >para3.txt &&
-		grep "A U Thor" para3.txt &&
+		test_grep "A U Thor" para3.txt &&
 		git fetch origin "refs/replace/*:refs/replace/*" &&
 		git log --pretty=oneline parallel3 >output.txt &&
-		grep $PARA3 output.txt &&
+		test_grep $PARA3 output.txt &&
 		git show $PARA3 >para3.txt &&
-		grep "O Thor" para3.txt
+		test_grep "O Thor" para3.txt
 	)
 '
 
@@ -284,7 +315,7 @@  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 rev-list --objects HEAD >actual &&
 	git --no-replace-objects pack-objects test- &&
 	git index-pack test-*.pack
 '
@@ -319,7 +350,8 @@  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 &&
+	test_grep $PARA3 actual &&
 	echo $PARA3 | git cat-file --batch
 '
 
@@ -344,7 +376,8 @@  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 +389,8 @@  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 +408,16 @@  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 &&
+	test_grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual &&
+	test_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 &&
+	test_grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual &&
+	test_grep "A fake Thor" actual
 '
 
 test_expect_success '--edit and change nothing or command failed' '
@@ -387,8 +425,10 @@  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 &&
+	test_grep "$PARA3" actual &&
+	git cat-file commit "$PARA3" >actual &&
+	test_grep "A fake Thor" actual
 '
 
 test_expect_success 'replace ref cleanup' '
@@ -468,7 +508,8 @@  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 &&
+	test_grep "^mergetag object" actual
 '
 
 test_expect_success GPG '--graft on a commit with a mergetag' '