[1/1] t2400: avoid using pipes

Commit Message

Achu Luma Oct. 3, 2023, 5:48 p.m. UTC

From: achluma <[e-mail]>

The exit code of the preceding command in a pipe is disregarded,
so it's advisable to refrain from relying on it. Instead, by
saving the output of a Git command to a file, we gain the
ability to examine the exit codes of both commands separately.

Signed-off-by: achluma <[e-mail]>
---
 t/t2400-worktree-add.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Eric Sunshine Oct. 3, 2023, 6:01 p.m. UTC | #1

On Tue, Oct 3, 2023 at 1:49 PM <[e-mail]> wrote:
> t2400: avoid using pipes

Pipes themselves are not necessarily problematic, and there are many
places in the test suite where they are legitimately used. Rather...

> The exit code of the preceding command in a pipe is disregarded,
> so it's advisable to refrain from relying on it. Instead, by
> saving the output of a Git command to a file, we gain the
> ability to examine the exit codes of both commands separately.

... as you correctly explain here, we don't want to lose the exit code
from the Git command. Thus, if you want to convey more information to
readers of `git log --oneline` (or other such commands), a better
subject for the patch might be:

    t2400: avoid losing Git exit code

That minor comment aside (which is probably not worth a reroll), the
commit message properly explains why this change is desirable and the
patch itself looks good.

> Signed-off-by: achluma <[e-mail]>
> ---
> diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
> @@ -468,7 +468,8 @@ test_expect_success 'put a worktree under rebase' '
>                 cd under-rebase &&
>                 set_fake_editor &&
>                 FAKE_LINES="edit 1" git rebase -i HEAD^ &&
> -               git worktree list | grep "under-rebase.*detached HEAD"
> +               git worktree list >actual &&

Thanks for following the style guideline and omitting whitespace
between the redirection operator and the destination file.

> +               grep "under-rebase.*detached HEAD" actual
>         )
>  '
>
> @@ -509,7 +510,8 @@ test_expect_success 'checkout a branch under bisect' '
>                 git bisect start &&
>                 git bisect bad &&
>                 git bisect good HEAD~2 &&
> -               git worktree list | grep "under-bisect.*detached HEAD" &&
> +               git worktree list >actual &&
> +               grep "under-bisect.*detached HEAD" actual &&
>                 test_must_fail git worktree add new-bisect under-bisect &&
>                 ! test -d new-bisect
>         )

Junio C Hamano Oct. 3, 2023, 6:42 p.m. UTC | #2

Eric Sunshine <[e-mail]> writes:

> On Tue, Oct 3, 2023 at 1:49 PM <[e-mail]> wrote:
>> t2400: avoid using pipes
>
> Pipes themselves are not necessarily problematic, and there are many
> places in the test suite where they are legitimately used. Rather...
> ...
> readers of `git log --oneline` (or other such commands), a better
> subject for the patch might be:
>
>     t2400: avoid losing Git exit code
>
> That minor comment aside (which is probably not worth a reroll), the
> commit message properly explains why this change is desirable and the
> patch itself looks good.

Thanks for writing and reviewing.  Will queue.

Patch

diff --git a/t/t2400-worktree-add.sh b/t/t2400-worktree-add.sh
index df4aff7825..7ead05bb98 100755
--- a/t/t2400-worktree-add.sh
+++ b/t/t2400-worktree-add.sh
@@ -468,7 +468,8 @@  test_expect_success 'put a worktree under rebase' '
 		cd under-rebase &&
 		set_fake_editor &&
 		FAKE_LINES="edit 1" git rebase -i HEAD^ &&
-		git worktree list | grep "under-rebase.*detached HEAD"
+		git worktree list >actual && 
+		grep "under-rebase.*detached HEAD" actual
 	)
 '
 
@@ -509,7 +510,8 @@  test_expect_success 'checkout a branch under bisect' '
 		git bisect start &&
 		git bisect bad &&
 		git bisect good HEAD~2 &&
-		git worktree list | grep "under-bisect.*detached HEAD" &&
+		git worktree list >actual && 
+		grep "under-bisect.*detached HEAD" actual &&
 		test_must_fail git worktree add new-bisect under-bisect &&
 		! test -d new-bisect
 	)