Subject: Re: [PATCH v4 08/20] mktag tests: don't create "mytag" twice

From: Junio C Hamano <[email-masked]>
Date: Tue, 22 Dec 2020 18:18:12 -0800

Ævar Arnfjörð Bjarmason  <[email-masked]> writes:

> Change a test added in e0aaf781f6 (mktag.c: improve verification of
> tagger field and tests, 2008-03-27) to not create "mytag", which
> should only be created and verified at the end in an earlier test
> added in 446c6faec6 (New tests and en-passant modifications to mktag.,
> 2006-07-29).

If mktag fails to create a tag, presumably .git/refs/tags/mytag file
would be left empty.  Wouldn't "git tag -l" notice that it is not a
valid ref and omit it from its output?  I suspect if you corrupt the
contents of tag.sig temporarily while the first of the original
tests were running in such a way that mktag notices a bogosity, the
second half of the original would notice.

Having said that, I like the new way much better.

But do we even need to create the tag with update-ref in the first
place?  What does it check?  The fact that "mktag" only creates an
object and never creates a new ref to point at the newly created
object (hence we expect refs/tags/mytag does not yet exist)?

I am not complaining that it runs update-ref there or creates the
mytag tag.  I am just saying I do not understand what the test wants
to check by doing so.

> While we're at it let's prevent a similar logic error from creeping
> into the test by asserting that "mytag" doesn't exist before we create
> it. Let's do this by moving the test to use "update-ref", instead of
> our own homebrew ad-hoc refstore update.

Great.  I really like this part of the change that future-proofs us.

Thanks.

> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>  t/t3800-mktag.sh | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/t/t3800-mktag.sh b/t/t3800-mktag.sh
> index bbd148618e..b6dcdbebe6 100755
> --- a/t/t3800-mktag.sh
> +++ b/t/t3800-mktag.sh
> @@ -257,7 +257,7 @@ EOF
>  
>  test_expect_success \
>      'allow empty tag email' \
> -    'git mktag <tag.sig >.git/refs/tags/mytag'
> +    'git mktag <tag.sig'
>  
>  ############################################################
>  # 16. disallow spaces in tag email
> @@ -383,16 +383,9 @@ tagger T A Gger <[email-masked]> 1206478233 -0500
>  
>  EOF
>  
> -test_expect_success \
> -    'create valid tag' \
> -    'git mktag <tag.sig >.git/refs/tags/mytag'
> -
> -############################################################
> -# 25. check mytag
> -
> -test_expect_success \
> -    'check mytag' \
> -    'git tag -l | grep mytag'
> -
> +test_expect_success 'create valid tag' '
> +	git mktag <tag.sig >hash &&
> +	git update-ref refs/tags/mytag $(cat hash) $(test_oid zero)
> +'
>  
>  test_done