{"thread":{"id":"65403","subject":"[PATCH] t7004: replace wc -l with modern test helpers","startedAt":"2026-04-01T06:20:38Z","lastAt":"2026-04-01T18:56:27Z","messageCount":2,"participants":["Siddharth Shrimali","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"540618","messageId":"20260401062029.54757-1-r.siddharth.shrimali@gmail.com","threadId":"65403","inReplyTo":null,"subject":"[PATCH] t7004: replace wc -l with modern test helpers","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-01T06:20:29Z","receivedAt":"2026-04-01T06:20:38Z","isPatch":true,"body":"Pipelines of the form \"test $(git tag | wc -l) -eq 0\" suppress git's\nexit code. This means a crash or unexpected failure from git tag would\ngo undetected. Additionally, the use of $(...) creates a subshell for\neach check, which adds unnecessary overhead.\n\nReplace these patterns with test_must_be_empty and test_line_count.\nThese helpers check the output of git directly from a file, ensuring\ngit's exit code is captured properly via the preceding \"&&\" chain.\nThey also provide better diagnostics on failure by printing the\ncontents of the file when a check does not pass.\n\nSigned-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n---\n t/t7004-tag.sh | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex ce2ff2a28a..faf7d97fc4 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -33,8 +33,10 @@ test_expect_success 'listing all tags in an empty tree should succeed' '\n '\n \n test_expect_success 'listing all tags in an empty tree should output nothing' '\n-\ttest $(git tag -l | wc -l) -eq 0 &&\n-\ttest $(git tag | wc -l) -eq 0\n+\tgit tag -l >actual &&\n+\ttest_must_be_empty actual &&\n+\tgit tag >actual &&\n+\ttest_must_be_empty actual\n '\n \n test_expect_success 'sort tags, ignore case' '\n@@ -178,7 +180,8 @@ test_expect_success 'listing tags using a non-matching pattern should succeed' '\n '\n \n test_expect_success 'listing tags using a non-matching pattern should output nothing' '\n-\ttest $(git tag -l xxx | wc -l) -eq 0\n+\tgit tag -l xxx >actual &&\n+\ttest_must_be_empty actual\n '\n \n # special cases for creating tags:\n@@ -188,13 +191,15 @@ test_expect_success 'trying to create a tag with the name of one existing should\n '\n \n test_expect_success 'trying to create a tag with a non-valid name should fail' '\n-\ttest $(git tag -l | wc -l) -eq 1 &&\n+\tgit tag -l >actual &&\n+\ttest_line_count = 1 actual &&\n \ttest_must_fail git tag \"\" &&\n \ttest_must_fail git tag .othertag &&\n \ttest_must_fail git tag \"other tag\" &&\n \ttest_must_fail git tag \"othertag^\" &&\n \ttest_must_fail git tag \"other~tag\" &&\n-\ttest $(git tag -l | wc -l) -eq 1\n+\tgit tag -l >actual &&\n+\ttest_line_count = 1 actual\n '\n \n test_expect_success 'creating a tag using HEAD directly should succeed' '\n-- \n2.51.2\n\n"},{"id":"540661","messageId":"xmqqldf6v7af.fsf@gitster.g","threadId":"65403","inReplyTo":"20260401062029.54757-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH] t7004: replace wc -l with modern test helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-01T18:56:24Z","receivedAt":"2026-04-01T18:56:27Z","isPatch":true,"body":"Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:\n\n> Pipelines of the form \"test $(git tag | wc -l) -eq 0\" suppress git's\n> exit code. This means a crash or unexpected failure from git tag would\n> go undetected. Additionally, the use of $(...) creates a subshell for\n> each check, which adds unnecessary overhead.\n>\n> Replace these patterns with test_must_be_empty and test_line_count.\n> These helpers check the output of git directly from a file, ensuring\n> git's exit code is captured properly via the preceding \"&&\" chain.\n> They also provide better diagnostics on failure by printing the\n> contents of the file when a check does not pass.\n>\n> Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>\n> ---\n>  t/t7004-tag.sh | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n\nThey all look the result of good and mechanical transformation.\nWill queue.\n\nSome tests, however, may want to be cleaned up on top in a follow-up\npatch.\n\nFor example, taking a look at this one:\n\n>  test_expect_success 'trying to create a tag with a non-valid name should fail' '\n> -\ttest $(git tag -l | wc -l) -eq 1 &&\n> +\tgit tag -l >actual &&\n> +\ttest_line_count = 1 actual &&\n>  \ttest_must_fail git tag \"\" &&\n>  \ttest_must_fail git tag .othertag &&\n>  \ttest_must_fail git tag \"other tag\" &&\n>  \ttest_must_fail git tag \"othertag^\" &&\n>  \ttest_must_fail git tag \"other~tag\" &&\n> -\ttest $(git tag -l | wc -l) -eq 1\n> +\tgit tag -l >actual &&\n> +\ttest_line_count = 1 actual\n>  '\n\nit is dubious to insist that we have a single tag at the beginning\nof the test, as more tests can be added before this step, or some\ntests may be retired before this step, changing the expected state\nbefore we start this step.  As the test title says, all the tag\ncreation tests that we mark with test_must_fail are very relevant\nto this one, but the last one that counts the existing tag again is\npointless, as it adds an unnecessary dependency on the state left by\nprevious tests (i.e., if we start with a single tag in the repository,\nand we expect all the tag creation in this test fail, then we should\nend up with a single tag in the repository---but this means that every\ntime a future change makes previous tests leave no tag or two or\nmore tags before this test runs, this test needs to be updated to\nexpect different number of tags).\n\nI didn't bother looking at all the other tests outside what was\nvisible in the patch, so please do not just react on this comment\nand update only this test.  If we were to make such a clean up that\nis more than mechanical rewrite, it should be done with a better\nunderstanding of the whole picture of the test script, not just what\nI happened to have noticed within the context of this patch that\nchanged something unrelated (i.e., use of \"| wc -l\").\n\nThanks.\n"}]}