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

>
> On Sun, Oct 6, 2024 at 2:31 AM Usman Akinyemi
> <usmanakinyemi202@gmail.com> wrote:
> > On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> > > 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).
> > >
> > > 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().
> >
> > 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 ?
>
> I'm not sure which three patches you mean. Does the third patch, the
> "new one on the same branch", fix problems from the earlier two
> patches? Or does your third patch implement the suggestion regarding
> test_line_count()?
>

> > 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 ?
>
> A patch series consists of one or more patches in sequence. Patches
> within a series don't conflict with one another; later patches build
> upon earlier patches. You create a series of commits on a single Git
> branch. When you submit that branch as a pull-request via
> GitGitGadget, it turns the commits on that branch into a series of
> patch emails to the Git mailing list, one per commit.
>
> Before submitting the patches via GitGitGadget, you polish them
> locally to repair any problems before submitting them for review.
> Rather than making subsequent commits fix problems with earlier
> commits, you instead should fix the bad commits by using "git rebase
> --interactive ..." to either "fixup", "squash", or otherwise edit the
> bad commits. Once you are happy with the commits, submit them. This
> way, reviewers only see your polished result, not the series of steps
> you made to arrive at the polished results.
>
> You do the same thing after reviewers point out problems or ask for
> changes. Edit and re-polish the existing commits to address reviewer
> comments rather than merely making new commits on top of the existing
> commits, and then resubmit once all the fixes have been applied and
> polished.
>
> When I suggested squashing your two original commits it was for the
> above reason. In your original submission, patch [1/2] had some
> problems which you fixed in patch [2/2], but reviewers don't need or
> want to see that; they just want to see the polished end-result, which
> you can obtain by squashing the two patches together. (However, in
> this case, as I pointed out in my review, you don't even need to use
> `tr`; just use `test 1 = $count` instead of `test 1 = "$count"`.)
>
> If you wanted to do the extra step of also updating the tests to use
> test_line_count(), then that would be a separate patch, still on the
> same branch, built on top of your "fix Git upstream of pipe" patch.
> Thus, it would become a two-patch series: patch [1/2] fixing Git
> upstream of a pipe, and [2/2] employing test_line_count().
>

> > Also, to be clear, "Description" is the body of the commit message if
> > I use the gitgitgadget while the "commit message" is the header ?
>
> The commit message is separate from the patch-series commentary. The
> commit message of each patch explains what that patch changes or does.
>
> Once you have polished your commit(s), force-push them to the
> GitGitGadget pull-request you already created. Then edit the very
> first (topmost) comment in the pull-request to explain what the patch
> series is about and what you changed since the previous version. That
> comment becomes the "commentary" portion of the patch series.