On Sun, Oct 6, 2024 at 5:48 AM Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > > On Sun, Oct 6, 2024 at 1:33 AM Usman Akinyemi via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: > > Changes since v1: > > - Added "tr -d '[:space:]'" to handle whitespace on macOS > > > > Signed-off-by: Usman Akinyemi <usmanakinyemi202@xxxxxxxxx> > > --- > > 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.
