On Sun, Oct 6, 2024 at 7:28 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > 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()? > Thanks, I really appreciate your explanation. Thank you very much. By the third patch, I meant the new one which I will be adding. > > 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(). > Thanks for this. I will submit the first patch, get feedback then approach the second one. > > 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.
