On Sun, Oct 6, 2024 at 2:31 AM Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> wrote: > On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> 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.
