[PATCH 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[PATCH 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

Changes since v1:

 * Added "tr -d '[:space:]'" to handle whitespace on macOS

Signed-off-by: Usman Akinyemi usmanakinyemi202@gmail.com

Usman Akinyemi (2):
  [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
  [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
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1805%2FUnique-Usman%2Favoid_git_pipes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1805/Unique-Usman/avoid_git_pipes-v1
Pull-Request: https://github.com/git/git/pull/1805
-- 
gitgitgadget

[PATCH 1/2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

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>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f171af3061d..33ea1f05e2c 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^)"
 '
 
-- 
gitgitgadget

[PATCH 2/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Changes since v1:
- Added "tr -d '[:space:]'" to handle whitespace on macOS

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 33ea1f05e2c..3d8de69d6ee 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -401,7 +401,7 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
+	count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
 	test 1 = "$count"
 '
 
@@ -416,7 +416,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
+	count=$(grep NEVER output | wc -l | tr -d '[:space:]') &&
 	test 0 = "$count" &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
@@ -436,7 +436,7 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
+	count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
 	test 1 = "$count" &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
@@ -456,7 +456,7 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
+	count=$(grep TWICE output | wc -l | tr -d '[:space:]') &&
 	test 2 = "$count" &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
@@ -501,7 +501,7 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
+	count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
 	test 1 = "$count" &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
@@ -519,7 +519,7 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
+	count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
 	test 1 = "$count" &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
-- 
gitgitgadget

Re: [PATCH 2/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Sun, Oct 6, 2024 at 1:33 AM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Changes since v1:
> - Added "tr -d '[:space:]'" to handle whitespace on macOS
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---

Thanks for the submission. A few comments...

This second patch fixes problems with the first patch, but since this
is an entirely new submission, you should instead "squash" these two
patches together and then force-push them to the same branch that you
used when submitting them via GitGitGadget, and re-submit them as a
single patch. When you squash them, keep the commit message from the
first patch.

Reviewers do appreciate that you explained what changed since the
previous version, but we'd like to see that information as commentary
in the patch cover letter, not as the commit message of the patch
itself. In GitGitGadget, the way you would do so is to write this as
the "Description" of the pull-request (possibly replacing or amending
the previous description).

Some more observations below...

> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -401,7 +401,7 @@ test_expect_success 'multi-squash only fires up editor once' '
>         git show >output &&
> -       count=$(grep ONCE output | wc -l) &&
> +       count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
>         test 1 = "$count"
>  '

The reason this was failing for you was because you quoted $count. Had
you instead written:

    test 1 = $count

when it would have worked as expected. In other words, you don't need `tr`.

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().

Re: [PATCH 1/2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Sun, Oct 6, 2024 at 1:33 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.

Okay, makes sense.

One minor style comment below...

> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
> -       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  &&

We wouldn't normally indent the `grep` line like this. Now that you've
dropped the patch, it would be best to lose the extra indentation, as
well:

    git cat-file commit HEAD@{2} >actual &&
    grep "^# This is a combination of 3 commits\." actual &&
    ...

Re: [PATCH 2/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 1:33 AM Usman Akinyemi via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Changes since v1:
> > - Added "tr -d '[:space:]'" to handle whitespace on macOS
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
>
> Thanks for the submission. A few comments...
>
> This second patch fixes problems with the first patch, but since this
> is an entirely new submission, you should instead "squash" these two
> patches together and then force-push them to the same branch that you
> used when submitting them via GitGitGadget, and re-submit them as a
> single patch. When you squash them, keep the commit message from the
> first patch.
>
> Reviewers do appreciate that you explained what changed since the
> previous version, but we'd like to see that information as commentary
> in the patch cover letter, not as the commit message of the patch
> itself. In GitGitGadget, the way you would do so is to write this as
> the "Description" of the pull-request (possibly replacing or amending
> the previous description).
>
> Some more observations below...
>
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > @@ -401,7 +401,7 @@ test_expect_success 'multi-squash only fires up editor once' '
> >         git show >output &&
> > -       count=$(grep ONCE output | wc -l) &&
> > +       count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
> >         test 1 = "$count"
> >  '
>
> The reason this was failing for you was because you quoted $count. Had
> you instead written:
>
>     test 1 = $count
>
> when it would have worked as expected. In other words, you don't need `tr`.
>
> 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().

Hi  Eric Sunshine,
thanks for the review. I really appreciate it. I have a couple of
doubts to clear.

My next patch should be the squash of my three patches which include
my first two patches and the new one on the same branch ?
Two patch series means two different commits on different patches ?
But, since they both depend on each other would not they lead to merge
conflict ?
Also, to be clear, "Description" is the body of the commit message if
I use the gitgitgadget while the "commit message" is the header ?

Thank you.

Re: [PATCH 1/2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Sun, Oct 6, 2024 at 5:55 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 1:33 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.
>
> Okay, makes sense.
>
> One minor style comment below...
>
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > @@ -470,10 +481,10 @@ test_expect_success 'squash and fixup generate correct log messages' '
> > -       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  &&
>
> We wouldn't normally indent the `grep` line like this. Now that you've
> dropped the patch, it would be best to lose the extra indentation, as
> well:
>
>     git cat-file commit HEAD@{2} >actual &&
>     grep "^# This is a combination of 3 commits\." actual &&
>     ...

Noted and thanks.

Re: [PATCH 2/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Sun, Oct 6, 2024 at 2:31 AM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
> On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > This second patch fixes problems with the first patch, but since this
> > is an entirely new submission, you should instead "squash" these two
> > patches together and then force-push them to the same branch that you
> > used when submitting them via GitGitGadget, and re-submit them as a
> > single patch. When you squash them, keep the commit message from the
> > first patch.
> >
> > Reviewers do appreciate that you explained what changed since the
> > previous version, but we'd like to see that information as commentary
> > in the patch cover letter, not as the commit message of the patch
> > itself. In GitGitGadget, the way you would do so is to write this as
> > the "Description" of the pull-request (possibly replacing or amending
> > the previous description).
> >
> > 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().
>
> thanks for the review. I really appreciate it. I have a couple of
> doubts to clear.
>
> My next patch should be the squash of my three patches which include
> my first two patches and the new one on the same branch ?

I'm not sure which three patches you mean. Does the third patch, the
"new one on the same branch", fix problems from the earlier two
patches? Or does your third patch implement the suggestion regarding
test_line_count()?

> Two patch series means two different commits on different patches ?
> But, since they both depend on each other would not they lead to merge
> conflict ?

A patch series consists of one or more patches in sequence. Patches
within a series don't conflict with one another; later patches build
upon earlier patches. You create a series of commits on a single Git
branch. When you submit that branch as a pull-request via
GitGitGadget, it turns the commits on that branch into a series of
patch emails to the Git mailing list, one per commit.

Before submitting the patches via GitGitGadget, you polish them
locally to repair any problems before submitting them for review.
Rather than making subsequent commits fix problems with earlier
commits, you instead should fix the bad commits by using "git rebase
--interactive ..." to either "fixup", "squash", or otherwise edit the
bad commits. Once you are happy with the commits, submit them. This
way, reviewers only see your polished result, not the series of steps
you made to arrive at the polished results.

You do the same thing after reviewers point out problems or ask for
changes. Edit and re-polish the existing commits to address reviewer
comments rather than merely making new commits on top of the existing
commits, and then resubmit once all the fixes have been applied and
polished.

When I suggested squashing your two original commits it was for the
above reason. In your original submission, patch [1/2] had some
problems which you fixed in patch [2/2], but reviewers don't need or
want to see that; they just want to see the polished end-result, which
you can obtain by squashing the two patches together. (However, in
this case, as I pointed out in my review, you don't even need to use
`tr`; just use `test 1 = $count` instead of `test 1 = "$count"`.)

If you wanted to do the extra step of also updating the tests to use
test_line_count(), then that would be a separate patch, still on the
same branch, built on top of your "fix Git upstream of pipe" patch.
Thus, it would become a two-patch series: patch [1/2] fixing Git
upstream of a pipe, and [2/2] employing test_line_count().

> Also, to be clear, "Description" is the body of the commit message if
> I use the gitgitgadget while the "commit message" is the header ?

The commit message is separate from the patch-series commentary. The
commit message of each patch explains what that patch changes or does.

Once you have polished your commit(s), force-push them to the
GitGitGadget pull-request you already created. Then edit the very
first (topmost) comment in the pull-request to explain what the patch
series is about and what you changed since the previous version. That
comment becomes the "commentary" portion of the patch series.

Re: [PATCH 2/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Sun, Oct 6, 2024 at 7:28 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 2:31 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> > On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > This second patch fixes problems with the first patch, but since this
> > > is an entirely new submission, you should instead "squash" these two
> > > patches together and then force-push them to the same branch that you
> > > used when submitting them via GitGitGadget, and re-submit them as a
> > > single patch. When you squash them, keep the commit message from the
> > > first patch.
> > >
> > > Reviewers do appreciate that you explained what changed since the
> > > previous version, but we'd like to see that information as commentary
> > > in the patch cover letter, not as the commit message of the patch
> > > itself. In GitGitGadget, the way you would do so is to write this as
> > > the "Description" of the pull-request (possibly replacing or amending
> > > the previous description).
> > >
> > > 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().
> >
> > thanks for the review. I really appreciate it. I have a couple of
> > doubts to clear.
> >
> > My next patch should be the squash of my three patches which include
> > my first two patches and the new one on the same branch ?
>
> I'm not sure which three patches you mean. Does the third patch, the
> "new one on the same branch", fix problems from the earlier two
> patches? Or does your third patch implement the suggestion regarding
> test_line_count()?
>
Thanks, I really appreciate your explanation. Thank you very much. By
the third patch, I meant the new one which I will be adding.

> > Two patch series means two different commits on different patches ?
> > But, since they both depend on each other would not they lead to merge
> > conflict ?
>
> A patch series consists of one or more patches in sequence. Patches
> within a series don't conflict with one another; later patches build
> upon earlier patches. You create a series of commits on a single Git
> branch. When you submit that branch as a pull-request via
> GitGitGadget, it turns the commits on that branch into a series of
> patch emails to the Git mailing list, one per commit.
>
> Before submitting the patches via GitGitGadget, you polish them
> locally to repair any problems before submitting them for review.
> Rather than making subsequent commits fix problems with earlier
> commits, you instead should fix the bad commits by using "git rebase
> --interactive ..." to either "fixup", "squash", or otherwise edit the
> bad commits. Once you are happy with the commits, submit them. This
> way, reviewers only see your polished result, not the series of steps
> you made to arrive at the polished results.
>
> You do the same thing after reviewers point out problems or ask for
> changes. Edit and re-polish the existing commits to address reviewer
> comments rather than merely making new commits on top of the existing
> commits, and then resubmit once all the fixes have been applied and
> polished.
>
> When I suggested squashing your two original commits it was for the
> above reason. In your original submission, patch [1/2] had some
> problems which you fixed in patch [2/2], but reviewers don't need or
> want to see that; they just want to see the polished end-result, which
> you can obtain by squashing the two patches together. (However, in
> this case, as I pointed out in my review, you don't even need to use
> `tr`; just use `test 1 = $count` instead of `test 1 = "$count"`.)
>
> If you wanted to do the extra step of also updating the tests to use
> test_line_count(), then that would be a separate patch, still on the
> same branch, built on top of your "fix Git upstream of pipe" patch.
> Thus, it would become a two-patch series: patch [1/2] fixing Git
> upstream of a pipe, and [2/2] employing test_line_count().
>
Thanks for this. I will submit the first patch, get feedback then
approach the second one.
> > Also, to be clear, "Description" is the body of the commit message if
> > I use the gitgitgadget while the "commit message" is the header ?
>
> The commit message is separate from the patch-series commentary. The
> commit message of each patch explains what that patch changes or does.
>
> Once you have polished your commit(s), force-push them to the
> GitGitGadget pull-request you already created. Then edit the very
> first (topmost) comment in the pull-request to explain what the patch
> series is about and what you changed since the previous version. That
> comment becomes the "commentary" portion of the patch series.

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

[Usman Akinyemi via GitGitGadget]

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(-)

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^)"
 '
 

base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1
-- 
gitgitgadget

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Eric Sunshine]

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).

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

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.

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Eric Sunshine]

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.

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[shejialuo]

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

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Eric Sunshine]

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.

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

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.

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[shejialuo]

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

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

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

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[shejialuo]

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/

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

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/

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[shejialuo]

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/

[PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

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

Usman Akinyemi (2):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-files`
  [Outreachy][Patch v1] t3404: employing test_line_count() to replace
    test

 t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 24 deletions(-)


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

Range-diff vs v2:

 1:  be5a691e96f ! 1:  c9a0cca179b [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
     @@ Metadata
      Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## Commit message ##
     -    [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
     +    t3404: avoid losing exit status with focus on `git show` and `git cat-files`
      
          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.
     +    command fails. This particular patch focuses on some of the instances
     +    which include `git show` and `git cat-files`.
      
          Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
 -:  ----------- > 2:  37b1411ee2c [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

-- 
gitgitgadget

[PATCH v3 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-files`

[Usman Akinyemi via GitGitGadget]

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. This particular patch focuses on some of the instances
which include `git show` and `git cat-files`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

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^)"
 '
 
-- 
gitgitgadget

[PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..19390eaf331 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
+	grep -v "^#" < .git/rebase-merge/done > actual &&
+	test_line_count = 4 actual &&
 	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
 '
 
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -436,8 +437,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +457,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +502,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +520,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

Hello,

Kindly, help take a look if this is okay now.

Also, I wanted to change this also to use test_line_count,
 test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)

 But, I tried a different approach and the test kept failing.

Similar as

git show >output &&
count=$(grep NEVER output | wc -l) &&
test 0 = $count &&

Thank you.

On Sun, Oct 6, 2024 at 4:06 PM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> 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
>
> Usman Akinyemi (2):
>   t3404: avoid losing exit status with focus on `git show` and `git
>     cat-files`
>   [Outreachy][Patch v1] t3404: employing test_line_count() to replace
>     test
>
>  t/t3404-rebase-interactive.sh | 74 +++++++++++++++++++++++------------
>  1 file changed, 50 insertions(+), 24 deletions(-)
>
>
> base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1805%2FUnique-Usman%2Favoid_git_pipes-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1805/Unique-Usman/avoid_git_pipes-v3
> Pull-Request: https://github.com/git/git/pull/1805
>
> Range-diff vs v2:
>
>  1:  be5a691e96f ! 1:  c9a0cca179b [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
>      @@ Metadata
>       Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
>        ## Commit message ##
>      -    [Outreachy][Patch v1] t3404: avoid losing exit status to pipes
>      +    t3404: avoid losing exit status with focus on `git show` and `git cat-files`
>
>           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.
>      +    command fails. This particular patch focuses on some of the instances
>      +    which include `git show` and `git cat-files`.
>
>           Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
>  -:  ----------- > 2:  37b1411ee2c [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
>
> --
> gitgitgadget

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Kristoffer Haugsbakk]

On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote:
>
> 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
>
> Usman Akinyemi (2):
>   t3404: avoid losing exit status with focus on `git show` and `git
>     cat-files`
>   [Outreachy][Patch v1] t3404: employing test_line_count() to replace
>     test

You don’t have to sign off on your cover letter.  Just the patches. :)

Of course do what you prefer.

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

Thanks for this. I will definitely take note of this next time. Thank you.

On Sun, Oct 6, 2024 at 4:21 PM Kristoffer Haugsbakk
<kristofferhaugsbakk@fastmail.com> wrote:
>
> On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote:
> >
> > 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
> >
> > Usman Akinyemi (2):
> >   t3404: avoid losing exit status with focus on `git show` and `git
> >     cat-files`
> >   [Outreachy][Patch v1] t3404: employing test_line_count() to replace
> >     test
>
> You don’t have to sign off on your cover letter.  Just the patches. :)
>
> Of course do what you prefer.

Re: [PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Kristoffer Haugsbakk]

On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

Nice commit message.

-- 
Kristoffer Haugsbakk

Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Sun, Oct 6, 2024 at 8:29 AM 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.

Okay, that makes sense. Fortunately, the behavior of git-am means that
we don't have to worry about that particular issue.

> 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.

It's true that I try to be careful to avoid asking submitters to do
unnecessary work, but my bigger concern is that there are many patches
being submitted but very few people reviewing them, so it is a good
idea to avoid piling more work on reviewers if possible.

By the way, I appreciate that you are helping to review patches on
this list; not just this series, but also larger and more complicated
series such as the one for making git-worktree employ relative paths.

> 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.

Agreed.

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

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi
<usmanakinyemi202@gmail.com> wrote:
> Kindly, help take a look if this is okay now.
>
> Also, I wanted to change this also to use test_line_count,
>  test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
>
>  But, I tried a different approach and the test kept failing.
>
> Similar as
>
> git show >output &&
> count=$(grep NEVER output | wc -l) &&
> test 0 = $count &&

What is the actual error you encountered?

By the way, we have a handy function, test_must_be_empty(), which can
be used if you expect the output to not contain anything. As an
example:

    git show >output &&
    grep NEVER output >actual &&
    test_must_be_empty actual

Re: [PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Eric Sunshine]

On Sun, Oct 6, 2024 at 12:06 PM Usman Akinyemi via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
>
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> @@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
> -       test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> +       grep -v "^#" < .git/rebase-merge/done > actual &&
> +       test_line_count = 4 actual &&

Thanks. Both patches in this series look fine.

Let's consider this a successfully-completed microproject.

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Patrick Steinhardt]

On Sun, Oct 06, 2024 at 04:26:03PM +0000, Usman Akinyemi wrote:
> Thanks for this. I will definitely take note of this next time. Thank you.

Please note that we tend to not top-post on this mailing list. Instead,
your reply should be either inline in the quoted parts in case you want
to reply to individual parts of it, or below the quoted part as others
are doing :)

Patrick

> On Sun, Oct 6, 2024 at 4:21 PM Kristoffer Haugsbakk
> <kristofferhaugsbakk@fastmail.com> wrote:
> >
> > On Sun, Oct 6, 2024, at 18:06, Usman Akinyemi via GitGitGadget wrote:
> > >
> > > 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
> > >
> > > Usman Akinyemi (2):
> > >   t3404: avoid losing exit status with focus on `git show` and `git
> > >     cat-files`
> > >   [Outreachy][Patch v1] t3404: employing test_line_count() to replace
> > >     test
> >
> > You don’t have to sign off on your cover letter.  Just the patches. :)
> >
> > Of course do what you prefer.

Re: [PATCH v3 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-files`

[Patrick Steinhardt]

On Sun, Oct 06, 2024 at 04:06:08PM +0000, Usman Akinyemi via GitGitGadget wrote:
> 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. This particular patch focuses on some of the instances
> which include `git show` and `git cat-files`.

Well-described.

> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>  t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
>  1 file changed, 48 insertions(+), 23 deletions(-)
> 
> 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
>  '

One thing to note is that it would be preferable to use `test_grep`
instead of `grep` here. `test_grep` brings in some additional benefits
over plain `grep` like better diagnosis of issues, and it would also
print the file if things didn't match. That makes it way easier e.g. in
our CI to see what the actual output was.

There is no need to reroll this patch series just because of that
though, as your changes are a strict improvement by themselves already.
But if you do end up rerolling it would be nice to incorporate this.

> @@ -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' '

I was wondering whether the following might be nicer:

    git show >output &&
    grep ONCE output >output.filtered &&
    test_line_count = 1 output.filtered

But after seeing this I don't strongly lean into one or another
direction. So please feel free to use either your current or my proposed
style, I'm fine with either.

> @@ -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
>  '

Nice cleanups while at it.

Patrick

Re: [PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Patrick Steinhardt]

On Sun, Oct 06, 2024 at 04:06:09PM +0000, Usman Akinyemi via GitGitGadget wrote:
> @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
>  	) &&
>  	test $base = $(git rev-parse HEAD^) &&
>  	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual
>  '
>  
>  test_expect_success 'multi-fixup does not fire up editor' '

Oh, you already do the change I proposed on the first commit. It's a bit
funny that we first change things one way and then touch it up again in
another commit as it leaves the reviewer wondering for a bit.

But I guess that's okay, especially for a microproject. So overall I
don't see a strong reason to reroll this series, thanks!

Patrick

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 4:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> > Kindly, help take a look if this is okay now.
> >
> > Also, I wanted to change this also to use test_line_count,
> >  test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> >
> >  But, I tried a different approach and the test kept failing.
> >
> > Similar as
> >
> > git show >output &&
> > count=$(grep NEVER output | wc -l) &&
> > test 0 = $count &&
>
> What is the actual error you encountered?
>
> By the way, we have a handy function, test_must_be_empty(), which can
> be used if you expect the output to not contain anything. As an
> example:
>
>     git show >output &&
>     grep NEVER output >actual &&
>     test_must_be_empty actual

Thanks for your review, I really appreciate it. I tried this approach,
but I was getting this particular error for the testing.

not ok 32 - multi-fixup does not fire up editor
#
# git checkout -b multi-fixup E &&
# base=$(git rev-parse HEAD~4) &&
# (
# set_fake_editor &&
# FAKE_COMMIT_AMEND="NEVER" \
# FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
# git rebase -i $base
# ) &&
# test $base = $(git rev-parse HEAD^) &&
# git show >output &&
# grep NEVER output >actual &&
# test_must_be_empty actual &&
# git checkout @{-1} &&
# git branch -D multi-fixup
#
Below is the particular test case

test_expect_success 'multi-fixup does not fire up editor' '
git checkout -b multi-fixup E &&
base=$(git rev-parse HEAD~4) &&
(
set_fake_editor &&
FAKE_COMMIT_AMEND="NEVER" \
FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
git rebase -i $base
) &&
test $base = $(git rev-parse HEAD^) &&
git show >output &&
grep NEVER output >actual &&
test_must_be_empty actual &&
git checkout @{-1} &&
git branch -D multi-fixup
'

Re: [PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 6:05 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Sun, Oct 06, 2024 at 04:06:09PM +0000, Usman Akinyemi via GitGitGadget wrote:
> > @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual
> >  '
> >
> >  test_expect_success 'multi-fixup does not fire up editor' '
>
> Oh, you already do the change I proposed on the first commit. It's a bit
> funny that we first change things one way and then touch it up again in
> another commit as it leaves the reviewer wondering for a bit.
>
> But I guess that's okay, especially for a microproject. So overall I
> don't see a strong reason to reroll this series, thanks!
>
> Patrick

Thank you very much for the review. I really appreciate it. I had to
make this separate commit as recommended in the resources you provided
and reviews from other reviewers.

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Patrick Steinhardt]

On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote:
> On Mon, Oct 7, 2024 at 4:24 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Sun, Oct 6, 2024 at 12:18 PM Usman Akinyemi
> > <usmanakinyemi202@gmail.com> wrote:
> > > Kindly, help take a look if this is okay now.
> > >
> > > Also, I wanted to change this also to use test_line_count,
> > >  test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> > >
> > >  But, I tried a different approach and the test kept failing.
> > >
> > > Similar as
> > >
> > > git show >output &&
> > > count=$(grep NEVER output | wc -l) &&
> > > test 0 = $count &&
> >
> > What is the actual error you encountered?
> >
> > By the way, we have a handy function, test_must_be_empty(), which can
> > be used if you expect the output to not contain anything. As an
> > example:
> >
> >     git show >output &&
> >     grep NEVER output >actual &&
> >     test_must_be_empty actual
> 
> Thanks for your review, I really appreciate it. I tried this approach,
> but I was getting this particular error for the testing.
> 
> not ok 32 - multi-fixup does not fire up editor
> #
> # git checkout -b multi-fixup E &&
> # base=$(git rev-parse HEAD~4) &&
> # (
> # set_fake_editor &&
> # FAKE_COMMIT_AMEND="NEVER" \
> # FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
> # git rebase -i $base
> # ) &&
> # test $base = $(git rev-parse HEAD^) &&
> # git show >output &&
> # grep NEVER output >actual &&
> # test_must_be_empty actual &&
> # git checkout @{-1} &&
> # git branch -D multi-fixup
> #
> Below is the particular test case
> 
> test_expect_success 'multi-fixup does not fire up editor' '
> git checkout -b multi-fixup E &&
> base=$(git rev-parse HEAD~4) &&
> (
> set_fake_editor &&
> FAKE_COMMIT_AMEND="NEVER" \
> FAKE_LINES="1 fixup 2 fixup 3 fixup 4" \
> git rebase -i $base
> ) &&
> test $base = $(git rev-parse HEAD^) &&
> git show >output &&
> grep NEVER output >actual &&
> test_must_be_empty actual &&
> git checkout @{-1} &&
> git branch -D multi-fixup
> '

That makes sense. The expectation here is that `output` shouldn't
contain the string "NEVER" at all. And as grep(1) would fail when it
doesn't find a match the whole test would fail like this. So the below
would likely be the best solution.

Patrick

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index f171af3061..978fdfc2f1 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -410,7 +410,8 @@ 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 &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Eric Sunshine]

On Mon, Oct 7, 2024 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote:
> > test $base = $(git rev-parse HEAD^) &&
> > git show >output &&
> > grep NEVER output >actual &&
> > test_must_be_empty actual &&
>
> That makes sense. The expectation here is that `output` shouldn't
> contain the string "NEVER" at all. And as grep(1) would fail when it
> doesn't find a match the whole test would fail like this. So the below
> would likely be the best solution.

Thanks. I was just about to respond with the same answer. As a bit of
extra explanation, the &&-chaining means that every command in the
chain must return "success" (status 0), but the return code of `grep`
depends upon whether or not it matched any lines. In this case, it
returned non-zero which caused the test to fail.

Re: [PATCH v3 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-files`

[phillip.wood123]

Hi Usman

On 06/10/2024 17:06, Usman Akinyemi via GitGitGadget wrote:
> 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.

This is a good description of the reason for making this change.

> This particular patch focuses on some of the instances
> which include `git show` and `git cat-files`.

This patch seems to fix all of the instances of "git show" rather than 
some. It also fixes a few instances of "git cat-file" (note there is no 
trailing "s" in the command name). It is not immediately clear to me why 
those instances of "git cat-file" were selected for conversion but not 
others. However they are a worthwhile improvement and converting them 
all in this patch would make it bit too big to comfortably review so I'm 
happy with the changes as they are.

The patch itself looks good.

Thanks

Phillip

> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>   t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
>   1 file changed, 48 insertions(+), 23 deletions(-)
> 
> 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^)"
>   '
>   

Re: [PATCH v3 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[phillip.wood123]

Hi Patrick

On 07/10/2024 07:05, Patrick Steinhardt wrote:
> 
> Oh, you already do the change I proposed on the first commit. It's a bit
> funny that we first change things one way and then touch it up again in
> another commit as it leaves the reviewer wondering for a bit.
> 
> But I guess that's okay, especially for a microproject. So overall I
> don't see a strong reason to reroll this series, thanks!

It looks like Eric suggested making this change in a separate patch. 
While they could perhaps be made in the same patch fixing the pipelines 
and using test_line_count are essentially independent changes so I think 
splitting them into two patches is good practice and makes sense from 
pedagogical point of view.

Best Wishes

Phillip

Re: [PATCH v3 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 8:11 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Mon, Oct 7, 2024 at 4:08 AM Patrick Steinhardt <ps@pks.im> wrote:
> > On Mon, Oct 07, 2024 at 07:25:44AM +0000, Usman Akinyemi wrote:
> > > test $base = $(git rev-parse HEAD^) &&
> > > git show >output &&
> > > grep NEVER output >actual &&
> > > test_must_be_empty actual &&
> >
> > That makes sense. The expectation here is that `output` shouldn't
> > contain the string "NEVER" at all. And as grep(1) would fail when it
> > doesn't find a match the whole test would fail like this. So the below
> > would likely be the best solution.
>
> Thanks. I was just about to respond with the same answer. As a bit of
> extra explanation, the &&-chaining means that every command in the
> chain must return "success" (status 0), but the return code of `grep`
> depends upon whether or not it matched any lines. In this case, it
> returned non-zero which caused the test to fail.
Thanks Eric and Partrick. Thanks for the explanation. I will update
the patch then.

Re: [PATCH v3 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-files`

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 8:52 AM <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> On 06/10/2024 17:06, Usman Akinyemi via GitGitGadget wrote:
> > 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.
>
> This is a good description of the reason for making this change.
>
> > This particular patch focuses on some of the instances
> > which include `git show` and `git cat-files`.
>
> This patch seems to fix all of the instances of "git show" rather than
> some. It also fixes a few instances of "git cat-file" (note there is no
> trailing "s" in the command name). It is not immediately clear to me why
> those instances of "git cat-file" were selected for conversion but not
> others. However they are a worthwhile improvement and converting them
> all in this patch would make it bit too big to comfortably review so I'm
> happy with the changes as they are.
>
> The patch itself looks good.
>
> Thanks
>
> Phillip
Since, I am updating a patch. I will update this commit message also.
Thank you for the review.
>
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >   t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
> >   1 file changed, 48 insertions(+), 23 deletions(-)
> >
> > 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^)"
> >   '
> >
>

[PATCH v4 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

Changes since v3:

 * Replaced the use of grep | wc -l with a simplified ! grep command to
   directly check for the absence of "NEVER" in the output. I was not able
   to add this in the second patch ( employing test_line_count() to replace
   test). I was able to come to this solution with the help of Eric and
   Patriack.

Usman Akinyemi (2):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-file`
  [Outreachy][Patch v1] t3404: employing test_line_count() to replace
    test

 t/t3404-rebase-interactive.sh | 73 +++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 24 deletions(-)


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

Range-diff vs v3:

 1:  c9a0cca179b ! 1:  bfff7937cd2 t3404: avoid losing exit status with focus on `git show` and `git cat-files`
     @@ Metadata
      Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## Commit message ##
     -    t3404: avoid losing exit status with focus on `git show` and `git cat-files`
     +    t3404: avoid losing exit status with focus on `git show` and `git cat-file`
      
          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. This particular patch focuses on some of the instances
     -    which include `git show` and `git cat-files`.
     +    command fails. This particular patch focuses on all `git show` and
     +    some instances of `git cat-file`.
      
          Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
 2:  37b1411ee2c ! 2:  864b00997b7 [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
     @@ Commit message
      
          Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
     +    removed test
     +
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'stop on conflicting pick' '
       	test_cmp expect2 file1 &&
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'multi-squash only fires up e
       '
       
       test_expect_success 'multi-fixup does not fire up editor' '
     +@@ t/t3404-rebase-interactive.sh: test_expect_success 'multi-fixup does not fire up editor' '
     + 	) &&
     + 	test $base = $(git rev-parse HEAD^) &&
     + 	git show >output &&
     +-	count=$(grep NEVER output | wc -l) &&
     +-	test 0 = $count &&
     ++	! grep NEVER output &&
     + 	git checkout @{-1} &&
     + 	git branch -D multi-fixup
     + '
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'commit message used after conflict' '
       	) &&
       	test $base = $(git rev-parse HEAD^) &&

-- 
gitgitgadget

[PATCH v4 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Usman Akinyemi via GitGitGadget]

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. This particular patch focuses on all `git show` and
some instances of `git cat-file`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

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^)"
 '
 
-- 
gitgitgadget

[PATCH v4 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>

removed test
---
 t/t3404-rebase-interactive.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..81f984f4cf5 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
+	grep -v "^#" < .git/rebase-merge/done > actual &&
+	test_line_count = 4 actual &&
 	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
 '
 
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
-	test 0 = $count &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

Re: [PATCH v4 2/2] [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

[Patrick Steinhardt]

On Mon, Oct 07, 2024 at 10:22:11AM +0000, Usman Akinyemi via GitGitGadget wrote:

The title of this commit still has the "[Outreachy][Patch v1]" prefix
that you likely want to remove.

> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> 
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
> 
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> 
> removed test

I think this line got here by accident :)

> ---
>  t/t3404-rebase-interactive.sh | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a65783c47..81f984f4cf5 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
>  	test_cmp expect2 file1 &&
>  	test "$(git diff --name-status |
>  		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> +	grep -v "^#" < .git/rebase-merge/done > actual &&

We do not use a space after redirects, so this should be
"<.git/rebase-merge/done" and ">actual".

Other than that this patch looks good to me, thanks!

Patrick

[PATCH v5 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

Changes since v3:

 * Replaced the use of grep | wc -l with a simplified ! grep command to
   directly check for the absence of "NEVER" in the output. I was not able
   to add this in the second patch ( employing test_line_count() to replace
   test). I was able to come to this solution with the help of Eric and
   Patriack.

Usman Akinyemi (2):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-file`
  t3404: employing test_line_count() to replace test

 t/t3404-rebase-interactive.sh | 73 +++++++++++++++++++++++------------
 1 file changed, 49 insertions(+), 24 deletions(-)


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

Range-diff vs v4:

 1:  bfff7937cd2 = 1:  bfff7937cd2 t3404: avoid losing exit status with focus on `git show` and `git cat-file`
 2:  864b00997b7 ! 2:  0ce40300fa3 [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
     @@ Metadata
      Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## Commit message ##
     -    [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
     +    t3404: employing test_line_count() to replace test
      
          Refactor t3404 to replace instances of `test` with `test_line_count()`
          for checking line counts. This improves readability and aligns with Git's
     @@ Commit message
      
          Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
     -    removed test
     -
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'stop on conflicting pick' '
       	test_cmp expect2 file1 &&
       	test "$(git diff --name-status |
       		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
      -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
     -+	grep -v "^#" < .git/rebase-merge/done > actual &&
     ++	grep -v "^#" < .git/rebase-merge/done >actual &&
      +	test_line_count = 4 actual &&
       	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
       '

-- 
gitgitgadget

[PATCH v5 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Usman Akinyemi via GitGitGadget]

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. This particular patch focuses on all `git show` and
some instances of `git cat-file`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

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^)"
 '
 
-- 
gitgitgadget

[PATCH v5 2/2] t3404: employing test_line_count() to replace test

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..1073eb88fa2 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
+	grep -v "^#" < .git/rebase-merge/done >actual &&
+	test_line_count = 4 actual &&
 	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
 '
 
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
-	test 0 = $count &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

Re: [PATCH v5 2/2] t3404: employing test_line_count() to replace test

[Patrick Steinhardt]

On Mon, Oct 07, 2024 at 10:51:48AM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a65783c47..1073eb88fa2 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
>  	test_cmp expect2 file1 &&
>  	test "$(git diff --name-status |
>  		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> +	grep -v "^#" < .git/rebase-merge/done >actual &&
> +	test_line_count = 4 actual &&
>  	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
>  '

There's still the space between `<` and `.git/rebase-merge/git-rebase-todo`.
But overall this version looks good enough to me, so I don't think this
requires a reroll. Thanks!

Patrick

Re: [PATCH v5 2/2] t3404: employing test_line_count() to replace test

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 10:54 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 07, 2024 at 10:51:48AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 96a65783c47..1073eb88fa2 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -281,7 +281,8 @@ test_expect_success 'stop on conflicting pick' '
> >       test_cmp expect2 file1 &&
> >       test "$(git diff --name-status |
> >               sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> > -     test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> > +     grep -v "^#" < .git/rebase-merge/done >actual &&
> > +     test_line_count = 4 actual &&
> >       test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> >  '
>
> There's still the space between `<` and `.git/rebase-merge/git-rebase-todo`.
> But overall this version looks good enough to me, so I don't think this
> requires a reroll. Thanks!
>
> Patrick
Thanks for the review. I think it is better to fix it now. I already
did. Thank you.

[PATCH v6 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

Changes since v5:

 * Remove spaces between redirect to conform with git style.

Usman Akinyemi (2):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-file`
  t3404: employing test_line_count() to replace test

 t/t3404-rebase-interactive.sh | 75 +++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 25 deletions(-)


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

Range-diff vs v5:

 1:  bfff7937cd2 = 1:  bfff7937cd2 t3404: avoid losing exit status with focus on `git show` and `git cat-file`
 2:  0ce40300fa3 ! 2:  b3d3deced25 t3404: employing test_line_count() to replace test
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'stop on conflicting pick' '
       	test "$(git diff --name-status |
       		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
      -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
     -+	grep -v "^#" < .git/rebase-merge/done >actual &&
     +-	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
     ++	grep -v "^#" <.git/rebase-merge/done >actual &&
      +	test_line_count = 4 actual &&
     - 	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
     ++	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
       '
       
     + test_expect_success 'show conflicted patch' '
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'multi-squash only fires up editor once' '
       	) &&
       	test $base = $(git rev-parse HEAD^) &&

-- 
gitgitgadget

[PATCH v6 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Usman Akinyemi via GitGitGadget]

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. This particular patch focuses on all `git show` and
some instances of `git cat-file`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

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^)"
 '
 
-- 
gitgitgadget

[PATCH v6 2/2] t3404: employing test_line_count() to replace test

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..2ab660ef30f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
-	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
+	grep -v "^#" <.git/rebase-merge/done >actual &&
+	test_line_count = 4 actual &&
+	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
 '
 
 test_expect_success 'show conflicted patch' '
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
-	test 0 = $count &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

Re: [PATCH v6 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Patrick Steinhardt]

On Mon, Oct 07, 2024 at 11:11:04AM +0000, Usman Akinyemi via GitGitGadget wrote:
> Changes since v5:
> 
>  * Remove spaces between redirect to conform with git style.

Thanks, this version looks good to me now.

Patrick

Re: [PATCH v6 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 11:12 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 07, 2024 at 11:11:04AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > Changes since v5:
> >
> >  * Remove spaces between redirect to conform with git style.
>
> Thanks, this version looks good to me now.
>
> Patrick

Thank you very much and I appreciate all the guidance from all
reviewers. I Learned a great lot of new things.
Going forward, what should be my next step? Can I work on other tasks
now or wait for this to be merged? Are there any other things also
expected from my side as an outreachy applicant ? Thank you  very
much.

Re: [PATCH v6 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Patrick Steinhardt]

On Mon, Oct 07, 2024 at 11:16:49AM +0000, Usman Akinyemi wrote:
> On Mon, Oct 7, 2024 at 11:12 AM Patrick Steinhardt <ps@pks.im> wrote:
> >
> > On Mon, Oct 07, 2024 at 11:11:04AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > Changes since v5:
> > >
> > >  * Remove spaces between redirect to conform with git style.
> >
> > Thanks, this version looks good to me now.
> >
> > Patrick
> 
> Thank you very much and I appreciate all the guidance from all
> reviewers. I Learned a great lot of new things.
> Going forward, what should be my next step? Can I work on other tasks
> now or wait for this to be merged? Are there any other things also
> expected from my side as an outreachy applicant ? Thank you  very
> much.

For now I'd recommend to wait a couple of days until the patch series
you have sent gets picked up by Junio and merged to `next`. You should
watch out for the "What's cooking?" reports that he sends out every
couple days and observe how your topic progresses in it. Note that
things may go a bit slower right now due to the pending Git v2.47
release, so it may take a while before he picks your topic.

When things go smoothly: congrats, you have checked the first box and
have successfully completed your microproject :) I'd recommend to keep
on reading the mailing list to get familiar with how things work over
here and get some familiarity with the code in question.

You are of course free to send additional patches, but this is not a
requirement. If you still want to do so I'd recommend to not pick up a
microproject, but try to find a work item on your own, e.g. by searching
for "#leftoverbits" in our mailing list. The reason for why I don't
recommend to pick up another microproject is so that other applicants
have items to work on, and it decreases the likelihood that your changes
collide with the work of another intern.

Patrick

Re: [PATCH v6 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 11:33 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Oct 07, 2024 at 11:16:49AM +0000, Usman Akinyemi wrote:
> > On Mon, Oct 7, 2024 at 11:12 AM Patrick Steinhardt <ps@pks.im> wrote:
> > >
> > > On Mon, Oct 07, 2024 at 11:11:04AM +0000, Usman Akinyemi via GitGitGadget wrote:
> > > > Changes since v5:
> > > >
> > > >  * Remove spaces between redirect to conform with git style.
> > >
> > > Thanks, this version looks good to me now.
> > >
> > > Patrick
> >
> > Thank you very much and I appreciate all the guidance from all
> > reviewers. I Learned a great lot of new things.
> > Going forward, what should be my next step? Can I work on other tasks
> > now or wait for this to be merged? Are there any other things also
> > expected from my side as an outreachy applicant ? Thank you  very
> > much.
>
> For now I'd recommend to wait a couple of days until the patch series
> you have sent gets picked up by Junio and merged to `next`. You should
> watch out for the "What's cooking?" reports that he sends out every
> couple days and observe how your topic progresses in it. Note that
> things may go a bit slower right now due to the pending Git v2.47
> release, so it may take a while before he picks your topic.
>
> When things go smoothly: congrats, you have checked the first box and
> have successfully completed your microproject :) I'd recommend to keep
> on reading the mailing list to get familiar with how things work over
> here and get some familiarity with the code in question.
>
> You are of course free to send additional patches, but this is not a
> requirement. If you still want to do so I'd recommend to not pick up a
> microproject, but try to find a work item on your own, e.g. by searching
> for "#leftoverbits" in our mailing list. The reason for why I don't
> recommend to pick up another microproject is so that other applicants
> have items to work on, and it decreases the likelihood that your changes
> collide with the work of another intern.
>
> Patrick

Thanks a lot, I really appreciate it.

Re: [PATCH v6 2/2] t3404: employing test_line_count() to replace test

[Phillip Wood]

Hi Usman

I missed this when I read the earlier version of this series but our 
commit subjects use the imperative so we would say

     t3403: employ test_line_count() to replace test

or

     t3404: replace test with test_line_count()

I agree with Patrick that we should let this series settle for a couple 
of days to allow others to comment before sending any new version.

This series is looking good, congratulations

Phillip

On 07/10/2024 12:11, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> 
> Refactor t3404 to replace instances of `test` with `test_line_count()`
> for checking line counts. This improves readability and aligns with Git's
> current test practices.
> 
> Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> ---
>   t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
>   1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 96a65783c47..2ab660ef30f 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
>   	test_cmp expect2 file1 &&
>   	test "$(git diff --name-status |
>   		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> -	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> -	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> +	grep -v "^#" <.git/rebase-merge/done >actual &&
> +	test_line_count = 4 actual &&
> +	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
>   '
>   
>   test_expect_success 'show conflicted patch' '
> @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual
>   '
>   
>   test_expect_success 'multi-fixup does not fire up editor' '
> @@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep NEVER output | wc -l) &&
> -	test 0 = $count &&
> +	! grep NEVER output &&
>   	git checkout @{-1} &&
>   	git branch -D multi-fixup
>   '
> @@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count &&
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual &&
>   	git checkout @{-1} &&
>   	git branch -D conflict-fixup
>   '
> @@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep TWICE output | wc -l) &&
> -	test 2 = $count &&
> +	grep TWICE output >actual &&
> +	test_line_count = 2 actual &&
>   	git checkout @{-1} &&
>   	git branch -D conflict-squash
>   '
> @@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count &&
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual &&
>   	git checkout @{-1} &&
>   	git branch -D skip-comments
>   '
> @@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
>   	) &&
>   	test $base = $(git rev-parse HEAD^) &&
>   	git show >output &&
> -	count=$(grep ONCE output | wc -l) &&
> -	test 1 = $count &&
> +	grep ONCE output >actual &&
> +	test_line_count = 1 actual &&
>   	git checkout @{-1} &&
>   	git branch -D skip-blank-lines
>   '

[PATCH v7 0/2] [Outreachy][Patch v2] t3404: avoid losing exit status to pipes

[Usman Akinyemi via GitGitGadget]

Changes since v6:

 * Updated commit subjects to use imperative for proper Git commit subject.

Usman Akinyemi (2):
  t3404: avoid losing exit status with focus on `git show` and `git
    cat-file`
  t3404: replace test with test_line_count()

 t/t3404-rebase-interactive.sh | 75 +++++++++++++++++++++++------------
 1 file changed, 50 insertions(+), 25 deletions(-)


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

Range-diff vs v6:

 1:  bfff7937cd2 = 1:  bfff7937cd2 t3404: avoid losing exit status with focus on `git show` and `git cat-file`
 2:  b3d3deced25 ! 2:  e2cae7f3a51 t3404: employing test_line_count() to replace test
     @@ Metadata
      Author: Usman Akinyemi <usmanakinyemi202@gmail.com>
      
       ## Commit message ##
     -    t3404: employing test_line_count() to replace test
     +    t3404: replace test with test_line_count()
      
          Refactor t3404 to replace instances of `test` with `test_line_count()`
          for checking line counts. This improves readability and aligns with Git's

-- 
gitgitgadget

[PATCH v7 1/2] t3404: avoid losing exit status with focus on `git show` and `git cat-file`

[Usman Akinyemi via GitGitGadget]

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. This particular patch focuses on all `git show` and
some instances of `git cat-file`.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)

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^)"
 '
 
-- 
gitgitgadget

[PATCH v7 2/2] t3404: replace test with test_line_count()

[Usman Akinyemi via GitGitGadget]

From: Usman Akinyemi <usmanakinyemi202@gmail.com>

Refactor t3404 to replace instances of `test` with `test_line_count()`
for checking line counts. This improves readability and aligns with Git's
current test practices.

Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 96a65783c47..2ab660ef30f 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
 	test_cmp expect2 file1 &&
 	test "$(git diff --name-status |
 		sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
-	test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
-	test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
+	grep -v "^#" <.git/rebase-merge/done >actual &&
+	test_line_count = 4 actual &&
+	test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
 '
 
 test_expect_success 'show conflicted patch' '
@@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count
+	grep ONCE output >actual &&
+	test_line_count = 1 actual
 '
 
 test_expect_success 'multi-fixup does not fire up editor' '
@@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep NEVER output | wc -l) &&
-	test 0 = $count &&
+	! grep NEVER output &&
 	git checkout @{-1} &&
 	git branch -D multi-fixup
 '
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-fixup
 '
@@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep TWICE output | wc -l) &&
-	test 2 = $count &&
+	grep TWICE output >actual &&
+	test_line_count = 2 actual &&
 	git checkout @{-1} &&
 	git branch -D conflict-squash
 '
@@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-comments
 '
@@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
 	) &&
 	test $base = $(git rev-parse HEAD^) &&
 	git show >output &&
-	count=$(grep ONCE output | wc -l) &&
-	test 1 = $count &&
+	grep ONCE output >actual &&
+	test_line_count = 1 actual &&
 	git checkout @{-1} &&
 	git branch -D skip-blank-lines
 '
-- 
gitgitgadget

Re: [PATCH v6 2/2] t3404: employing test_line_count() to replace test

[Usman Akinyemi]

On Mon, Oct 7, 2024 at 3:07 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Usman
>
> I missed this when I read the earlier version of this series but our
> commit subjects use the imperative so we would say
>
>      t3403: employ test_line_count() to replace test
>
> or
>
>      t3404: replace test with test_line_count()
>
Thanks for the review. I just improved the commit subject now.
> I agree with Patrick that we should let this series settle for a couple
> of days to allow others to comment before sending any new version.
>
> This series is looking good, congratulations
Thanks Philip.
>
> Phillip
>
> On 07/10/2024 12:11, Usman Akinyemi via GitGitGadget wrote:
> > From: Usman Akinyemi <usmanakinyemi202@gmail.com>
> >
> > Refactor t3404 to replace instances of `test` with `test_line_count()`
> > for checking line counts. This improves readability and aligns with Git's
> > current test practices.
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
> >   t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
> >   1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > index 96a65783c47..2ab660ef30f 100755
> > --- a/t/t3404-rebase-interactive.sh
> > +++ b/t/t3404-rebase-interactive.sh
> > @@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' '
> >       test_cmp expect2 file1 &&
> >       test "$(git diff --name-status |
> >               sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 &&
> > -     test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) &&
> > -     test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo)
> > +     grep -v "^#" <.git/rebase-merge/done >actual &&
> > +     test_line_count = 4 actual &&
> > +     test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
> >   '
> >
> >   test_expect_success 'show conflicted patch' '
> > @@ -401,8 +402,8 @@ test_expect_success 'multi-squash only fires up editor once' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual
> >   '
> >
> >   test_expect_success 'multi-fixup does not fire up editor' '
> > @@ -416,8 +417,7 @@ test_expect_success 'multi-fixup does not fire up editor' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep NEVER output | wc -l) &&
> > -     test 0 = $count &&
> > +     ! grep NEVER output &&
> >       git checkout @{-1} &&
> >       git branch -D multi-fixup
> >   '
> > @@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count &&
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual &&
> >       git checkout @{-1} &&
> >       git branch -D conflict-fixup
> >   '
> > @@ -456,8 +456,8 @@ test_expect_success 'commit message retained after conflict' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep TWICE output | wc -l) &&
> > -     test 2 = $count &&
> > +     grep TWICE output >actual &&
> > +     test_line_count = 2 actual &&
> >       git checkout @{-1} &&
> >       git branch -D conflict-squash
> >   '
> > @@ -501,8 +501,8 @@ test_expect_success 'squash ignores comments' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count &&
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual &&
> >       git checkout @{-1} &&
> >       git branch -D skip-comments
> >   '
> > @@ -519,8 +519,8 @@ test_expect_success 'squash ignores blank lines' '
> >       ) &&
> >       test $base = $(git rev-parse HEAD^) &&
> >       git show >output &&
> > -     count=$(grep ONCE output | wc -l) &&
> > -     test 1 = $count &&
> > +     grep ONCE output >actual &&
> > +     test_line_count = 1 actual &&
> >       git checkout @{-1} &&
> >       git branch -D skip-blank-lines
> >   '
>