Re: [PATCH 2/3] t3404: replace test with test_line_count()
From: Taylor Blau <hidden>
Date: 2024-10-14 21:35:48
On Sat, Oct 12, 2024 at 11:09:33PM +0000, Usman Akinyemi via GitGitGadget wrote:
quoted hunk ↗ jump to hunk
From: Usman Akinyemi <redacted> Refactor t3404 to replace instances of `test` with `test_line_count()` for checking line counts. This improves readability and aligns with Git's current test practices. Signed-off-by: Usman Akinyemi <redacted> --- t/t3404-rebase-interactive.sh | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 96a65783c47..2ab660ef30f 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh@@ -281,8 +281,9 @@ test_expect_success 'stop on conflicting pick' ' test_cmp expect2 file1 && test "$(git diff --name-status | sed -n -e "/^U/s/^U[^a-z]*//p")" = file1 && - test 4 = $(grep -v "^#" < .git/rebase-merge/done | wc -l) && - test 0 = $(grep -c "^[^#]" < .git/rebase-merge/git-rebase-todo) + grep -v "^#" <.git/rebase-merge/done >actual && + test_line_count = 4 actual && + test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
You use 'test_line_count' in one instance here, but 'test 0 =' below.
You could use 'test_must_be_empty' to stick with our test_-helper
functions.
But I like that you used 'grep -c' here, so it may be better to match
the two like so:
test 4 $(grep -c -v "^#" <.git/rebase-merge/done) &&
test 0 = $(grep -c "^[^#]" <.git/rebase-merge/git-rebase-todo)
quoted hunk ↗ jump to hunk
@@ -416,8 +417,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) && - test 0 = $count && + ! grep NEVER output && git checkout @{-1} && git branch -D multi-fixup '
Hmm. Wasn't this modified by the previous step as well? Is there a reason that these can't be combined to avoid a new intermediate state that will be thrown away in the next step?
quoted hunk ↗ jump to hunk
@@ -436,8 +436,8 @@ test_expect_success 'commit message used after conflict' ' ) && test $base = $(git rev-parse HEAD^) && git show >output && - count=$(grep ONCE output | wc -l) && - test 1 = $count && + grep ONCE output >actual && + test_line_count = 1 actual && git checkout @{-1} && git branch -D conflict-fixup
I am not sure what the benefit of using test_line_count here is over
bare 'test'. Can you explain why you chose to use it here?
In the body of your patch above, you appear to suggest that using
test_line_count is more in the style of Git's current test practices. I
think that's true for cases like writing:
test_line_count = 1 actual
as opposed to:
test 1 = $(wc -l <actual)
Since the former doesn't have the gotcha that you must remember redirect
the input of 'wc -l' to avoid having the filename appear in the output,
and the former also ensures that the file exists, has better error
messages, etc.
But in the case where we're running 'grep -c' directly, it seems cleaner
to use bare test, since we're not writing the matches to a file on disk.
Thanks,
Taylor