Subject: Re: [PATCH] t7004: replace wc -l with modern test helpers

From: Junio C Hamano <[email-masked]>
Date: Wed, 01 Apr 2026 11:56:24 -0700

Siddharth Shrimali <[email-masked]> writes:

> Pipelines of the form "test $(git tag | wc -l) -eq 0" suppress git's
> exit code. This means a crash or unexpected failure from git tag would
> go undetected. Additionally, the use of $(...) creates a subshell for
> each check, which adds unnecessary overhead.
>
> Replace these patterns with test_must_be_empty and test_line_count.
> These helpers check the output of git directly from a file, ensuring
> git's exit code is captured properly via the preceding "&&" chain.
> They also provide better diagnostics on failure by printing the
> contents of the file when a check does not pass.
>
> Signed-off-by: Siddharth Shrimali <[email-masked]>
> ---
>  t/t7004-tag.sh | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)

They all look the result of good and mechanical transformation.
Will queue.

Some tests, however, may want to be cleaned up on top in a follow-up
patch.

For example, taking a look at this one:

>  test_expect_success 'trying to create a tag with a non-valid name should fail' '
> -	test $(git tag -l | wc -l) -eq 1 &&
> +	git tag -l >actual &&
> +	test_line_count = 1 actual &&
>  	test_must_fail git tag "" &&
>  	test_must_fail git tag .othertag &&
>  	test_must_fail git tag "other tag" &&
>  	test_must_fail git tag "othertag^" &&
>  	test_must_fail git tag "other~tag" &&
> -	test $(git tag -l | wc -l) -eq 1
> +	git tag -l >actual &&
> +	test_line_count = 1 actual
>  '

it is dubious to insist that we have a single tag at the beginning
of the test, as more tests can be added before this step, or some
tests may be retired before this step, changing the expected state
before we start this step.  As the test title says, all the tag
creation tests that we mark with test_must_fail are very relevant
to this one, but the last one that counts the existing tag again is
pointless, as it adds an unnecessary dependency on the state left by
previous tests (i.e., if we start with a single tag in the repository,
and we expect all the tag creation in this test fail, then we should
end up with a single tag in the repository---but this means that every
time a future change makes previous tests leave no tag or two or
more tags before this test runs, this test needs to be updated to
expect different number of tags).

I didn't bother looking at all the other tests outside what was
visible in the patch, so please do not just react on this comment
and update only this test.  If we were to make such a clean up that
is more than mechanical rewrite, it should be done with a better
understanding of the whole picture of the test script, not just what
I happened to have noticed within the context of this patch that
changed something unrelated (i.e., use of "| wc -l").

Thanks.