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

From: Usman Akinyemi <usmanakinyemi202@gmail.com>
Date: Oct 6, 2024, 13:27 UTC
Message-ID: <CAPSxiM_+LCH04ZbTZm8oZWDc=Mz2JNJ_Uw7u+gO4e_ie4yPw5g@mail.gmail.com>
In-Reply-To: <ZwKKhzbeyQ5e_H9Q@ArchLinux>

Thanks very much Jialuo,

My understanding of this, kindly tell me if I am wrong.

1. Make a new commit for test_line_count on the same branch right ?
2. I should remove the "Outreachy][Patch v1]" in the first patch to
enhance the commit message right?

Thank you.

On Sun, Oct 6, 2024 at 1:02 PM shejialuo <shejialuo@gmail.com> wrote:

Show 37 quoted lines

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