diff mbox series

[v2,Outreachy,v1] t3404: avoid losing exit status to pipes

Message ID pull.1805.v2.git.git.1728203495287.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series [v2,Outreachy,v1] t3404: avoid losing exit status to pipes | expand

Commit Message

Usman Akinyemi Oct. 6, 2024, 8:31 a.m. UTC
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.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
    [Outreachy][Patch v2] t3404: avoid losing exit status to pipes
    
    At the beginning of my task, I made the mistake of submitting two
    patches for two separate commits instead of one. The first patch
    addressed the issue of losing the Git exit status due to pipes.
    
    After submitting the first patch, I noticed that the output of wc -l was
    failing due to trailing whitespace. I attempted to fix this by using tr
    -d to remove the whitespace. However, instead of squashing the two
    patches into one, I inadvertently created another commit.
    
    Eric Sunshine sunshine@sunshineco.com provided valuable feedback during
    the review process. He explained the details of the patches to me and
    pointed out that using tr -d was unnecessary to resolve the whitespace
    issue.
    
    The root cause of the whitespace issue was quoting $count in the test
    command, which led to the inclusion of whitespace in the comparison. By
    removing the quotes around $count, the comparison works as expected
    without the need for tr -d.
    
    Signed-off-by: Usman Akinyemi

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1805%2FUnique-Usman%2Favoid_git_pipes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1805/Unique-Usman/avoid_git_pipes-v2
Pull-Request: https://github.com/git/git/pull/1805

Range-diff vs v1:

 1:  5dd96eaf14c ! 1:  be5a691e96f [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'multi-squash only fires up e
      -	test 1 = $(git show | grep ONCE | wc -l)
      +	git show >output &&
      +	count=$(grep ONCE output | wc -l) &&
     -+	test 1 = "$count"
     ++	test 1 = $count
       '
       
       test_expect_success 'multi-fixup does not fire up editor' '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'multi-fixup does not fire up
      -	test 0 = $(git show | grep NEVER | wc -l) &&
      +	git show >output &&
      +	count=$(grep NEVER output | wc -l) &&
     -+	test 0 = "$count" &&
     ++	test 0 = $count &&
       	git checkout @{-1} &&
       	git branch -D multi-fixup
       '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'commit message used after co
      -	test 1 = $(git show | grep ONCE | wc -l) &&
      +	git show >output &&
      +	count=$(grep ONCE output | wc -l) &&
     -+	test 1 = "$count" &&
     ++	test 1 = $count &&
       	git checkout @{-1} &&
       	git branch -D conflict-fixup
       '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'commit message retained afte
      -	test 2 = $(git show | grep TWICE | wc -l) &&
      +	git show >output &&
      +	count=$(grep TWICE output | wc -l) &&
     -+	test 2 = "$count" &&
     ++	test 2 = $count &&
       	git checkout @{-1} &&
       	git branch -D conflict-squash
       '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'squash and fixup generate co
      -	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 &&
     ++	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  &&
     ++	grep "^# This is a combination of 2 commits\." actual  &&
       	git checkout @{-1} &&
       	git branch -D squash-fixup
       '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'squash ignores comments' '
      -	test 1 = $(git show | grep ONCE | wc -l) &&
      +	git show >output &&
      +	count=$(grep ONCE output | wc -l) &&
     -+	test 1 = "$count" &&
     ++	test 1 = $count &&
       	git checkout @{-1} &&
       	git branch -D skip-comments
       '
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'squash ignores blank lines'
      -	test 1 = $(git show | grep ONCE | wc -l) &&
      +	git show >output &&
      +	count=$(grep ONCE output | wc -l) &&
     -+	test 1 = "$count" &&
     ++	test 1 = $count &&
       	git checkout @{-1} &&
       	git branch -D skip-blank-lines
       '
 2:  4199434bd6e < -:  ----------- [Outreachy][Patch v2] t3404: avoid losing exit status to pipes


 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)


base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1

Comments

Eric Sunshine Oct. 6, 2024, 9:19 a.m. UTC | #1
On Sun, Oct 6, 2024 at 4:31 AM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>     After submitting the first patch, I noticed that the output of wc -l was
>     failing due to trailing whitespace. I attempted to fix this by using tr
>     -d to remove the whitespace. However, instead of squashing the two
>     patches into one, I inadvertently created another commit.
>
>     Eric Sunshine sunshine@sunshineco.com provided valuable feedback during
>     the review process. He explained the details of the patches to me and
>     pointed out that using tr -d was unnecessary to resolve the whitespace
>     issue.

Thanks. This version of the patch looks fine.

I notice that there are still quite a few instances of `git` upstream
of a pipe remaining in t3404 even after this patch. But that's okay;
fixing all of them would have made the patch far longer and more
tiresome to review, so it is not a problem that you selectively
converted only `git show` and `git cat-file` cases. It probably would
have been helpful to reviewers if the patch's commit message mentioned
that it only converts some of the instances, but it's not worth
rerolling the patch just for that.

So, I think it makes sense to stop here and consider this microproject
successful (unless some other reviewer notices some problem or
requests some other change).
Usman Akinyemi Oct. 6, 2024, 9:32 a.m. UTC | #2
On Sun, Oct 6, 2024 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 4:31 AM Usman Akinyemi via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > 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.
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >     After submitting the first patch, I noticed that the output of wc -l was
> >     failing due to trailing whitespace. I attempted to fix this by using tr
> >     -d to remove the whitespace. However, instead of squashing the two
> >     patches into one, I inadvertently created another commit.
> >
> >     Eric Sunshine sunshine@sunshineco.com provided valuable feedback during
> >     the review process. He explained the details of the patches to me and
> >     pointed out that using tr -d was unnecessary to resolve the whitespace
> >     issue.
>
> Thanks. This version of the patch looks fine.
>
> I notice that there are still quite a few instances of `git` upstream
> of a pipe remaining in t3404 even after this patch. But that's okay;
> fixing all of them would have made the patch far longer and more
> tiresome to review, so it is not a problem that you selectively
> converted only `git show` and `git cat-file` cases. It probably would
> have been helpful to reviewers if the patch's commit message mentioned
> that it only converts some of the instances, but it's not worth
> rerolling the patch just for that.
>
> So, I think it makes sense to stop here and consider this microproject
> successful (unless some other reviewer notices some problem or
> requests some other change).

Thanks Eric Sunshine, I appreciate your time and review. I am more
eager to continue working on it after review from the others. And also
would like to work on the test_line_count also if permitted. Thank you
very much.
Eric Sunshine Oct. 6, 2024, 10:21 a.m. UTC | #3
On Sun, Oct 6, 2024 at 5:32 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
> On Sun, Oct 6, 2024 at 9:19 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > So, I think it makes sense to stop here and consider this microproject
> > successful (unless some other reviewer notices some problem or
> > requests some other change).
>
> Thanks Eric Sunshine, I appreciate your time and review. I am more
> eager to continue working on it after review from the others. And also
> would like to work on the test_line_count also if permitted. Thank you
> very much.

You don't have to ask permission; if you feel like converting the test
to use test_line_count(), you are welcome to do so.
shejialuo Oct. 6, 2024, 11:12 a.m. UTC | #4
On Sun, Oct 06, 2024 at 05:19:13AM -0400, Eric Sunshine wrote:

[snip]

> It probably would have been helpful to reviewers if the patch's
> commit message mentioned that it only converts some of the
> instances, but it's not worth rerolling the patch just for that.

Except that, the commit title should not either include
"[Outreachy][Patch v1]" here. From these two reasons, I think we should
reroll the patch.

Thanks,
Jialuo
Eric Sunshine Oct. 6, 2024, 12:06 p.m. UTC | #5
On Sun, Oct 6, 2024 at 7:12 AM shejialuo <shejialuo@gmail.com> wrote:
> On Sun, Oct 06, 2024 at 05:19:13AM -0400, Eric Sunshine wrote:
> > It probably would have been helpful to reviewers if the patch's
> > commit message mentioned that it only converts some of the
> > instances, but it's not worth rerolling the patch just for that.
>
> Except that, the commit title should not either include
> "[Outreachy][Patch v1]" here. From these two reasons, I think we should
> reroll the patch.

Your observation about outdated/confusing "[foo]" annotations is
certainly something the submitter should take into consideration for
future submissions, but does not seem worthy of a reroll, IMHO. First,
`git am` will strip those off automatically, so they won't become part
of the permanent project history anyhow when/if Junio picks up the
patch. Second, asking for a reroll for something which does not impact
the correctness of either the patch or the commit message just makes
busy-work for the submitter and wastes reviewer time (which is a
limited resource on this project). Third, the point of a microproject
is to expose the submitter to the workflow of the Git project and to
the review process, and for reviewers to see how the submitter
responds. That goal has already been achieved in this case, and
rerolling for something so minor provides no additional benefit in
that regard.
Usman Akinyemi Oct. 6, 2024, 12:28 p.m. UTC | #6
Hello,

I really appreciate all the maintainers for their help and time.

I am a bit confused now, I am already working on converting the
test_line_count right now. Can I update the patch or do something
regarding the first patch ?

Thank you.

On Sun, Oct 6, 2024 at 12:06 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 7:12 AM shejialuo <shejialuo@gmail.com> wrote:
> > On Sun, Oct 06, 2024 at 05:19:13AM -0400, Eric Sunshine wrote:
> > > It probably would have been helpful to reviewers if the patch's
> > > commit message mentioned that it only converts some of the
> > > instances, but it's not worth rerolling the patch just for that.
> >
> > Except that, the commit title should not either include
> > "[Outreachy][Patch v1]" here. From these two reasons, I think we should
> > reroll the patch.
>
> Your observation about outdated/confusing "[foo]" annotations is
> certainly something the submitter should take into consideration for
> future submissions, but does not seem worthy of a reroll, IMHO. First,
> `git am` will strip those off automatically, so they won't become part
> of the permanent project history anyhow when/if Junio picks up the
> patch. Second, asking for a reroll for something which does not impact
> the correctness of either the patch or the commit message just makes
> busy-work for the submitter and wastes reviewer time (which is a
> limited resource on this project). Third, the point of a microproject
> is to expose the submitter to the workflow of the Git project and to
> the review process, and for reviewers to see how the submitter
> responds. That goal has already been achieved in this case, and
> rerolling for something so minor provides no additional benefit in
> that regard.
shejialuo Oct. 6, 2024, 12:29 p.m. UTC | #7
On Sun, Oct 06, 2024 at 08:06:10AM -0400, Eric Sunshine wrote:

> Your observation about outdated/confusing "[foo]" annotations is
> certainly something the submitter should take into consideration for
> future submissions, but does not seem worthy of a reroll, IMHO. First,
> `git am` will strip those off automatically, so they won't become part
> of the permanent project history anyhow when/if Junio picks up the
> patch. Second, asking for a reroll for something which does not impact
> the correctness of either the patch or the commit message just makes
> busy-work for the submitter and wastes reviewer time (which is a
> limited resource on this project). Third, the point of a microproject
> is to expose the submitter to the workflow of the Git project and to
> the review process, and for reviewers to see how the submitter
> responds. That goal has already been achieved in this case, and
> rerolling for something so minor provides no additional benefit in
> that regard.

Thanks for your detailed explanation here. I don't know that "git am"
could strip those off automatically. I thought the maintainer would
delete "[foo]" manually. So, my main intention here is that I want the
submitter to make it more perfect to reduce the overhead of the
maintainer and also pay attention to this for further submissions.

And from my perspective, the reroll would not bring much overhead for
the submitter, so I expressed my words in the previous email. I know you
concerned that my words would frustrate the Usman. And I wanna say this
is not my intention here. I think Usamn has already done a great job for
this microproject to understand the workflow of the Git project. So,
actually we are on the same boat here.

Let me withdraw my previous words ("We should reroll the patch"). This
patch is good and don't need a reroll.

Thanks,
Jialuo
Usman Akinyemi Oct. 6, 2024, 12:46 p.m. UTC | #8
Thanks Shejialuo and Eric, I really appreciate both of your time and
help. I am not frustrated at all and I see any changes as a perfect
opportunity to learn something new. I have also learned from my
mistake and your guidance and would keep it in mind for future
submissions.

In that case, I will send the second patch.
Thank you.

On Sun, Oct 6, 2024 at 12:29 PM shejialuo <shejialuo@gmail.com> wrote:
>
> On Sun, Oct 06, 2024 at 08:06:10AM -0400, Eric Sunshine wrote:
>
> > Your observation about outdated/confusing "[foo]" annotations is
> > certainly something the submitter should take into consideration for
> > future submissions, but does not seem worthy of a reroll, IMHO. First,
> > `git am` will strip those off automatically, so they won't become part
> > of the permanent project history anyhow when/if Junio picks up the
> > patch. Second, asking for a reroll for something which does not impact
> > the correctness of either the patch or the commit message just makes
> > busy-work for the submitter and wastes reviewer time (which is a
> > limited resource on this project). Third, the point of a microproject
> > is to expose the submitter to the workflow of the Git project and to
> > the review process, and for reviewers to see how the submitter
> > responds. That goal has already been achieved in this case, and
> > rerolling for something so minor provides no additional benefit in
> > that regard.
>
> Thanks for your detailed explanation here. I don't know that "git am"
> could strip those off automatically. I thought the maintainer would
> delete "[foo]" manually. So, my main intention here is that I want the
> submitter to make it more perfect to reduce the overhead of the
> maintainer and also pay attention to this for further submissions.
>
> And from my perspective, the reroll would not bring much overhead for
> the submitter, so I expressed my words in the previous email. I know you
> concerned that my words would frustrate the Usman. And I wanna say this
> is not my intention here. I think Usamn has already done a great job for
> this microproject to understand the workflow of the Git project. So,
> actually we are on the same boat here.
>
> Let me withdraw my previous words ("We should reroll the patch"). This
> patch is good and don't need a reroll.
>
> Thanks,
> Jialuo
shejialuo Oct. 6, 2024, 1:03 p.m. UTC | #9
On Sun, Oct 06, 2024 at 12:28:26PM +0000, Usman Akinyemi wrote:
> I am a bit confused now, I am already working on converting the
> test_line_count right now. Can I update the patch or do something
> regarding the first patch ?

Hi Usman:

I have just scanned the review from Eric.

> These days, instead of manually using `wc -l` and `test`, we would
> instead write:

>    grep ONCE output >actual &&
>    test_line_count 1 actual

> However, that sort of change is independent of the purpose of this
> patch, so you probably should not make such a change in this patch. If
> you're up to it, you could instead turn this into a two-patch series
> in which patch [1/2] fixes the "Git upstream of a pipe" problem, and
> then patch [2/2] converts these cases to use test_line_count().

If you decide to do this. As Eric has commented in [1], you should add a
new commit (a new patch) followed by current patch to convert to the
`test_line_count`. Then you should re-send the series to the mailing
list. And thus you could enhance the commit message of the first patch.
If you do not decide to do this (the current patch is enough for the
microproject), you don't need to reroll for the minor things.

So, you should never update the current patch for converting the test
using `test_line_count`. Instead, create a new commit to do this. and
BTW you could change the commit message of the first patch if you want.

Thanks,
Jialuo

[1] https://lore.kernel.org/git/CAPig+cTb4mgpXnN79UrXvjvCnqGZhaR51oZX_Ds=HwdqQYFN9w@mail.gmail.com/
Usman Akinyemi Oct. 6, 2024, 1:27 p.m. UTC | #10
Thanks very much Jialuo,

My understanding of this, kindly tell me if I am wrong.

1. Make a new commit for test_line_count on the same branch right ?
2. I should remove the "Outreachy][Patch v1]" in the first patch to
enhance the commit message right?

Thank you.

On Sun, Oct 6, 2024 at 1:02 PM shejialuo <shejialuo@gmail.com> wrote:
>
> On Sun, Oct 06, 2024 at 12:28:26PM +0000, Usman Akinyemi wrote:
> > I am a bit confused now, I am already working on converting the
> > test_line_count right now. Can I update the patch or do something
> > regarding the first patch ?
>
> Hi Usman:
>
> I have just scanned the review from Eric.
>
> > These days, instead of manually using `wc -l` and `test`, we would
> > instead write:
>
> >    grep ONCE output >actual &&
> >    test_line_count 1 actual
>
> > However, that sort of change is independent of the purpose of this
> > patch, so you probably should not make such a change in this patch. If
> > you're up to it, you could instead turn this into a two-patch series
> > in which patch [1/2] fixes the "Git upstream of a pipe" problem, and
> > then patch [2/2] converts these cases to use test_line_count().
>
> If you decide to do this. As Eric has commented in [1], you should add a
> new commit (a new patch) followed by current patch to convert to the
> `test_line_count`. Then you should re-send the series to the mailing
> list. And thus you could enhance the commit message of the first patch.
> If you do not decide to do this (the current patch is enough for the
> microproject), you don't need to reroll for the minor things.
>
> So, you should never update the current patch for converting the test
> using `test_line_count`. Instead, create a new commit to do this. and
> BTW you could change the commit message of the first patch if you want.
>
> Thanks,
> Jialuo
>
> [1] https://lore.kernel.org/git/CAPig+cTb4mgpXnN79UrXvjvCnqGZhaR51oZX_Ds=HwdqQYFN9w@mail.gmail.com/
shejialuo Oct. 6, 2024, 1:36 p.m. UTC | #11
On Sun, Oct 06, 2024 at 01:27:00PM +0000, Usman Akinyemi wrote:
> Thanks very much Jialuo,
> 
> My understanding of this, kindly tell me if I am wrong.
> 
> 1. Make a new commit for test_line_count on the same branch right ?
> 2. I should remove the "Outreachy][Patch v1]" in the first patch to
> enhance the commit message right?
> 

For 1, it is correct. For 2, if you decide to change the commit message
of the first patch, you should also follow the advice from Eric in [1].

> It probably would have been helpful to reviewers if the patch's
> commit message mentioned that it only converts some of the
> instances, but it's not worth rerolling the patch just for that.

[1] https://lore.kernel.org/git/CAPig+cRePbX6OyE6WhFp1GEZenZyC7HFeA3188F_nErt7Gin6A@mail.gmail.com/
diff mbox series

Patch

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
 '
 
 test_expect_success 'retain authorship w/ conflicts' '
@@ -360,7 +361,8 @@  test_expect_success 'squash' '
 '
 
 test_expect_success 'retain authorship when squashing' '
-	git show HEAD | grep "^Author: Twerp Snog"
+	git show HEAD >actual &&
+	grep "^Author: Twerp Snog" actual
 '
 
 test_expect_success '--continue tries to commit' '
@@ -374,7 +376,8 @@  test_expect_success '--continue tries to commit' '
 		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
 	) &&
 	test_cmp_rev HEAD^ new-branch1 &&
-	git show HEAD | grep chouette
+	git show HEAD >actual &&
+	grep chouette actual
 '
 
 test_expect_success 'verbose flag is heeded, even after --continue' '
@@ -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' '
@@ -410,7 +415,9 @@  test_expect_success 'multi-fixup does not fire up editor' '
 			git rebase -i $base
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 0 = $(git show | grep NEVER | wc -l) &&
+	git show >output &&
+	count=$(grep NEVER output | wc -l) &&
+	test 0 = $count &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -428,7 +435,9 @@  test_expect_success 'commit message used after conflict' '
 			git rebase --continue
 	) &&
 	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 &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -446,7 +455,9 @@  test_expect_success 'commit message retained after conflict' '
 			git rebase --continue
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
-	test 2 = $(git show | grep TWICE | wc -l) &&
+	git show >output &&
+	count=$(grep TWICE output | wc -l) &&
+	test 2 = $count &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -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
 '
@@ -489,7 +500,9 @@  test_expect_success 'squash ignores comments' '
 			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 &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -505,7 +518,9 @@  test_expect_success 'squash ignores blank lines' '
 			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 &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
@@ -572,7 +587,8 @@  test_expect_success '--continue tries to commit, even for "edit"' '
 		FAKE_COMMIT_MESSAGE="chouette!" git rebase --continue
 	) &&
 	test edited = $(git show HEAD:file7) &&
-	git show HEAD | grep chouette &&
+	git show HEAD >actual &&
+	grep chouette actual &&
 	test $parent = $(git rev-parse HEAD^)
 '
 
@@ -757,19 +773,23 @@  test_expect_success 'reword' '
 		set_fake_editor &&
 		FAKE_LINES="1 2 3 reword 4" FAKE_COMMIT_MESSAGE="E changed" \
 			git rebase -i A &&
-		git show HEAD | grep "E changed" &&
+		git show HEAD >actual &&
+		grep "E changed" actual &&
 		test $(git rev-parse primary) != $(git rev-parse HEAD) &&
 		test_cmp_rev primary^ HEAD^ &&
 		FAKE_LINES="1 2 reword 3 4" FAKE_COMMIT_MESSAGE="D changed" \
 			git rebase -i A &&
-		git show HEAD^ | grep "D changed" &&
+		git show HEAD^ >actual &&
+		grep "D changed" actual &&
 		FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" \
 			git rebase -i A &&
-		git show HEAD~3 | grep "B changed" &&
+		git show HEAD~3 >actual &&
+		grep "B changed" actual &&
 		FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" \
 			git rebase -i A
 	) &&
-	git show HEAD~2 | grep "C changed"
+	git show HEAD~2 >actual &&
+	grep "C changed" actual
 '
 
 test_expect_success 'no uncommitted changes when rewording and the todo list is reloaded' '
@@ -1003,8 +1023,10 @@  test_expect_success 'rebase -i --root retain root commit author and message' '
 		set_fake_editor &&
 		FAKE_LINES="2" git rebase -i --root
 	) &&
-	git cat-file commit HEAD | grep -q "^author Twerp Snog" &&
-	git cat-file commit HEAD | grep -q "^different author$"
+	git cat-file commit HEAD >output &&
+	grep -q "^author Twerp Snog" output &&
+	git cat-file commit HEAD >actual &&
+	grep -q "^different author$" actual
 '
 
 test_expect_success 'rebase -i --root temporary sentinel commit' '
@@ -1013,7 +1035,8 @@  test_expect_success 'rebase -i --root temporary sentinel commit' '
 		set_fake_editor &&
 		test_must_fail env FAKE_LINES="2" git rebase -i --root
 	) &&
-	git cat-file commit HEAD | grep "^tree $EMPTY_TREE" &&
+	git cat-file commit HEAD >actual &&
+	grep "^tree $EMPTY_TREE" actual &&
 	git rebase --abort
 '
 
@@ -1036,7 +1059,8 @@  test_expect_success 'rebase -i --root reword original root commit' '
 		FAKE_LINES="reword 1 2" FAKE_COMMIT_MESSAGE="A changed" \
 			git rebase -i --root
 	) &&
-	git show HEAD^ | grep "A changed" &&
+	git show HEAD^ >actual &&
+	grep "A changed" actual &&
 	test -z "$(git show -s --format=%p HEAD^)"
 '
 
@@ -1048,7 +1072,8 @@  test_expect_success 'rebase -i --root reword new root commit' '
 		FAKE_LINES="reword 3 1" FAKE_COMMIT_MESSAGE="C changed" \
 		git rebase -i --root
 	) &&
-	git show HEAD^ | grep "C changed" &&
+	git show HEAD^ >actual &&
+	grep "C changed" actual &&
 	test -z "$(git show -s --format=%p HEAD^)"
 '