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().
