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

[Usman Akinyemi <usmanakinyemi202@gmail.com>]

On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
>
> On Sun, Oct 6, 2024 at 1:33 AM Usman Akinyemi via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > Changes since v1:
> > - Added "tr -d '[:space:]'" to handle whitespace on macOS
> >
> > Signed-off-by: Usman Akinyemi <usmanakinyemi202@gmail.com>
> > ---
>
> Thanks for the submission. A few comments...
>
> This second patch fixes problems with the first patch, but since this
> is an entirely new submission, you should instead "squash" these two
> patches together and then force-push them to the same branch that you
> used when submitting them via GitGitGadget, and re-submit them as a
> single patch. When you squash them, keep the commit message from the
> first patch.
>
> Reviewers do appreciate that you explained what changed since the
> previous version, but we'd like to see that information as commentary
> in the patch cover letter, not as the commit message of the patch
> itself. In GitGitGadget, the way you would do so is to write this as
> the "Description" of the pull-request (possibly replacing or amending
> the previous description).
>
> Some more observations below...
>
> > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> > @@ -401,7 +401,7 @@ test_expect_success 'multi-squash only fires up editor once' '
> >         git show >output &&
> > -       count=$(grep ONCE output | wc -l) &&
> > +       count=$(grep ONCE output | wc -l | tr -d '[:space:]') &&
> >         test 1 = "$count"
> >  '
>
> The reason this was failing for you was because you quoted $count. Had
> you instead written:
>
>     test 1 = $count
>
> when it would have worked as expected. In other words, you don't need `tr`.
>
> 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().

Hi  Eric Sunshine,
thanks for the review. I really appreciate it. I have a couple of
doubts to clear.

My next patch should be the squash of my three patches which include
my first two patches and the new one on the same branch ?
Two patch series means two different commits on different patches ?
But, since they both depend on each other would not they lead to merge
conflict ?
Also, to be clear, "Description" is the body of the commit message if
I use the gitgitgadget while the "commit message" is the header ?

Thank you.