Re: [Outreachy][PATCH v3] t2400: avoid using pipes

[Junio C Hamano <gitster@pobox.com>]

Achu Luma <ach.lumap@gmail.com> writes:

> Subject: Re: [Outreachy][PATCH v3] t2400: avoid using pipes

"avoid using pipes" is a means to an end.  And it is more important
to tell readers what that "end" is.  With this patch, what are we
trying to achieve?  Cater to platforms that lack pipes?  Help
platforms that cannot run two processes at the same time, so let one
run and store the result in a file, and then let the other one run,
to reduce the CPU load?

If we run a "git" command, especially a command we are testing, on
the upstream side of a pipe, we lose information.  We cannot tell
what exit status the command exited with.  That is what we care
about.

So, it is better to say that in the title, e.g.,

    Subject: [PATCH] t2400: avoid losing exit status to pipes

> The exit code of the preceding command in a pipe is disregarded,
> so it's advisable to refrain from relying on it.

It is unclear what "it" refers to here.  We cannot rely on the exit
code of the command on the upstream side of a pipe, obviously.

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

Surely.  I personally think that the title that says what the
purpose of the patch is clearly should be sufficient without any
further description in the body, though.
>
> Signed-off-by: Achu Luma <ach.lumap@gmail.com>
> ---
>  Since v2 I don't send a cover  letter anymore, and I changed 
>  my "Signed-of-by: ..." line so that it
>  contains my full real name and I added "Outreachy" to the subject.

Nicely done.

>
>  t/t2400-worktree-add.sh | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> 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
>  	)