Re: [Outreachy][PATCH v3] t2400: avoid using pipes
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 8, 2023, 21:00 UTC
- Message-ID
- <xmqqr0jw1kbq.fsf@gitster.g>
- In-Reply-To
- <20231204153740.2992-1-ach.lumap@gmail.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.
Show 6 quoted lines
> > 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.
Show 28 quoted lines
> > 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 > )