From: AbdAlRahman Gad <abdobngad@gmail.com>
Date: Tue, 06 Aug 2024 08:38:33 GMT
Subject: Re: [PATCH v4 2/8] [Newcomer] t7004: Do not lose exit status to pipe
Message-ID: <3cba795c-631b-4e86-a1f2-ddefe76ef0f4@gmail.com>
In-Reply-To: <CAPig+cS6-gqZbJSz3wBcummyycJVudKybVZW=D33WHk-Fa9ytQ@mail.gmail.com>



On 8/6/24 06:13, Eric Sunshine wrote:
> On Mon, Aug 5, 2024 at 8:00 PM AbdAlRahman Gad <abdobngad@gmail.com> wrote:
>> Split "test-tool ... | sed" pipeline into two commands to avoid losing
>> exit status from test-tool.
>>
>> Signed-off-by: AbdAlRahman Gad <abdobngad@gmail.com>
>> ---
>> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
>> @@ -97,7 +97,8 @@ test_expect_success 'creating a tag with --create-reflog should create reflog' '
>> -       test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 | sed -e "s/^.* //" >actual &&
>> +       test-tool ref-store main for-each-reflog-ent refs/tags/tag_with_reflog1 >actual.body &&
>> +       sed -e "s/^.*   //" actual.body >actual &&
> 
> It's not just `test_tool` we care about; we also (importantly) don't
> want to see `git` itself upstream of a pipe, and there are many such
> instances remaining in this script. Here are some common examples:
> 
>      test $(git tag -l | wc -l) -eq 0 &&
>      git cat-file tag "$1" | sed -e "/BEGIN PGP/q"
>      git tag -l | grep "^tag-one-line" >actual &&
>      forged=$(git cat-file tag ... | sed -e ... | git mktag) &&
>      git tag -l --no-sort "foo*" | sort >actual &&
> 
> By the way, these days, rather than:
> 
>      test $(git tag -l | wc -l) -eq 0 &&
> 
> we would say:
> 
>      test_stdout_line_count = 0 git tag -l &&
> 
> which nicely avoids placing `git` upstream of a pipe.

Thanks for the review. Could this be done in a separate patch series? As 
modifying a patch in the beginning of a patch series requires lots of 
time to adapt the rest of the series to the change.