This patch series addresses some brittle testing patterns in t7004-tag.sh

The first patch follows a suggestion by Junio to remove redundant tag counting in the invalid name test. The second and third patches extend this cleanup to hardcoded global state and modernizing subshell patterns that could otherwise suppress git exit codes.

--- This series is a follow-up to my previous patch "t7004: replace wc -l with modern test helpers". Thanks to Junio for the feedback on the last patch. I have applied those changes here and have gone through the rest of the file and fixed similar issues.

Siddharth Shrimali (3):
  t7004: drop hardcoded tag count in invalid name test
  t7004: dynamically grab expected state in tests
  t7004: avoid subshells to capture git exit codes

 t/t7004-tag.sh | 43 +++++++++++++++++++++----------------------
 1 file changed, 21 insertions(+), 22 deletions(-)

The test 'trying to create a tag with a non-valid name should fail', checked that exactly one tag existed in the repository before and after attempting to create invalid tags.

As pointed out by Junio, this makes the test brittle by relying on a specific global tag count. If future tests are added or removed before this test, the expected state changes and this test would break for completely unrelated reasons.

Since we already use 'test_must_fail' to guarantee that the invalid tags are rejected by Git, counting the tags before and after is redundant.

Drop the 'test_line_count = 1' checks so the test doesn't rely on the exact number of tags left behind by earlier tests.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

The tests for 'Multiple -l or --list options' and 'trying to delete tags without params', hardcodes that exactly one or two specific tags ('myhead', 'mytag') exist in the repository.

If other tests are added, modified, or removed earlier in the script, this expected global state will change, resulting in these tests to fail for completely unrelated reasons.

Instead of hardcoding the expected tags, dynamically grab the state of the repository before running the commands under test ('git tag -l' and 'git tag -d'), and verify that the output matches or remains unchanged afterward. This keeps the tests independent from the script's overall state.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Several tests in t7004 use the 'test$(git ...) = ...' or the '! (git ...)' subshell pattern. This swallows git's exit code. If git crashes (e.g. segmentation fault) the crash would go undetected, and the test would fail due to a mismatch or an inverted exit code.

Modernize these tests by directly writing output to files(actual) and verifying them with 'test_cmp' or 'test_grep'. Replace subshell negations with 'test_must_fail'. This way, if git crashes, the test fails immediately and clearly instead of hiding the error behind a string mismatch.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

The only thing I suggested was that relying on exact state before this test makes this test brittle. I do not necessarily think "redundant" is bad. Having belt-and-suspenders sometimes help.

Alternatively, if we wanted to catch a bug where "git tag" exits with a non-zero status, satisfying test_must_fail, but still creates the requested tag, then we could do

	git tag -l >tags-before &&
	test_must_fail git tag "" &&
	... random attempts to create with invalid names ...
	test_must_fail git tag "other~tag" &&
	git tag -l >tags-after &&
	test_cmp tags-before tags-after

instead.   And that is a belt-and-suspenders approach.

Having said that, the patch is already an improvement, so let's take it as is. Unless there are other things we may want to improve in this or other patches in the series, that is.

Thanks.

Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:

Excellent.  I agree with both reasoning above and execution below.

On Tue, Apr 14, 2026 at 09:54:23AM -0700, Junio C Hamano wrote:

Hm. We now rely on exit code alone, without verifying that the exit code actually results in the expected behaviour. I'm a bit torn myself whether this is sensible and a step into the right direction, and I would have preferred the `test_cmp` piece above that you propose.

Patrick

This patch series addresses brittle testing patterns in t7004-tag.sh. 

In this second version, the first patch has been updated to follow Junio's "belt-and-suspenders" suggestion. Instead of simply removing the tag count check, it now uses 'test_cmp' to verify that the repository state remains unchanged after failed tag creation attempts. This maintains verification while removing the reliance on a hardcoded global tag count.

Subsequent patches continue to modernize the script by removing hardcoded global state and replacing subshell patterns that could otherwise suppress Git exit codes, ensuring that crashes (like segmentation faults) are properly detected.

Thanks to Patrick and Junio for the feedback on v1 regarding state verification.

---
Changes since v1:
- Updated patch 1 to use 'test_cmp' for state verification 
  instead of just dropping the count check.

Siddharth Shrimali (3):
  t7004: drop hardcoded tag count for state verification
  t7004: dynamically grab expected state in tests
  t7004: avoid subshells to capture git exit codes

 t/t7004-tag.sh | 44 +++++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 21 deletions(-)

The test 'trying to create a tag with a non-valid name should fail', checked that exactly one tag existed in the repository before and after attempting to create invalid tags.

As pointed out by Junio, this makes the test brittle by relying on a specific global tag count. If future tests are added or removed before this test, the expected state changes and this test would break for completely unrelated reasons.

Modernize the test by taking a snapshot of the existing tags before the failure attempts and comparing it to a snapshot taken after. This provides a "belt-and-suspenders" approach: we verify that 'git tag' both exits with the expected error code and leaves the repository state untouched, without being brittle to the specific number of tags present.

This replaces the hardcoded 'test_line_count = 1' checks with 'test_cmp' to ensure the tag list remains identical.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

The tests for 'Multiple -l or --list options' and 'trying to delete tags without params', hardcodes that exactly one or two specific tags ('myhead', 'mytag') exist in the repository.

If other tests are added, modified, or removed earlier in the script, this expected global state will change, resulting in these tests to fail for completely unrelated reasons.

Instead of hardcoding the expected tags, dynamically grab the state of the repository before running the commands under test ('git tag -l' and 'git tag -d'), and verify that the output matches or remains unchanged afterward. This keeps the tests independent from the script's overall state.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Several tests in t7004 use the 'test$(git ...) = ...' or the '! (git ...)' subshell pattern. This swallows git's exit code. If git crashes (e.g. segmentation fault) the crash would go undetected, and the test would fail due to a mismatch or an inverted exit code.

Modernize these tests by directly writing output to files(actual) and verifying them with 'test_cmp' or 'test_grep'. Replace subshell negations with 'test_must_fail'. This way, if git crashes, the test fails immediately and clearly instead of hiding the error behind a string mismatch.

Signed-off-by: Siddharth Shrimali <r.siddharth.shrimali@gmail.com>
---
 t/t7004-tag.sh | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

On Tue, Apr 21, 2026 at 11:03:31AM +0530, Siddharth Shrimali wrote:

Thanks, this version looks good to me!

Patrick