[PATCH] t2027: avoid using pipes

[PATCH] t2027: avoid using pipes

[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 <pc44800@gmail.com>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f..daa7a04 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -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

Re: [PATCH] t2027: avoid using pipes

[Jon Loeliger]

So, like, Prathamesh Chavan said:
> 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 <pc44800@gmail.com>
> ---
>  t/t2027-worktree-list.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f..daa7a04 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -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
>  '

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

Re: [PATCH] t2027: avoid using pipes

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

On Wed, Mar 8, 2017 at 9:14 PM, Jon Loeliger <jdl@jdl.com> wrote:
> So, like, Prathamesh Chavan said:
>> 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 <pc44800@gmail.com>
>> ---
>>  t/t2027-worktree-list.sh | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>> index 848da5f..daa7a04 100755
>> --- a/t/t2027-worktree-list.sh
>> +++ b/t/t2027-worktree-list.sh
>> @@ -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
>>  '
>
> 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

Re: [PATCH] t2027: avoid using pipes

[Prathamesh Chavan]

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.

On Thu, Mar 9, 2017 at 3:08 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> 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.
>
> On Wed, Mar 8, 2017 at 9:14 PM, Jon Loeliger <jdl@jdl.com> wrote:
>> So, like, Prathamesh Chavan said:
>>> 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 <pc44800@gmail.com>
>>> ---
>>>  t/t2027-worktree-list.sh | 14 +++++++-------
>>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>>> index 848da5f..daa7a04 100755
>>> --- a/t/t2027-worktree-list.sh
>>> +++ b/t/t2027-worktree-list.sh
>>> @@ -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
>>>  '
>>
>> 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

Re: [PATCH] t2027: avoid using pipes

[Christian Couder]

On Wed, Mar 8, 2017 at 4:13 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> 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.

> 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 <pc44800@gmail.com>
> ---
>  t/t2027-worktree-list.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f..daa7a04 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -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...

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

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

Re: [PATCH] t2027: avoid using pipes

[Prathamesh Chavan]

On Thu, Mar 9, 2017 at 1:38 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Wed, Mar 8, 2017 at 4:13 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> 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.

>
>> 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 <pc44800@gmail.com>
>> ---
>>  t/t2027-worktree-list.sh | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>> index 848da5f..daa7a04 100755
>> --- a/t/t2027-worktree-list.sh
>> +++ b/t/t2027-worktree-list.sh
>> @@ -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...
>

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.


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

[PATCH v2] t2027: avoid using pipes

[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 <pc44800@gmail.com>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f..daa7a04 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -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

[PATCH v2] t2027: avoid using pipes

[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 <pc44800@gmail.com>
---
 t/t2027-worktree-list.sh | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f..daa7a04 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -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

Re: [PATCH v2] t2027: avoid using pipes

[Christian Couder]

On Thu, Mar 9, 2017 at 10:53 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> 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 <pc44800@gmail.com>
> ---

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

>  t/t2027-worktree-list.sh | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f..daa7a04 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -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 &&

>         test_cmp expect actual
>  '

[PATCH] t2027: avoid using pipes

[Prathamesh Chavan]

From: Prathamesh <pc44800@gmail.com>

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 <pc44800@gmail.com>
---
 t/t2027-worktree-list.sh | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f36..d8b3907e0 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -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
-- 
2.11.0

[Patch v2] t2027: avoid using pipes

[Prathamesh Chavan]

From: Prathamesh <pc44800@gmail.com>

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 <pc44800@gmail.com>
---
New version of patch updated to include suggested change of add the git
command which was above the grep command on a new line. Also some more
changes similar to the above change were made.
Another reason for a newer version in to improvise the previous mistake
of not including the patch version, as well as getting more familiar with
the submitting patch process.


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

diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
index 848da5f36..a3e77fee5 100755
--- a/t/t2027-worktree-list.sh
+++ b/t/t2027-worktree-list.sh
@@ -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,11 @@ 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 +140,8 @@ 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
-- 
2.11.0

Re: [PATCH v2] t2027: avoid using pipes

[Prathamesh Chavan]

On Thu, Mar 9, 2017 at 6:00 PM, Christian Couder
<christian.couder@gmail.com> wrote:
> On Thu, Mar 9, 2017 at 10:53 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> 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 <pc44800@gmail.com>
>> ---
>
> 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.

>
>>  t/t2027-worktree-list.sh | 14 +++++++-------
>>  1 file changed, 7 insertions(+), 7 deletions(-)
>>
>> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
>> index 848da5f..daa7a04 100755
>> --- a/t/t2027-worktree-list.sh
>> +++ b/t/t2027-worktree-list.sh
>> @@ -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 &&
>
>>         test_cmp expect actual
>>  '

Re: [Patch v2] t2027: avoid using pipes

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> From: Prathamesh <pc44800@gmail.com>

Your e-mail header says 

	From: Prathamesh Chavan <pc44800@gmail.com>

and you probably have "[user] name = Prathamesh" somewhere in your
config, and I think that is why we see the above line in format-patch
output and on your sign-off.  If you really prefer to be known as a
person with a single name (without Chavan) to the community, that is
fine, but if that is not the case, perhaps you'd want to review your
config and fix this.

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

OK.

> Signed-off-by: Prathamesh <pc44800@gmail.com>

> diff --git a/t/t2027-worktree-list.sh b/t/t2027-worktree-list.sh
> index 848da5f36..a3e77fee5 100755
> --- a/t/t2027-worktree-list.sh
> +++ b/t/t2027-worktree-list.sh
> @@ -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
>  '

We'll have "out" as a new leftover file, but it probably would not
make too much of a difference.  We already leave 'expect' and 'actual'
in the working tree as known crufts.  

Just FYI, if you want to clean them, there is already when_finished
in this piece (and others) so you could do

    -test_when_finished "rm -rf here && git worktree prune" &&
    -test_when_finished "rm -rf here out actual expect && git worktree prune" &&

When a test fails, when_finished is not run, so this will not
interfere with necessary debugging effort when/if somebody breaks
"git worktree" and this test starts failing.