t3404: avoid losing exit status to pipes

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..33ea1f05e2c 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-06 05:33 UTC #4 lore raw

PATCH v2 2/2 t3404: avoid losing exit status to pipes

From: Usman Akinyemi <[email protected]>

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

---
 t/t3404-rebase-interactive.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

t/t3404-rebase-interactive.sh +6 -6

index 33ea1f05e2c..3d8de69d6ee 100755
@@ -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

Signature · 1 line

gitgitgadget

Eric Sunshine

2024-10-06 05:48 UTC #5 lore raw

Quoted 5 lines · Usman Akinyemi via GitGitGadget

Changes since v1:
- Added "tr -d '[:space:]'" to handle whitespace on macOS
Signed-off-by Usman Akinyemi <[email protected]>
---

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

t/t3404-rebase-interactive.sh test_expect_success 'multi-squash only fires up editor once' ' · line 404

Quoted 7 lines

t/t3404-rebase-interactive.sh +1 -1

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

Eric Sunshine

2024-10-06 05:55 UTC #2 lore raw

Quoted 5 lines · Usman Akinyemi via GitGitGadget

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

t/t3404-rebase-interactive.sh test_expect_success 'squash and fixup generate correct log messages' ' · line 484

Quoted 12 lines

---

t/t3404-rebase-interactive.sh +4 -4

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

Usman Akinyemi

2024-10-06 06:30 UTC #6 lore raw

Quoted 45 lines · Eric Sunshine

Quoted 5 lines · Usman Akinyemi via GitGitGadget
Changes since v1:
- Added "tr -d '[:space:]'" to handle whitespace on macOS
Signed-off-by Usman Akinyemi <[email protected]>
---
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...
t/t3404-rebase-interactive.sh test_expect_success 'multi-squash only fires up editor once' ' · line 404
Quoted 7 lines
t/t3404-rebase-interactive.sh +1 -1
@@ -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:

Quoted 8 lines

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.

Usman Akinyemi

2024-10-06 06:31 UTC #3 lore raw

Quoted 30 lines · Eric Sunshine

Quoted 5 lines · Usman Akinyemi via GitGitGadget
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...
t/t3404-rebase-interactive.sh test_expect_success 'squash and fixup generate correct log messages' ' · line 484
Quoted 12 lines
Signed-off-by Usman Akinyemi <[email protected]>
---
t/t3404-rebase-interactive.sh +4 -4
@@ -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:

Quoted 3 lines

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

Noted and thanks.

Eric Sunshine

2024-10-06 07:28 UTC #7 lore raw

Quoted 27 lines · Usman Akinyemi

Quoted 17 lines · Eric Sunshine
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:
Quoted 8 lines
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().

Quoted 5 lines

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

Quoted 3 lines

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

Quoted 2 lines

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.

Usman Akinyemi

2024-10-06 08:26 UTC #8 lore raw

Quoted 33 lines · Eric Sunshine

Quoted 30 lines · Usman Akinyemi
Quoted 17 lines · Eric Sunshine
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:
Quoted 8 lines
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.

Quoted 8 lines

Quoted 2 lines
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.

Quoted 34 lines

Quoted 3 lines
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".)

Quoted 6 lines

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.

Quoted 11 lines

Quoted 2 lines
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.

Usman Akinyemi via GitGitGadget

author

2024-10-06 08:31 UTC #9 lore raw

PATCH 1/1 t3404: avoid losing exit status to pipes

From: Usman Akinyemi <[email protected]>

---
[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 [email protected] 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(-)

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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

Signature · 1 line

gitgitgadget

Eric Sunshine

2024-10-06 09:19 UTC #10 lore raw

Quoted 13 lines · Usman Akinyemi via GitGitGadget

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 <[email protected]>
---
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.

Quoted 4 lines

Eric Sunshine [email protected] provided valuable feedback during the review process. He explained the details of the patches to me and pointed out that using tr -d was unnecessary to resolve the whitespace issue.

Thanks. This version of the patch looks fine.

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

So, I think it makes sense to stop here and consider this microproject successful (unless some other reviewer notices some problem or requests some other change).

Usman Akinyemi

2024-10-06 09:32 UTC #11 lore raw

Quoted 32 lines · Eric Sunshine

Quoted 13 lines · Usman Akinyemi via GitGitGadget
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 <[email protected]>
---
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.
Quoted 4 lines
Eric Sunshine [email protected] 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.

Quoted 3 lines

So, I think it makes sense to stop here and consider this microproject successful (unless some other reviewer notices some problem or requests some other change).

Thanks Eric Sunshine, I appreciate your time and review. I am more eager to continue working on it after review from the others. And also would like to work on the test_line_count also if permitted. Thank you very much.

Eric Sunshine

2024-10-06 10:21 UTC #12 lore raw

Quoted 9 lines · Usman Akinyemi

Quoted 3 lines · Eric Sunshine
So, I think it makes sense to stop here and consider this microproject successful (unless some other reviewer notices some problem or requests some other change).
Thanks Eric Sunshine, I appreciate your time and review. I am more eager to continue working on it after review from the others. And also would like to work on the test_line_count also if permitted. Thank you very much.

You don't have to ask permission; if you feel like converting the test to use test_line_count(), you are welcome to do so.

shejialuo

2024-10-06 11:12 UTC #13 lore raw

On Sun, Oct 06, 2024 at 05:19:13AM -0400, Eric Sunshine wrote:

[snip]

Quoted 3 lines

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

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

Thanks,
Jialuo

Eric Sunshine

2024-10-06 12:06 UTC #14 lore raw

Quoted 8 lines · shejialuo

Quoted 3 lines · Eric Sunshine
It probably would have been helpful to reviewers if the patch's commit message mentioned that it only converts some of the instances, but it's not worth rerolling the patch just for that.
Except that, the commit title should not either include "[Outreachy][Patch v1]" here. From these two reasons, I think we should reroll the patch.

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

Usman Akinyemi

2024-10-06 12:28 UTC #15 lore raw

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.

Quoted 25 lines · Eric Sunshine

Quoted 8 lines · shejialuo
Quoted 3 lines · Eric Sunshine
It probably would have been helpful to reviewers if the patch's commit message mentioned that it only converts some of the instances, but it's not worth rerolling the patch just for that.
Except that, the commit title should not either include "[Outreachy][Patch v1]" here. From these two reasons, I think we should reroll the patch.
Your observation about outdated/confusing "[foo]" annotations is certainly something the submitter should take into consideration for future submissions, but does not seem worthy of a reroll, IMHO. First, git am will strip those off automatically, so they won't become part of the permanent project history anyhow when/if Junio picks up the patch. Second, asking for a reroll for something which does not impact the correctness of either the patch or the commit message just makes busy-work for the submitter and wastes reviewer time (which is a limited resource on this project). Third, the point of a microproject is to expose the submitter to the workflow of the Git project and to the review process, and for reviewers to see how the submitter responds. That goal has already been achieved in this case, and rerolling for something so minor provides no additional benefit in that regard.

shejialuo

2024-10-06 12:29 UTC #19 lore raw

Quoted 6 lines · Eric Sunshine

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

Quoted 8 lines

the correctness of either the patch or the commit message just makes busy-work for the submitter and wastes reviewer time (which is a limited resource on this project). Third, the point of a microproject is to expose the submitter to the workflow of the Git project and to the review process, and for reviewers to see how the submitter responds. That goal has already been achieved in this case, and rerolling for something so minor provides no additional benefit in that regard.

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

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

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

Thanks,
Jialuo

Usman Akinyemi

2024-10-06 12:46 UTC #20 lore raw

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.

Quoted 36 lines · shejialuo

Quoted 6 lines · Eric Sunshine
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
Quoted 8 lines
the correctness of either the patch or the commit message just makes busy-work for the submitter and wastes reviewer time (which is a limited resource on this project). Third, the point of a microproject is to expose the submitter to the workflow of the Git project and to the review process, and for reviewers to see how the submitter responds. That goal has already been achieved in this case, and rerolling for something so minor provides no additional benefit in that regard.
Thanks for your detailed explanation here. I don't know that "git am" could strip those off automatically. I thought the maintainer would delete "[foo]" manually. So, my main intention here is that I want the submitter to make it more perfect to reduce the overhead of the maintainer and also pay attention to this for further submissions.
And from my perspective, the reroll would not bring much overhead for the submitter, so I expressed my words in the previous email. I know you concerned that my words would frustrate the Usman. And I wanna say this is not my intention here. I think Usamn has already done a great job for this microproject to understand the workflow of the Git project. So, actually we are on the same boat here.
Let me withdraw my previous words ("We should reroll the patch"). This patch is good and don't need a reroll.
Thanks,
Jialuo

shejialuo

2024-10-06 13:02 UTC #16 lore raw

[1] https://lore.kernel.org/git/CAPig+cTb4mgpXnN79UrXvjvCnqGZhaR51oZX_Ds=HwdqQYFN9w@mail.gmail.com/

Quoted 3 lines · Usman Akinyemi

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.

Quoted 11 lines

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

Usman Akinyemi

2024-10-06 13:27 UTC #17 lore raw

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.

Quoted 37 lines · shejialuo

Quoted 3 lines · Usman Akinyemi
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.
Quoted 11 lines
These days, instead of manually using wc -l and test, we would instead write:
grep ONCE output >actual &&
test_line_count 1 actual
However, that sort of change is independent of the purpose of this patch, so you probably should not make such a change in this patch. If you're up to it, you could instead turn this into a two-patch series in which patch [1/2] fixes the "Git upstream of a pipe" problem, and then patch [2/2] converts these cases to use test_line_count().
If you decide to do this. As Eric has commented in [1], you should add a new commit (a new patch) followed by current patch to convert to the test_line_count. Then you should re-send the series to the mailing list. And thus you could enhance the commit message of the first patch. If you do not decide to do this (the current patch is enough for the microproject), you don't need to reroll for the minor things.
So, you should never update the current patch for converting the test using test_line_count. Instead, create a new commit to do this. and BTW you could change the commit message of the first patch if you want.
Thanks,
Jialuo
[1] https://lore.kernel.org/git/CAPig+cTb4mgpXnN79UrXvjvCnqGZhaR51oZX_Ds=HwdqQYFN9w@mail.gmail.com/

shejialuo

2024-10-06 13:36 UTC #18 lore raw

Quoted 8 lines · 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?

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

Quoted 3 lines

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/

Usman Akinyemi via GitGitGadget

author

2024-10-06 16:06 UTC #22 lore raw

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

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 <[email protected]>

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

-: ----------- > 2: 37b1411ee2c [Outreachy][Patch v1] t3404: employing test_line_count() to replace test

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-06 16:06 UTC #23 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +12 ~~-11~~

index 96a65783c47..19390eaf331 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-06 16:06 UTC #29 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi

2024-10-06 16:18 UTC #33 lore raw

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.

Quoted 60 lines · 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 [email protected] 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 <[email protected]>
## 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 <[email protected]>
-: ----------- > 2: 37b1411ee2c [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
--
gitgitgadget

Kristoffer Haugsbakk

2024-10-06 16:21 UTC #39 lore raw

Quoted 6 lines · Usman Akinyemi via GitGitGadget

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.

Quoted 7 lines

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.

Usman Akinyemi

2024-10-06 16:26 UTC #40 lore raw

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

Quoted 19 lines · Kristoffer Haugsbakk

Quoted 6 lines · Usman Akinyemi via GitGitGadget
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.
Quoted 7 lines
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.

Kristoffer Haugsbakk

2024-10-06 16:31 UTC #24 lore raw

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

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>
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 <[email protected]>

Nice commit message.

Signature · 1 line

Kristoffer Haugsbakk

Eric Sunshine

2024-10-07 04:19 UTC #21 lore raw

In reply to shejialuo

Quoted 16 lines · shejialuo

Quoted 6 lines · Eric Sunshine
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
Quoted 8 lines
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.

Quoted 5 lines

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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

Eric Sunshine

2024-10-07 04:24 UTC #34 lore raw

Quoted 5 lines · Usman Akinyemi

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)

Quoted 7 lines

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

Eric Sunshine

2024-10-07 04:38 UTC #25 lore raw

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

t/t3404-rebase-interactive.sh test_expect_success 'stop on conflicting pick' ' · line 282

Quoted 11 lines · Usman Akinyemi via GitGitGadget

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 <[email protected]>
---
t/t3404-rebase-interactive.sh +2 -1
@@ -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.

Patrick Steinhardt

2024-10-07 05:55 UTC #41 lore raw

Quoted 1 line · Usman Akinyemi

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

Quoted 21 lines

Quoted 19 lines · Kristoffer Haugsbakk
Quoted 6 lines · Usman Akinyemi via GitGitGadget
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.
Quoted 7 lines
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.

Patrick Steinhardt

2024-10-07 06:05 UTC #30 lore raw

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

Quoted 8 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>
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.

Quoted 10 lines

Signed-off-by Usman Akinyemi <[email protected]>
---
 t/t3404-rebase-interactive.sh | 71 +++++++++++++++++++++++------------
 1 file changed, 48 insertions(+), 23 deletions(-)
t/t3404-rebase-interactive.sh +0 -0
index f171af3061d..96a65783c47 100755

t/t3404-rebase-interactive.sh test_expect_success 'retain authorship' ' · line 324

Quoted 7 lines

t/t3404-rebase-interactive.sh +2 -1

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

t/t3404-rebase-interactive.sh test_expect_success 'multi-squash only fires up editor once' ' · line 408

Quoted 11 lines

t/t3404-rebase-interactive.sh +3 -1

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

Quoted 5 lines

t/t3404-rebase-interactive.sh +0 -1

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

t/t3404-rebase-interactive.sh test_expect_success 'squash and fixup generate correct log messages' ' · line 490

Quoted 10 lines

t/t3404-rebase-interactive.sh +4 -3

-		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

Patrick Steinhardt

2024-10-07 06:05 UTC #26 lore raw

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

t/t3404-rebase-interactive.sh test_expect_success 'multi-squash only fires up editor once' ' · line 409

Quoted 11 lines · Usman Akinyemi via GitGitGadget

t/t3404-rebase-interactive.sh +2 -2

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

Usman Akinyemi

2024-10-07 07:25 UTC #35 lore raw

In reply to Patrick Steinhardt

Quoted 18 lines · Eric Sunshine

Quoted 5 lines · Usman Akinyemi
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)
Quoted 7 lines
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?

Quoted 7 lines

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
'

Usman Akinyemi

2024-10-07 07:32 UTC #27 lore raw

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

Quoted 18 lines · Patrick Steinhardt

t/t3404-rebase-interactive.sh test_expect_success 'multi-squash only fires up editor once' ' · line 409
Quoted 11 lines · Usman Akinyemi via GitGitGadget
t/t3404-rebase-interactive.sh +2 -2
@@ -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.

Quoted 4 lines

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.

Patrick Steinhardt

2024-10-07 08:08 UTC #36 lore raw

Quoted 49 lines · Usman Akinyemi

Quoted 18 lines · Eric Sunshine
Quoted 5 lines · Usman Akinyemi
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)
Quoted 7 lines
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?
Quoted 7 lines
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

Quoted 16 lines

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

t/t3404-rebase-interactive.sh +2 -1

index f171af3061..978fdfc2f1 100755
@@ -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
 '

Eric Sunshine

2024-10-07 08:11 UTC #37 lore raw

Quoted 10 lines · Patrick Steinhardt

Quoted 4 lines · Usman Akinyemi
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.

[email protected]

2024-10-07 08:52 UTC #31 lore raw

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

Hi Usman

Quoted 7 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>
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.

Quoted 2 lines

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

Quoted 201 lines

In reply to Patrick Steinhardt

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
  '

[email protected]

2024-10-07 09:00 UTC #28 lore raw

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

Hi Patrick

Quoted 7 lines · Patrick Steinhardt

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

Usman Akinyemi

2024-10-07 09:02 UTC #38 lore raw

In reply to [email protected]

Quoted 13 lines · Eric Sunshine

Quoted 10 lines · Patrick Steinhardt
Quoted 4 lines · Usman Akinyemi
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.

Quoted 5 lines

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.

Usman Akinyemi

2024-10-07 09:06 UTC #32 lore raw

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

Quoted 25 lines · On Mon, Oct 7, 2024 at 8:52 AM <[email protected]> wrote:

Hi Usman
Quoted 7 lines · Usman Akinyemi via GitGitGadget
From: Usman Akinyemi <[email protected]>
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.
Quoted 2 lines
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.

Quoted 5 lines

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.

Quoted 203 lines

Quoted 202 lines

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
  '

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:22 UTC #43 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:22 UTC #44 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

removed test

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

t/t3404-rebase-interactive.sh +13 ~~-13~~

index 96a65783c47..81f984f4cf5 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:22 UTC #42 lore raw

v2 cover t3404: avoid losing exit status to pipes

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 <[email protected]>

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

2: 37b1411ee2c ! 2: 864b00997b7 [Outreachy][Patch v1] t3404: employing test_line_count() to replace test
@@ Commit message

+ 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^) &&

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2024-10-07 10:28 UTC #45 lore raw

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

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.

Quoted 9 lines

From: Usman Akinyemi <[email protected]>
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 <[email protected]>
removed test

I think this line got here by accident :)

Quoted 9 lines

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

t/t3404-rebase-interactive.sh +0 -0

index 96a65783c47..81f984f4cf5 100755

t/t3404-rebase-interactive.sh test_expect_success 'stop on conflicting pick' ' · line 284

Quoted 5 lines

t/t3404-rebase-interactive.sh +1 -1

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

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:51 UTC #46 lore raw

v2 cover t3404: avoid losing exit status to pipes

Changes since v3:

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 <[email protected]>
      
       ## 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 <[email protected]>
      
     -    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)
       '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:51 UTC #47 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 10:51 UTC #48 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +13 ~~-13~~

index 96a65783c47..1073eb88fa2 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2024-10-07 10:54 UTC #49 lore raw

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

Quoted 6 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>
t/t3404-rebase-interactive.sh +0 -0
index 96a65783c47..1073eb88fa2 100755

t/t3404-rebase-interactive.sh test_expect_success 'stop on conflicting pick' ' · line 287

Quoted 8 lines

t/t3404-rebase-interactive.sh +2 -1

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

Usman Akinyemi

2024-10-07 11:11 UTC #50 lore raw

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

Quoted 21 lines · Patrick Steinhardt

Quoted 6 lines · Usman Akinyemi via GitGitGadget
From: Usman Akinyemi <[email protected]>
t/t3404-rebase-interactive.sh +0 -0
index 96a65783c47..1073eb88fa2 100755
t/t3404-rebase-interactive.sh test_expect_success 'stop on conflicting pick' ' · line 287
Quoted 8 lines
t/t3404-rebase-interactive.sh +2 -1
@@ -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!

Quoted 1 line

Patrick

Thanks for the review. I think it is better to fix it now. I already did. Thank you.

Usman Akinyemi via GitGitGadget

author

2024-10-07 11:11 UTC #51 lore raw

v2 cover t3404: avoid losing exit status to pipes

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^) &&

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 11:11 UTC #52 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 11:11 UTC #53 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

t/t3404-rebase-interactive.sh +14 ~~-14~~

index 96a65783c47..2ab660ef30f 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Patrick Steinhardt

2024-10-07 11:12 UTC #56 lore raw

Quoted 3 lines · Usman Akinyemi via GitGitGadget

Changes since v5:
* Remove spaces between redirect to conform with git style.

Thanks, this version looks good to me now.

Patrick

Usman Akinyemi

2024-10-07 11:17 UTC #57 lore raw

Quoted 9 lines · Patrick Steinhardt

Quoted 3 lines · Usman Akinyemi via GitGitGadget
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.

Patrick Steinhardt

2024-10-07 11:33 UTC #58 lore raw

Quoted 9 lines · Usman Akinyemi

Quoted 8 lines · Patrick Steinhardt
Quoted 3 lines · Usman Akinyemi via GitGitGadget
Changes since v5:
* Remove spaces between redirect to conform with git style.
Thanks, this version looks good to me now.

Quoted 8 lines

Quoted 1 line
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

Usman Akinyemi

2024-10-07 11:44 UTC #59 lore raw

Quoted 40 lines · Patrick Steinhardt

Quoted 9 lines · Usman Akinyemi
Quoted 8 lines · Patrick Steinhardt
Quoted 3 lines · Usman Akinyemi via GitGitGadget
Changes since v5:
* Remove spaces between redirect to conform with git style.
Thanks, this version looks good to me now.
Quoted 8 lines
Quoted 1 line
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.

Quoted 1 line

Patrick

Thanks a lot, I really appreciate it.

Phillip Wood

2024-10-07 15:07 UTC #54 lore raw

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

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

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

Quoted 92 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.

---
  t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
  1 file changed, 14 insertions(+), 14 deletions(-)

t/t3404-rebase-interactive.sh +14 ~~-14~~

index 96a65783c47..2ab660ef30f 100755
@@ -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
  '

Usman Akinyemi via GitGitGadget

author

2024-10-07 15:32 UTC #60 lore raw

v2 cover t3404: avoid losing exit status to pipes

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 <[email protected]>

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

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 15:32 UTC #61 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

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

t/t3404-rebase-interactive.sh +48 ~~-23~~

index f171af3061d..96a65783c47 100755
@@ -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^)"
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi via GitGitGadget

author

2024-10-07 15:32 UTC #62 lore raw

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

From: Usman Akinyemi <[email protected]>

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.

---
 t/t3404-rebase-interactive.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

t/t3404-rebase-interactive.sh +14 ~~-14~~

index 96a65783c47..2ab660ef30f 100755
@@ -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
 '

Signature · 1 line

gitgitgadget

Usman Akinyemi

2024-10-07 15:32 UTC #55 lore raw

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

In reply to Phillip Wood

Quoted 12 lines · 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()

Thanks for the review. I just improved the commit subject now.

Quoted 4 lines

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.

Quoted 97 lines

Phillip

Quoted 92 lines · Usman Akinyemi via GitGitGadget

From: Usman Akinyemi <[email protected]>

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.