Re: [PATCH] Add test-script for git-tag

Johannes Schindelin [off-list ref] forwarded:

+# creating and listing lightweight tags:
+
+tag_exists () {
+	git show-ref --quiet --verify refs/tags/$1
+}

It would be better if you _always_ quoted "$1" for my eyes, as
it would let my brain coast over it without thinking.  This does
not matter in practice as we do not allow metacharacters nor
whitespaces in tagnames, but that comes _after_ having to think
about it.

+test_expect_success 'listing all tags in an empty tree now should succeed' \
+	'git tag -l'

A recent patch made it not to exit with non-zero, but I think
that is probably a bug we might want to fix.

+test_expect_success \
+	'listing a tag using a matching pattern should output that tag' \
+	'test `git-tag -l mytag` = mytag'
+
+test_expect_success \
+	'listing tags using a non-matching pattern now should suceed' \
+	'git-tag -l xxx'
+
+test_expect_success \
+	'listing tags using a non-matching pattern should output nothing' \
+	'test `git-tag -l xxx | wc -l` -eq 0'
+

Additional test would be to see git-tag -l "my*" should succeed,
and git-tag -l "my.*" should fail, as the pattern should match
with fnmatch() not regexp().

+# special cases for creating tags:
+
+test_expect_failure \
+	'trying to create a tag with the name of one existing should fail' \
+	'git tag mytag'
+
+test_expect_failure 'trying to create a tag with a non-valid name should fail' '
+	test `git-tag -l | wc -l` -ne 1 ||
+	git tag .othertag ||
+	git tag "other tag" ||
+	git tag "othertag^" ||
+	git tag "other~tag" ||
+	test `git-tag -l | wc -l` -ne 1
+'

While it is logically correct to string error conditions with ||
and test inside expect-failure (you are saying "it is a failure
if any of these succeeds"), it needs a bit of brain-twisting to
get what is going on.  But I think this is a valid way to use
expect-failure.

+test_expect_success \
+	'listing tags with substring as pattern now must print those matching' '
...
+test_expect_success \
+	'listing tags with substring as pattern now must print those matching' '
...
+test_expect_success \
+	'listing tags with substring as pattern now must print those matching' '
...
+test_expect_success \
+	'listing tags using a name as pattern now must print those matching' '
...
+test_expect_success \
+	'listing tags using a name as pattern now must print those matching' '
...

Somehow, "now" gets quite irritating.

+
+cat >expect <<EOF
+v1.1.3
+EOF
+test_expect_success \
+	'listing tags with ? in the pattern should print those matching' '
+	git-tag -l 1.1? > actual &&
+	git-diff expect actual
+'

I know that there is no such file as 1.1X in the working tree,
but I would feel better if the test protected the ? from the
shell, as in "git tag -l 1.1\? >actual".

+# creating and verifying lightweight tags:
+
+test_expect_success \
+	'a non-annotated tag created without parameters should point to HEAD' '
+	git-tag non-annotated-tag &&
+	! git cat-file tag non-annotated-tag &&
+	test "$(git rev-parse non-annotated-tag^{commit})" = \
+		"$(git rev-parse HEAD)"
+'

I think testing the type directly is more to the point, as in

	test $(git-cat-file -t non-annotated-tag) = commit &&
        test $(git-rev-parse non-annotated-tag) = $(git-rev-parse HEAD)

+
+test_expect_failure 'trying to verify an unknown tag should fail' \
+	'git-tag -v unknown-tag'
+
+test_expect_failure \
+	'trying to verify a non-annotated and non-signed tag should fail' \
+	'git-tag -v non-annotated-tag'
+
+# creating annotated tags:
+
+get_tag_msg () {
+	git cat-file tag "$1" | sed -n -e "1,/BEGIN PGP/p"
+}
+

I would write that as "sed -e '/BEGIN PGP/q'", shorter and more
to the point.

+get_tag_header () {
+cat >expect <<EOF
+object $2
+type commit
+tag $1
+tagger C O Mitter [off-list ref] $3 -0700
+
+EOF
+}

Maybe -0700 deserves a comment --- test_tick always gives time
in that timezone.

You probably would want to also test annotated tags on non
commits (i.e. trees, blobs and tags).

+
+commit=$(git rev-parse HEAD)
+time=$test_tick
+
+get_tag_header annotated-tag $commit $time >expect

Huh? get_tag_header stores into "expect" file without saying
anything itself, and you redirect that nothingless to the same
file?

+touch emptyfile

Maybe it's just me being old-fashioned, but I prefer creation of
an empty file be done with:

	: >emptyfile
or just

	>emptyfile

and avoid using "touch".  Some broken systems did not create a
new file with "touch non-existing-file" (I think it was old AIX).

+# creating and verifying signed tags:
+
+gpg --version >/dev/null
+if [ $? -eq 127 ]; then
+	echo "Skipping signed tags tests, because gpg was not found"
+	test_done
+	exit
+fi
+
+# key generation info: gpg --homedir t/t7003 --gen-key
+# Type DSA and Elgamal, size 2048 bits, no expiration date.
+# Name and email: C O Mitter [off-list ref]
+# No password given, to enable non-interactive operation.
+
+cp -R ../t7003 ./gpghome
+chmod 0700 gpghome
+export GNUPGHOME="$(pwd)/gpghome"
+
+get_tag_header signed-tag $commit $time >expect
+echo 'A signed tag message' >>expect
+echo '-----BEGIN PGP SIGNATURE-----' >>expect
+test_expect_success 'creating a signed tag with -m message should succeed' '
+	git-tag -s -m "A signed tag message" signed-tag &&
+	get_tag_msg signed-tag >actual &&
+	git-diff expect actual
+'
+

This is just me being curious, but I wonder if this test is safe
to run on an otherwise idle server without moving parts and
keyboard (i.e. lack of sufficient entropy).  I think you made a
prudent choice of not generating a test key in the test script,
by the way.

+test_expect_success 'verifying a signed tag should succeed' \
+	'git-tag -v signed-tag'
+

Forging the tag at this point and make sure it does not verify
would also be interesting, as in:

	forged=$(git cat-file tag signed-tag |
		sed -e "s/signed-tag/forged-tag/" |
		git mktag) &&
	git tag forged-tag $forged &&
        if git-tag -v forged-tag
        then
        	echo Oops
		false
	else
        	: happy
	fi

Is it necessary to ship random_seed, or is it created as needed
during the test?