| Message ID | 4199434bd6ef2142192d1c720ccd877b1a80536b.1728192815.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | t3404: avoid losing exit status to pipes | expand |
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().
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.
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.
On Sun, Oct 6, 2024 at 7:28 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > 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()? > 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.
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 33ea1f05e2c..3d8de69d6ee 100755 --- 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' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep ONCE output | wc -l) && + count=$(grep ONCE output | wc -l | tr -d '[:space:]') && test 1 = "$count" ' @@ -416,7 +416,7 @@ test_expect_success 'multi-fixup does not fire up editor' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep NEVER output | wc -l) && + count=$(grep NEVER output | wc -l | tr -d '[:space:]') && test 0 = "$count" && git checkout @{-1} && git branch -D multi-fixup @@ -436,7 +436,7 @@ test_expect_success 'commit message used after conflict' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep ONCE output | wc -l) && + count=$(grep ONCE output | wc -l | tr -d '[:space:]') && test 1 = "$count" && git checkout @{-1} && git branch -D conflict-fixup @@ -456,7 +456,7 @@ test_expect_success 'commit message retained after conflict' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep TWICE output | wc -l) && + count=$(grep TWICE output | wc -l | tr -d '[:space:]') && test 2 = "$count" && git checkout @{-1} && git branch -D conflict-squash @@ -501,7 +501,7 @@ test_expect_success 'squash ignores comments' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep ONCE output | wc -l) && + count=$(grep ONCE output | wc -l | tr -d '[:space:]') && test 1 = "$count" && git checkout @{-1} && git branch -D skip-comments @@ -519,7 +519,7 @@ test_expect_success 'squash ignores blank lines' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep ONCE output | wc -l) && + count=$(grep ONCE output | wc -l | tr -d '[:space:]') && test 1 = "$count" && git checkout @{-1} && git branch -D skip-blank-lines