Re: [PATCH 2/3] t3404: replace test with test_line_count()

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 12, 2024 at 11:09:33PM +0000, Usman Akinyemi via GitGitGadget wrote:
> From: Usman Akinyemi <usmanakinyemi202@gmail.com>
>
> 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 <usmanakinyemi202@gmail.com>
> ---
>  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)

> @@ -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?

> @@ -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