{"thread":{"id":"65481","subject":"[PATCH 0/3] t7004: cleanup and modernize brittle tests","startedAt":"2026-04-14T14:18:41Z","lastAt":"2026-04-21T05:39:06Z","messageCount":12,"participants":["Siddharth Shrimali","Junio C Hamano","Patrick Steinhardt"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"541559","messageId":"20260414141828.27576-1-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":null,"subject":"[PATCH 0/3] t7004: cleanup and modernize brittle tests","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-14T14:18:25Z","receivedAt":"2026-04-14T14:18:41Z","isPatch":true,"body":"This patch series addresses some brittle testing patterns in t7004-tag.sh\n\nThe first patch follows a suggestion by Junio to remove redundant tag\ncounting in the invalid name test. The second and third patches extend\nthis cleanup to hardcoded global state and modernizing subshell patterns\nthat could otherwise suppress git exit codes.\n\n---\nThis series is a follow-up to my previous patch \"t7004: replace wc -l \nwith modern test helpers\". Thanks to Junio for the feedback on the last\npatch. I have applied those changes here and have gone through the rest\nof the file and fixed similar issues.\n\nSiddharth Shrimali (3):\n t7004: drop hardcoded tag count in invalid name test\n t7004: dynamically grab expected state in tests\n t7004: avoid subshells to capture git exit codes\n\n t/t7004-tag.sh | 43 +++++++++++++++++++++----------------------\n 1 file changed, 21 insertions(+), 22 deletions(-)\n\n-- \n2.51.2\n\n"},{"id":"541560","messageId":"20260414141828.27576-2-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260414141828.27576-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH 1/3] t7004: drop hardcoded tag count in invalid name test","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-14T14:18:26Z","receivedAt":"2026-04-14T14:18:48Z","isPatch":true,"body":"The test 'trying to create a tag with a non-valid name should fail',\nchecked that exactly one tag existed in the repository before and after\nattempting to create invalid tags.\n\nAs pointed out by Junio, this makes the test brittle by relying on a\nspecific global tag count. If future tests are added or removed before\nthis test, the expected state changes and this test would break for\ncompletely unrelated reasons.\n\nSince we already use 'test_must_fail' to guarantee that the invalid\ntags are rejected by Git, counting the tags before and after is redundant.\n\nDrop the 'test_line_count = 1' checks so the test doesn't rely on the\nexact number of tags left behind by earlier tests.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 6 +-----\n 1 file changed, 1 insertion(+), 5 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex faf7d97fc4..6ca5c75b57 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -191,15 +191,11 @@ 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-\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-\tgit tag -l >actual &&\n-\ttest_line_count = 1 actual\n+\ttest_must_fail git tag \"other~tag\"\n '\n \n test_expect_success 'creating a tag using HEAD directly should succeed' '\n-- \n2.51.2\n\n"},{"id":"541561","messageId":"20260414141828.27576-3-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260414141828.27576-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH 2/3] t7004: dynamically grab expected state in tests","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-14T14:18:27Z","receivedAt":"2026-04-14T14:18:54Z","isPatch":true,"body":"The tests for 'Multiple -l or --list options' and 'trying to delete\ntags without params', hardcodes that exactly one or two specific tags\n('myhead', 'mytag') exist in the repository.\n\nIf other tests are added, modified, or removed earlier in the script,\nthis expected global state will change, resulting in these tests to fail\nfor completely unrelated reasons.\n\nInstead of hardcoding the expected tags, dynamically grab the state\nof the repository before running the commands under test ('git tag -l'\nand 'git tag -d'), and verify that the output matches or remains\nunchanged afterward. This keeps the tests independent from the script's\noverall state.\n\nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 6ca5c75b57..4fdd47cd21 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -145,9 +145,7 @@ test_expect_success 'listing all tags if one exists should succeed' '\n '\n \n test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n-\tcat >expect <<-\\EOF &&\n-\tmytag\n-\tEOF\n+\tgit tag -l >expect &&\n \tgit tag -l -l >actual &&\n \ttest_cmp expect actual &&\n \tgit tag --list --list >actual &&\n@@ -223,12 +221,7 @@ test_expect_success 'trying to delete an unknown tag should fail' '\n '\n \n test_expect_success 'trying to delete tags without params should succeed and do nothing' '\n-\tcat >expect <<-\\EOF &&\n-\tmyhead\n-\tmytag\n-\tEOF\n-\tgit tag -l >actual &&\n-\ttest_cmp expect actual &&\n+\tgit tag -l >expect &&\n \tgit tag -d &&\n \tgit tag -l >actual &&\n \ttest_cmp expect actual\n-- \n2.51.2\n\n"},{"id":"541562","messageId":"20260414141828.27576-4-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260414141828.27576-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH 3/3] t7004: avoid subshells to capture git exit codes","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-14T14:18:28Z","receivedAt":"2026-04-14T14:19:00Z","isPatch":true,"body":"Several tests in t7004 use the 'test$(git ...) = ...' or the '! (git ...)'\nsubshell pattern. This swallows git's exit code. If git crashes\n(e.g. segmentation fault) the crash would go undetected, and the test\nwould fail due to a mismatch or an inverted exit code.\n\nModernize these tests by directly writing output to files(actual) and\nverifying them with 'test_cmp' or 'test_grep'. Replace subshell\nnegations with 'test_must_fail'. This way, if git crashes, the test\nfails immediately and clearly instead of hiding the error behind a\nstring mismatch.\n\nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 4fdd47cd21..e8c59c9105 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -155,8 +155,10 @@ test_expect_success 'Multiple -l or --list options are equivalent to one -l opti\n '\n \n test_expect_success 'listing all tags if one exists should output that tag' '\n-\ttest $(git tag -l) = mytag &&\n-\ttest $(git tag) = mytag\n+\tgit tag -l >actual &&\n+\ttest_grep \"^mytag$\" actual &&\n+\tgit tag >actual &&\n+\ttest_grep \"^mytag$\" actual\n '\n \n # pattern matching:\n@@ -166,11 +168,15 @@ test_expect_success 'listing a tag using a matching pattern should succeed' '\n '\n \n test_expect_success 'listing a tag with --ignore-case' '\n-\ttest $(git tag -l --ignore-case MYTAG) = mytag\n+\techo mytag >expect &&\n+\tgit tag -l --ignore-case MYTAG >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'listing a tag using a matching pattern should output that tag' '\n-\ttest $(git tag -l mytag) = mytag\n+\techo mytag >expect &&\n+\tgit tag -l mytag >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'listing tags using a non-matching pattern should succeed' '\n@@ -427,8 +433,12 @@ test_expect_success 'listing tags -n in column with column.ui ignored' '\n \n test_expect_success 'a non-annotated tag created without parameters should point to HEAD' '\n \tgit tag non-annotated-tag &&\n-\ttest $(git cat-file -t non-annotated-tag) = commit &&\n-\ttest $(git rev-parse non-annotated-tag) = $(git rev-parse HEAD)\n+\techo commit >expect &&\n+\tgit cat-file -t non-annotated-tag >actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse non-annotated-tag >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'trying to verify an unknown tag should fail' '\n@@ -1517,11 +1527,11 @@ test_expect_success GPG 'verify signed tag fails when public key is not present'\n '\n \n test_expect_success 'git tag -a fails if tag annotation is empty' '\n-\t! (GIT_EDITOR=cat git tag -a initial-comment)\n+\ttest_must_fail env GIT_EDITOR=cat git tag -a initial-comment\n '\n \n test_expect_success 'message in editor has initial comment' '\n-\t! (GIT_EDITOR=cat git tag -a initial-comment >actual)\n+\ttest_must_fail env GIT_EDITOR=cat git tag -a initial-comment >actual\n '\n \n test_expect_success 'message in editor has initial comment: first line' '\n-- \n2.51.2\n\n"},{"id":"541569","messageId":"xmqqmrz5bhz4.fsf@gitster.g","threadId":"65481","inReplyTo":"20260414141828.27576-2-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 1/3] t7004: drop hardcoded tag count in invalid name test","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-14T16:54:23Z","receivedAt":"2026-04-14T16:54:26Z","isPatch":true,"body":"Siddharth Shrimali writes:\n\n> The test 'trying to create a tag with a non-valid name should fail',\n> checked that exactly one tag existed in the repository before and after\n> attempting to create invalid tags.\n>\n> As pointed out by Junio, this makes the test brittle by relying on a\n> specific global tag count. If future tests are added or removed before\n> this test, the expected state changes and this test would break for\n> completely unrelated reasons.\n>\n> Since we already use 'test_must_fail' to guarantee that the invalid\n> tags are rejected by Git, counting the tags before and after is redundant.\n>\n> Drop the 'test_line_count = 1' checks so the test doesn't rely on the\n> exact number of tags left behind by earlier tests.\n\nThe only thing I suggested was that relying on exact state before\nthis test makes this test brittle. I do not necessarily think\n\"redundant\" is bad. Having belt-and-suspenders sometimes help.\n\nAlternatively, if we wanted to catch a bug where \"git tag\" exits\nwith a non-zero status, satisfying test_must_fail, but still creates\nthe requested tag, then we could do\n\n\tgit tag -l >tags-before &&\n\ttest_must_fail git tag \"\" &&\n\t... random attempts to create with invalid names ...\n\ttest_must_fail git tag \"other~tag\" &&\n\tgit tag -l >tags-after &&\n\ttest_cmp tags-before tags-after\n\ninstead. And that is a belt-and-suspenders approach.\n\nHaving said that, the patch is already an improvement, so let's take\nit as is. Unless there are other things we may want to improve in\nthis or other patches in the series, that is.\n\nThanks.\n\n> Suggested-by: Junio C Hamano \n> Signed-off-by: Siddharth Shrimali \n> ---\n> t/t7004-tag.sh | 6 +-----\n> 1 file changed, 1 insertion(+), 5 deletions(-)\n>\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index faf7d97fc4..6ca5c75b57 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -191,15 +191,11 @@ 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> -\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> -\tgit tag -l >actual &&\n> -\ttest_line_count = 1 actual\n> +\ttest_must_fail git tag \"other~tag\"\n> '\n> \n> test_expect_success 'creating a tag using HEAD directly should succeed' '\n"},{"id":"541570","messageId":"xmqqik9tbhog.fsf@gitster.g","threadId":"65481","inReplyTo":"20260414141828.27576-3-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH 2/3] t7004: dynamically grab expected state in tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-04-14T17:00:47Z","receivedAt":"2026-04-14T17:00:50Z","isPatch":true,"body":"Siddharth Shrimali writes:\n\n> The tests for 'Multiple -l or --list options' and 'trying to delete\n> tags without params', hardcodes that exactly one or two specific tags\n> ('myhead', 'mytag') exist in the repository.\n>\n> If other tests are added, modified, or removed earlier in the script,\n> this expected global state will change, resulting in these tests to fail\n> for completely unrelated reasons.\n>\n> Instead of hardcoding the expected tags, dynamically grab the state\n> of the repository before running the commands under test ('git tag -l'\n> and 'git tag -d'), and verify that the output matches or remains\n> unchanged afterward. This keeps the tests independent from the script's\n> overall state.\n>\n> Signed-off-by: Siddharth Shrimali \n> ---\n> t/t7004-tag.sh | 11 ++---------\n> 1 file changed, 2 insertions(+), 9 deletions(-)\n\nExcellent. I agree with both reasoning above and execution below.\n\n>\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 6ca5c75b57..4fdd47cd21 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -145,9 +145,7 @@ test_expect_success 'listing all tags if one exists should succeed' '\n> '\n> \n> test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n> -\tcat >expect <<-\\EOF &&\n> -\tmytag\n> -\tEOF\n> +\tgit tag -l >expect &&\n> \tgit tag -l -l >actual &&\n> \ttest_cmp expect actual &&\n> \tgit tag --list --list >actual &&\n> @@ -223,12 +221,7 @@ test_expect_success 'trying to delete an unknown tag should fail' '\n> '\n> \n> test_expect_success 'trying to delete tags without params should succeed and do nothing' '\n> -\tcat >expect <<-\\EOF &&\n> -\tmyhead\n> -\tmytag\n> -\tEOF\n> -\tgit tag -l >actual &&\n> -\ttest_cmp expect actual &&\n> +\tgit tag -l >expect &&\n> \tgit tag -d &&\n> \tgit tag -l >actual &&\n> \ttest_cmp expect actual\n"},{"id":"541909","messageId":"aeXSLcVl_eGFkagr@pks.im","threadId":"65481","inReplyTo":"xmqqmrz5bhz4.fsf@gitster.g","subject":"Re: [PATCH 1/3] t7004: drop hardcoded tag count in invalid name test","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-20T07:13:49Z","receivedAt":"2026-04-20T07:13:56Z","isPatch":true,"body":"On Tue, Apr 14, 2026 at 09:54:23AM -0700, Junio C Hamano wrote:\n> Siddharth Shrimali writes:\n> \n> > The test 'trying to create a tag with a non-valid name should fail',\n> > checked that exactly one tag existed in the repository before and after\n> > attempting to create invalid tags.\n> >\n> > As pointed out by Junio, this makes the test brittle by relying on a\n> > specific global tag count. If future tests are added or removed before\n> > this test, the expected state changes and this test would break for\n> > completely unrelated reasons.\n> >\n> > Since we already use 'test_must_fail' to guarantee that the invalid\n> > tags are rejected by Git, counting the tags before and after is redundant.\n> >\n> > Drop the 'test_line_count = 1' checks so the test doesn't rely on the\n> > exact number of tags left behind by earlier tests.\n> \n> The only thing I suggested was that relying on exact state before\n> this test makes this test brittle. I do not necessarily think\n> \"redundant\" is bad. Having belt-and-suspenders sometimes help.\n> \n> Alternatively, if we wanted to catch a bug where \"git tag\" exits\n> with a non-zero status, satisfying test_must_fail, but still creates\n> the requested tag, then we could do\n> \n> \tgit tag -l >tags-before &&\n> \ttest_must_fail git tag \"\" &&\n> \t... random attempts to create with invalid names ...\n> \ttest_must_fail git tag \"other~tag\" &&\n> \tgit tag -l >tags-after &&\n> \ttest_cmp tags-before tags-after\n> \n> instead. And that is a belt-and-suspenders approach.\n> \n> Having said that, the patch is already an improvement, so let's take\n> it as is. Unless there are other things we may want to improve in\n> this or other patches in the series, that is.\n\nHm. We now rely on exit code alone, without verifying that the exit code\nactually results in the expected behaviour. I'm a bit torn myself\nwhether this is sensible and a step into the right direction, and I\nwould have preferred the `test_cmp` piece above that you propose.\n\nPatrick\n"},{"id":"542009","messageId":"20260421053334.5414-1-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260414141828.27576-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2 0/3] t7004: cleanup and modernize brittle tests","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-21T05:33:31Z","receivedAt":"2026-04-21T05:33:49Z","isPatch":true,"body":"This patch series addresses brittle testing patterns in t7004-tag.sh. \n\nIn this second version, the first patch has been updated to follow \nJunio's \"belt-and-suspenders\" suggestion. Instead of simply removing\nthe tag count check, it now uses 'test_cmp' to verify that the repository\nstate remains unchanged after failed tag creation attempts. This\nmaintains verification while removing the reliance on a hardcoded\nglobal tag count.\n\nSubsequent patches continue to modernize the script by removing \nhardcoded global state and replacing subshell patterns that could \notherwise suppress Git exit codes, ensuring that crashes (like \nsegmentation faults) are properly detected.\n\nThanks to Patrick and Junio for the feedback on v1 regarding\nstate verification.\n\n---\nChanges since v1:\n- Updated patch 1 to use 'test_cmp' for state verification \n instead of just dropping the count check.\n\nSiddharth Shrimali (3):\n t7004: drop hardcoded tag count for state verification\n t7004: dynamically grab expected state in tests\n t7004: avoid subshells to capture git exit codes\n\n t/t7004-tag.sh | 44 +++++++++++++++++++++++---------------------\n 1 file changed, 23 insertions(+), 21 deletions(-)\n\n-- \n2.51.2\n\n"},{"id":"542010","messageId":"20260421053334.5414-2-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260421053334.5414-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2 1/3] t7004: drop hardcoded tag count for state verification","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-21T05:33:32Z","receivedAt":"2026-04-21T05:33:54Z","isPatch":true,"body":"The test 'trying to create a tag with a non-valid name should fail',\nchecked that exactly one tag existed in the repository before and after\nattempting to create invalid tags.\n\nAs pointed out by Junio, this makes the test brittle by relying on a\nspecific global tag count. If future tests are added or removed before\nthis test, the expected state changes and this test would break for\ncompletely unrelated reasons.\n\nModernize the test by taking a snapshot of the existing tags before the\nfailure attempts and comparing it to a snapshot taken after.\nThis provides a \"belt-and-suspenders\" approach: we verify that\n'git tag' both exits with the expected error code and leaves the\nrepository state untouched, without being brittle to the specific\nnumber of tags present.\n\nThis replaces the hardcoded 'test_line_count = 1' checks with 'test_cmp'\nto ensure the tag list remains identical.\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex faf7d97fc4..77a7a9777d 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -191,15 +191,14 @@ 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-\tgit tag -l >actual &&\n-\ttest_line_count = 1 actual &&\n+\tgit tag -l >tags-before &&\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-\tgit tag -l >actual &&\n-\ttest_line_count = 1 actual\n+\tgit tag -l >tags-after &&\n+\ttest_cmp tags-before tags-after\n '\n \n test_expect_success 'creating a tag using HEAD directly should succeed' '\n-- \n2.51.2\n\n"},{"id":"542011","messageId":"20260421053334.5414-3-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260421053334.5414-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2 2/3] t7004: dynamically grab expected state in tests","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-21T05:33:33Z","receivedAt":"2026-04-21T05:34:01Z","isPatch":true,"body":"The tests for 'Multiple -l or --list options' and 'trying to delete\ntags without params', hardcodes that exactly one or two specific tags\n('myhead', 'mytag') exist in the repository.\n\nIf other tests are added, modified, or removed earlier in the script,\nthis expected global state will change, resulting in these tests to fail\nfor completely unrelated reasons.\n\nInstead of hardcoding the expected tags, dynamically grab the state\nof the repository before running the commands under test ('git tag -l'\nand 'git tag -d'), and verify that the output matches or remains\nunchanged afterward. This keeps the tests independent from the script's\noverall state.\n\nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 11 ++---------\n 1 file changed, 2 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 77a7a9777d..bef7618da2 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -145,9 +145,7 @@ test_expect_success 'listing all tags if one exists should succeed' '\n '\n \n test_expect_success 'Multiple -l or --list options are equivalent to one -l option' '\n-\tcat >expect <<-\\EOF &&\n-\tmytag\n-\tEOF\n+\tgit tag -l >expect &&\n \tgit tag -l -l >actual &&\n \ttest_cmp expect actual &&\n \tgit tag --list --list >actual &&\n@@ -226,12 +224,7 @@ test_expect_success 'trying to delete an unknown tag should fail' '\n '\n \n test_expect_success 'trying to delete tags without params should succeed and do nothing' '\n-\tcat >expect <<-\\EOF &&\n-\tmyhead\n-\tmytag\n-\tEOF\n-\tgit tag -l >actual &&\n-\ttest_cmp expect actual &&\n+\tgit tag -l >expect &&\n \tgit tag -d &&\n \tgit tag -l >actual &&\n \ttest_cmp expect actual\n-- \n2.51.2\n\n"},{"id":"542012","messageId":"20260421053334.5414-4-r.siddharth.shrimali@gmail.com","threadId":"65481","inReplyTo":"20260421053334.5414-1-r.siddharth.shrimali@gmail.com","subject":"[PATCH v2 3/3] t7004: avoid subshells to capture git exit codes","fromName":"Siddharth Shrimali","fromEmail":"r.siddharth.shrimali@gmail.com","sentAt":"2026-04-21T05:33:34Z","receivedAt":"2026-04-21T05:34:08Z","isPatch":true,"body":"Several tests in t7004 use the 'test$(git ...) = ...' or the '! (git ...)'\nsubshell pattern. This swallows git's exit code. If git crashes\n(e.g. segmentation fault) the crash would go undetected, and the test\nwould fail due to a mismatch or an inverted exit code.\n\nModernize these tests by directly writing output to files(actual) and\nverifying them with 'test_cmp' or 'test_grep'. Replace subshell\nnegations with 'test_must_fail'. This way, if git crashes, the test\nfails immediately and clearly instead of hiding the error behind a\nstring mismatch.\n\nSigned-off-by: Siddharth Shrimali \n---\n t/t7004-tag.sh | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex bef7618da2..d918005dd9 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -155,8 +155,10 @@ test_expect_success 'Multiple -l or --list options are equivalent to one -l opti\n '\n \n test_expect_success 'listing all tags if one exists should output that tag' '\n-\ttest $(git tag -l) = mytag &&\n-\ttest $(git tag) = mytag\n+\tgit tag -l >actual &&\n+\ttest_grep \"^mytag$\" actual &&\n+\tgit tag >actual &&\n+\ttest_grep \"^mytag$\" actual\n '\n \n # pattern matching:\n@@ -166,11 +168,15 @@ test_expect_success 'listing a tag using a matching pattern should succeed' '\n '\n \n test_expect_success 'listing a tag with --ignore-case' '\n-\ttest $(git tag -l --ignore-case MYTAG) = mytag\n+\techo mytag >expect &&\n+\tgit tag -l --ignore-case MYTAG >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'listing a tag using a matching pattern should output that tag' '\n-\ttest $(git tag -l mytag) = mytag\n+\techo mytag >expect &&\n+\tgit tag -l mytag >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'listing tags using a non-matching pattern should succeed' '\n@@ -430,8 +436,12 @@ test_expect_success 'listing tags -n in column with column.ui ignored' '\n \n test_expect_success 'a non-annotated tag created without parameters should point to HEAD' '\n \tgit tag non-annotated-tag &&\n-\ttest $(git cat-file -t non-annotated-tag) = commit &&\n-\ttest $(git rev-parse non-annotated-tag) = $(git rev-parse HEAD)\n+\techo commit >expect &&\n+\tgit cat-file -t non-annotated-tag >actual &&\n+\ttest_cmp expect actual &&\n+\tgit rev-parse HEAD >expect &&\n+\tgit rev-parse non-annotated-tag >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'trying to verify an unknown tag should fail' '\n@@ -1520,11 +1530,11 @@ test_expect_success GPG 'verify signed tag fails when public key is not present'\n '\n \n test_expect_success 'git tag -a fails if tag annotation is empty' '\n-\t! (GIT_EDITOR=cat git tag -a initial-comment)\n+\ttest_must_fail env GIT_EDITOR=cat git tag -a initial-comment\n '\n \n test_expect_success 'message in editor has initial comment' '\n-\t! (GIT_EDITOR=cat git tag -a initial-comment >actual)\n+\ttest_must_fail env GIT_EDITOR=cat git tag -a initial-comment >actual\n '\n \n test_expect_success 'message in editor has initial comment: first line' '\n-- \n2.51.2\n\n"},{"id":"542013","messageId":"aecNc-BNwaqFlg5c@pks.im","threadId":"65481","inReplyTo":"20260421053334.5414-1-r.siddharth.shrimali@gmail.com","subject":"Re: [PATCH v2 0/3] t7004: cleanup and modernize brittle tests","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-04-21T05:38:59Z","receivedAt":"2026-04-21T05:39:06Z","isPatch":true,"body":"On Tue, Apr 21, 2026 at 11:03:31AM +0530, Siddharth Shrimali wrote:\n> This patch series addresses brittle testing patterns in t7004-tag.sh. \n> \n> In this second version, the first patch has been updated to follow \n> Junio's \"belt-and-suspenders\" suggestion. Instead of simply removing\n> the tag count check, it now uses 'test_cmp' to verify that the repository\n> state remains unchanged after failed tag creation attempts. This\n> maintains verification while removing the reliance on a hardcoded\n> global tag count.\n> \n> Subsequent patches continue to modernize the script by removing \n> hardcoded global state and replacing subshell patterns that could \n> otherwise suppress Git exit codes, ensuring that crashes (like \n> segmentation faults) are properly detected.\n> \n> Thanks to Patrick and Junio for the feedback on v1 regarding\n> state verification.\n\nThanks, this version looks good to me!\n\nPatrick\n"}]}