t2027: avoid using pipes

PATCH Superseded Prathamesh Chavan 3 participants 11 messages Mar 8, 2017 – Mar 10, 2017

Share & tools

Jon Loeliger

2017-03-08 15:44 UTC #1 lore raw

Quoted 18 lines · Prathamesh Chavan

The exit code of the upstream of a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.
Signed-off-by Prathamesh <[email protected]>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
t/t2027-worktree-list.sh +0 -0
index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short 
HEAD) (detached HEAD)" >>expect &&

t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 36

Quoted 4 lines

t/t2027-worktree-list.sh +1 -1

-	git worktree list | sed "s/  */ /g" >actual &&
+	git worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

I confess I am not familiar with the test set up. However, I'd ask the question do we care about the lingering "out" and "actual" files here? Or will they silently be cleaned up along the way later?

Thanks,
jdl

Whenever a test suite is executed, after finishing every test, after running all tests, the function test_done is called. You may find this function in test-lib.sh . This function displays the result of the test and also removes the trash created by running the test.

Quoted 31 lines · Jon Loeliger

Quoted 18 lines · Prathamesh Chavan
The exit code of the upstream of a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.
Signed-off-by Prathamesh <[email protected]>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
t/t2027-worktree-list.sh +0 -0
index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
      test_when_finished "rm -rf here && git worktree prune" &&
      git worktree add --detach here master &&
      echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short
HEAD) (detached HEAD)" >>expect &&
t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 36
Quoted 4 lines
t/t2027-worktree-list.sh +1 -1
-     git worktree list | sed "s/  */ /g" >actual &&
+     git worktree list >out && sed "s/  */ /g" <out >actual &&
      test_cmp expect actual
 '
I confess I am not familiar with the test set up. However, I'd ask the question do we care about the lingering "out" and "actual" files here? Or will they silently be cleaned up along the way later?
Thanks,
jdl

Prathamesh Chavan

author

2017-03-08 22:13 UTC #3 lore raw

But when I read the function carefully, it only removes the trash files created when test_failure is equal to zero. But as far as I know, I can see the files being removed even when a test_failure is non-zero for some test script.

Quoted 37 lines · Prathamesh Chavan

Whenever a test suite is executed, after finishing every test, after running all tests, the function test_done is called. You may find this function in test-lib.sh . This function displays the result of the test and also removes the trash created by running the test.
Quoted 31 lines · Jon Loeliger
Quoted 18 lines · Prathamesh Chavan
The exit code of the upstream of a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.
Signed-off-by Prathamesh <[email protected]>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
t/t2027-worktree-list.sh +0 -0
index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
      test_when_finished "rm -rf here && git worktree prune" &&
      git worktree add --detach here master &&
      echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short
HEAD) (detached HEAD)" >>expect &&
t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 36
Quoted 4 lines
t/t2027-worktree-list.sh +1 -1
-     git worktree list | sed "s/  */ /g" >actual &&
+     git worktree list >out && sed "s/  */ /g" <out >actual &&
      test_cmp expect actual
 '
I confess I am not familiar with the test set up. However, I'd ask the question do we care about the lingering "out" and "actual" files here? Or will they silently be cleaned up along the way later?
Thanks,
jdl

Christian Couder

2017-03-09 08:08 UTC #4 lore raw

In reply to Prathamesh Chavan

Quoted 2 lines · Prathamesh Chavan

The exit code of the upstream of a pipe is ignored thus we should avoid using it.

You might want to say more specifically that we should avoid piping a git command into another one as this could mask a failure of the git command.

Quoted 13 lines

By writing out the output of the git command to a file, we can test the exit codes of both the commands.
Signed-off-by Prathamesh <[email protected]>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
t/t2027-worktree-list.sh +0 -0
index 848da5f..daa7a04 100755

t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 34

Quoted 5 lines

t/t2027-worktree-list.sh +1 -1

@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
        test_when_finished "rm -rf here && git worktree prune" &&
        git worktree add --detach here master &&
        echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-       git worktree list | sed "s/  */ /g" >actual &&
+       git worktree list >out && sed "s/  */ /g" <out >actual &&

I think it's better if the 'sed' command is on a separate line.

Also you may have used just "out" instead of "<out" in the 'sed' command...

t/t2027-worktree-list.sh test_expect_success 'broken main worktree still at the top' ' · line 121

Quoted 9 lines

t/t2027-worktree-list.sh +1 -1

        test_cmp expect actual
 '

@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
                cd linked &&
                echo "worktree $(pwd)" >expected &&
                echo "ref: .broken" >../.git/HEAD &&
-               git worktree list --porcelain | head -n 3 >actual &&
+               git worktree list --porcelain >out && head -n 3 out >actual &&

... as above you use "out" not "<out" in the 'head' command.

Quoted 6 lines

t/t2027-worktree-list.sh +1 -1

                test_cmp ../expected actual &&
-               git worktree list | head -n 1 >actual.2 &&
+               git worktree list >out && head -n 1 out >actual.2 &&
                grep -F "(error)" actual.2
        )
 '

Prathamesh Chavan

author

2017-03-09 09:31 UTC #5 lore raw

Quoted 7 lines · Christian Couder

Quoted 2 lines · Prathamesh Chavan
The exit code of the upstream of a pipe is ignored thus we should avoid using it.
You might want to say more specifically that we should avoid piping a git command into another one as this could mask a failure of the git command.

Yes. I will add be specific, and update my patch.

Quoted 22 lines

Quoted 14 lines
By writing out the output of the git command to a file, we can test the exit codes of both the commands.
Signed-off-by Prathamesh <[email protected]>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
t/t2027-worktree-list.sh +0 -0
index 848da5f..daa7a04 100755
t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 34
Quoted 5 lines
t/t2027-worktree-list.sh +1 -1
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
        test_when_finished "rm -rf here && git worktree prune" &&
        git worktree add --detach here master &&
        echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-       git worktree list | sed "s/  */ /g" >actual &&
+       git worktree list >out && sed "s/  */ /g" <out >actual &&
I think it's better if the 'sed' command is on a separate line.

Quoted 2 lines

Also you may have used just "out" instead of "<out" in the 'sed' command...

Actually I noticed that:

$ git grep sed |grep "<" |wc -l
307

As at most places, wherever pipes aren't being used, the input to sed command is passed using "<". Hence I chose to use "<" at places specifically at places where sed
was used, even after knowing that just "out" will work.

Quoted 18 lines

t/t2027-worktree-list.sh test_expect_success 'broken main worktree still at the top' ' · line 121

Quoted 9 lines

t/t2027-worktree-list.sh +1 -1

        test_cmp expect actual
 '

@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
                cd linked &&
                echo "worktree $(pwd)" >expected &&
                echo "ref: .broken" >../.git/HEAD &&
-               git worktree list --porcelain | head -n 3 >actual &&
+               git worktree list --porcelain >out && head -n 3 out >actual &&

... as above you use "out" not "<out" in the 'head' command.

Quoted 6 lines

t/t2027-worktree-list.sh +1 -1

                test_cmp ../expected actual &&
-               git worktree list | head -n 1 >actual.2 &&
+               git worktree list >out && head -n 1 out >actual.2 &&
                grep -F "(error)" actual.2
        )
 '

Prathamesh Chavan

author

2017-03-09 09:40 UTC #6 lore raw

PATCH v2 1/1 t2027: avoid using pipes

In reply to Prathamesh Chavan

The exit code of the upstream of a pipe is ignored thus we should avoid using it. By writing out the output of the git command to a file, we can test the exit codes of both the commands.

Signed-off-by Prathamesh <[email protected]>

---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t2027-worktree-list.sh +7 -7

index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git worktree list | sed "s/  */ /g" >actual &&
+	git worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -40,7 +40,7 @@ test_expect_success '"list" all worktrees from linked' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C here worktree list | sed "s/  */ /g" >actual &&
+	git -C here worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -73,7 +73,7 @@ test_expect_success '"list" all worktrees from bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C bare1 worktree list | sed "s/  */ /g" >actual &&
+	git -C bare1 worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -96,7 +96,7 @@ test_expect_success '"list" all worktrees from linked with a bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C there worktree list | sed "s/  */ /g" >actual &&
+	git -C there worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
 		cd linked &&
 		echo "worktree $(pwd)" >expected &&
 		echo "ref: .broken" >../.git/HEAD &&
-		git worktree list --porcelain | head -n 3 >actual &&
+		git worktree list --porcelain >out && head -n 3 out >actual &&
 		test_cmp ../expected actual &&
-		git worktree list | head -n 1 >actual.2 &&
+		git worktree list >out && head -n 1 out >actual.2 &&
 		grep -F "(error)" actual.2
 	)
 '

@@ -134,7 +134,7 @@ test_expect_success 'linked worktrees are sorted' '
 		test_commit new &&
 		git worktree add ../first &&
 		git worktree add ../second &&
-		git worktree list --porcelain | grep ^worktree >actual
+		git worktree list --porcelain >out && grep ^worktree out >actual
 	) &&
 	cat >expected <<-EOF &&
 	worktree $(pwd)/sorted/main

--
https://github.com/git/git/pull/336

Prathamesh Chavan

author

2017-03-09 09:54 UTC #7 lore raw

PATCH v2 1/1 t2027: avoid using pipes

Whenever a git command is present in the upstream of a pipe, its failure gets masked by piping and hence it should be avoided for testing the upstream git command. By writing out the output of the git command to a file, we can test the exit codes of both the commands as a failure exit code in any command is able to stop the && chain.

Signed-off-by Prathamesh <[email protected]>

---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t2027-worktree-list.sh +7 -7

index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git worktree list | sed "s/  */ /g" >actual &&
+	git worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -40,7 +40,7 @@ test_expect_success '"list" all worktrees from linked' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C here worktree list | sed "s/  */ /g" >actual &&
+	git -C here worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -73,7 +73,7 @@ test_expect_success '"list" all worktrees from bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C bare1 worktree list | sed "s/  */ /g" >actual &&
+	git -C bare1 worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -96,7 +96,7 @@ test_expect_success '"list" all worktrees from linked with a bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C there worktree list | sed "s/  */ /g" >actual &&
+	git -C there worktree list >out && sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -118,9 +118,9 @@ test_expect_success 'broken main worktree still at the top' '
 		cd linked &&
 		echo "worktree $(pwd)" >expected &&
 		echo "ref: .broken" >../.git/HEAD &&
-		git worktree list --porcelain | head -n 3 >actual &&
+		git worktree list --porcelain >out && head -n 3 out >actual &&
 		test_cmp ../expected actual &&
-		git worktree list | head -n 1 >actual.2 &&
+		git worktree list >out && head -n 1 out >actual.2 &&
 		grep -F "(error)" actual.2
 	)
 '

@@ -134,7 +134,7 @@ test_expect_success 'linked worktrees are sorted' '
 		test_commit new &&
 		git worktree add ../first &&
 		git worktree add ../second &&
-		git worktree list --porcelain | grep ^worktree >actual
+		git worktree list --porcelain >out && grep ^worktree out >actual
 	) &&
 	cat >expected <<-EOF &&
 	worktree $(pwd)/sorted/main

--
https://github.com/git/git/pull/336

Christian Couder

2017-03-09 12:31 UTC #8 lore raw

Quoted 8 lines · Prathamesh Chavan

Whenever a git command is present in the upstream of a pipe, its failure gets masked by piping and hence it should be avoided for testing the upstream git command. By writing out the output of the git command to a file, we can test the exit codes of both the commands as a failure exit code in any command is able to stop the && chain.
Signed-off-by Prathamesh <[email protected]>
---

Please add in Cc those who previously commented on the patch.

t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 34

Quoted 13 lines

 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t2027-worktree-list.sh +1 -1

index 848da5f..daa7a04 100755
@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
        test_when_finished "rm -rf here && git worktree prune" &&
        git worktree add --detach here master &&
        echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-       git worktree list | sed "s/  */ /g" >actual &&
+       git worktree list >out && sed "s/  */ /g" <out >actual &&

I still think that it would be better if the 'sed' commend was on its own line like this:

+ git worktree list >out &&
+ sed "s/ */ /g" <out >actual &&

Quoted 2 lines

t/t2027-worktree-list.sh +0 -0
        test_cmp expect actual
 '

Prathamesh Chavan

author

2017-03-09 19:29 UTC #9 lore raw

PATCH 1/1 t2027: avoid using pipes

From: Prathamesh <[email protected]>

Signed-off-by Prathamesh <[email protected]>

---
 t/t2027-worktree-list.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

t/t2027-worktree-list.sh +11 -7

index 848da5f36..d8b3907e0 100755
@@ -31,7 +31,8 @@ test_expect_success '"list" all worktrees from main' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git worktree list | sed "s/  */ /g" >actual &&
+	git worktree list >out &&
+	sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -40,7 +41,8 @@ test_expect_success '"list" all worktrees from linked' '
 	test_when_finished "rm -rf here && git worktree prune" &&
 	git worktree add --detach here master &&
 	echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C here worktree list | sed "s/  */ /g" >actual &&
+	git -C here worktree list >out &&
+	sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -73,7 +75,8 @@ test_expect_success '"list" all worktrees from bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C bare1 worktree list | sed "s/  */ /g" >actual &&
+	git -C bare1 worktree list >out &&
+	sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -96,7 +99,8 @@ test_expect_success '"list" all worktrees from linked with a bare main' '
 	git -C bare1 worktree add --detach ../there master &&
 	echo "$(pwd)/bare1 (bare)" >expect &&
 	echo "$(git -C there rev-parse --show-toplevel) $(git -C there rev-parse --short HEAD) (detached HEAD)" >>expect &&
-	git -C there worktree list | sed "s/  */ /g" >actual &&
+	git -C there worktree list >out &&
+	sed "s/  */ /g" <out >actual &&
 	test_cmp expect actual
 '

@@ -118,9 +122,9 @@ test_expect_success 'broken main worktree still at the top' '
 		cd linked &&
 		echo "worktree $(pwd)" >expected &&
 		echo "ref: .broken" >../.git/HEAD &&
-		git worktree list --porcelain | head -n 3 >actual &&
+		git worktree list --porcelain >out && head -n 3 out >actual &&
 		test_cmp ../expected actual &&
-		git worktree list | head -n 1 >actual.2 &&
+		git worktree list >out && head -n 1 out >actual.2 &&
 		grep -F "(error)" actual.2
 	)
 '

@@ -134,7 +138,7 @@ test_expect_success 'linked worktrees are sorted' '
 		test_commit new &&
 		git worktree add ../first &&
 		git worktree add ../second &&
-		git worktree list --porcelain | grep ^worktree >actual
+		git worktree list --porcelain >out && grep ^worktree out >actual
 	) &&
 	cat >expected <<-EOF &&
 	worktree $(pwd)/sorted/main

Signature · 1 line

2.11.0

Prathamesh Chavan

author

2017-03-10 13:34 UTC #10 lore raw

In reply to Christian Couder

Quoted 11 lines · Christian Couder

Quoted 8 lines · Prathamesh Chavan
Whenever a git command is present in the upstream of a pipe, its failure gets masked by piping and hence it should be avoided for testing the upstream git command. By writing out the output of the git command to a file, we can test the exit codes of both the commands as a failure exit code in any command is able to stop the && chain.
Signed-off-by Prathamesh <[email protected]>
---
Please add in Cc those who previously commented on the patch.

Actually I initially used submitGit to send patches, where there was no option of
adding cc to the patch. But after your comment I have switched to git send-email and git format-patch for sending patches.

Quoted 23 lines

Quoted 9 lines

 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

t/t2027-worktree-list.sh +0 -0

index 848da5f..daa7a04 100755

t/t2027-worktree-list.sh test_expect_success '"list" all worktrees from main' ' · line 34

Quoted 5 lines

t/t2027-worktree-list.sh +1 -1

@@ -31,7 +31,7 @@ test_expect_success '"list" all worktrees from main' '
        test_when_finished "rm -rf here && git worktree prune" &&
        git worktree add --detach here master &&
        echo "$(git -C here rev-parse --show-toplevel) $(git rev-parse --short HEAD) (detached HEAD)" >>expect &&
-       git worktree list | sed "s/  */ /g" >actual &&
+       git worktree list >out && sed "s/  */ /g" <out >actual &&

I still think that it would be better if the 'sed' commend was on its own line like this:

(quoted diff) +2 -0

+       git worktree list >out &&
+       sed "s/  */ /g" <out >actual &&

Quoted 3 lines

t/t2027-worktree-list.sh +0 -0
        test_cmp expect actual
 '