Re: [PATCH 1/3] t7004: drop hardcoded tag count in invalid name test

[Patrick Steinhardt <ps@pks.im>]

On Tue, Apr 14, 2026 at 09:54:23AM -0700, Junio C Hamano wrote:
> Siddharth Shrimali <r.siddharth.shrimali@gmail.com> writes:
> 
> > 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.
> 
> 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.

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