Re: [PATCH v2] [Outreachy][Patch v1] t3404: avoid losing exit status to pipes

From: shejialuo <shejialuo@gmail.com>
Date: Oct 6, 2024, 13:03 UTC
Message-ID: <ZwKKhzbeyQ5e_H9Q@ArchLinux>
In-Reply-To: <CAPSxiM-bx7KdhP0OyajfiTczg-WqiJDPso1EAxLzntLqcuOUkA@mail.gmail.com>

On Sun, Oct 06, 2024 at 12:28:26PM +0000, Usman Akinyemi wrote:

> I am a bit confused now, I am already working on converting the
> test_line_count right now. Can I update the patch or do something
> regarding the first patch ?

Hi Usman:

I have just scanned the review from Eric.

> These days, instead of manually using `wc -l` and `test`, we would
> instead write:

>    grep ONCE output >actual &&
>    test_line_count 1 actual

Show 5 quoted lines

> However, that sort of change is independent of the purpose of this
> patch, so you probably should not make such a change in this patch. If
> you're up to it, you could instead turn this into a two-patch series
> in which patch [1/2] fixes the "Git upstream of a pipe" problem, and
> then patch [2/2] converts these cases to use test_line_count().

If you decide to do this. As Eric has commented in [1], you should add a new commit (a new patch) followed by current patch to convert to the `test_line_count`. Then you should re-send the series to the mailing list. And thus you could enhance the commit message of the first patch. If you do not decide to do this (the current patch is enough for the microproject), you don't need to reroll for the minor things.

So, you should never update the current patch for converting the test using `test_line_count`. Instead, create a new commit to do this. and BTW you could change the commit message of the first patch if you want.

Thanks, Jialuo

[1] https://lore.kernel.org/git/CAPig+cTb4mgpXnN79UrXvjvCnqGZhaR51oZX_Ds=HwdqQYFN9w@mail.gmail.com/