Fix bug in FindAllTags when commit shas are used as tag names

When commit shas are used as tag names, the catfile call finds the
commit object instead of the tag object with the commit sha as the name.
This threw an error. The fix is to use the tag id explicitly instead of
using the tag name.

6 files changed, 26 insertions, 13 deletions

diff --git a/changelogs/unreleased/jc-fix-find-all-tags.yml b/changelogs/unreleased/jc-fix-find-all-tags.yml
new file mode 100644
index 000000000..077d44214
--- /dev/null
+++ b/changelogs/unreleased/jc-fix-find-all-tags.yml
@@ -0,0 +1,5 @@
+---
+title: Fix bug in FindAllTags when commit shas are used as tag names
+merge_request: 1119
+author:
+type: fixed
diff --git a/internal/git/log/tag.go b/internal/git/log/tag.go
index f1de94d7b..a117ddac5 100644
--- a/internal/git/log/tag.go
+++ b/internal/git/log/tag.go
@@ -14,14 +14,11 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
-// GetTagCatfile looks up a commit by revision using an existing *catfile.Batch instance.
-func GetTagCatfile(c *catfile.Batch, tagName string) (*gitalypb.Tag, error) {
-	info, err := c.Info(tagName)
-	if err != nil {
-		return nil, err
-	}
-
-	r, err := c.Tag(info.Oid)
+// GetTagCatfile looks up a commit by tagID using an existing *catfile.Batch instance.
+// note: we pass in the tagName because the tag name from refs/tags may be different
+// than the name found in the actual tag object. We want to use the tagName found in refs/tags
+func GetTagCatfile(c *catfile.Batch, tagID, tagName string) (*gitalypb.Tag, error) {
+	r, err := c.Tag(tagID)
 	if err != nil {
 		return nil, err
 	}
@@ -31,7 +28,8 @@ func GetTagCatfile(c *catfile.Batch, tagName string) (*gitalypb.Tag, error) {
 		return nil, err
 	}
 
-	tag, err := buildAnnotatedTag(c, info.Oid, tagName, header, body)
+	// the tagID is the oid of the tag object
+	tag, err := buildAnnotatedTag(c, tagID, tagName, header, body)
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/git/log/tag_test.go b/internal/git/log/tag_test.go
index bfce2a361..6c702910e 100644
--- a/internal/git/log/tag_test.go
+++ b/internal/git/log/tag_test.go
@@ -40,9 +40,9 @@ func TestGetTag(t *testing.T) {
 	require.NoError(t, err)
 	for _, testCase := range testCases {
 		t.Run(testCase.tagName, func(t *testing.T) {
-			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-m", testCase.message, testCase.tagName, testCase.rev)
+			tagID := testhelper.CreateTag(t, testRepoPath, testCase.tagName, testCase.rev, &testhelper.CreateTagOpts{Message: testCase.message})
 
-			tag, err := GetTagCatfile(c, testCase.tagName)
+			tag, err := GetTagCatfile(c, string(tagID), testCase.tagName)
 			require.NoError(t, err)
 			if len(testCase.message) >= helper.MaxCommitOrTagMessageSize {
 				testCase.message = testCase.message[:helper.MaxCommitOrTagMessageSize]
diff --git a/internal/service/ref/refs.go b/internal/service/ref/refs.go
index 940b69869..580c1670c 100644
--- a/internal/service/ref/refs.go
+++ b/internal/service/ref/refs.go
@@ -120,7 +120,7 @@ func parseAndReturnTags(ctx context.Context, repo *gitalypb.Repository, stream g
 		switch refType {
 		// annotated tag
 		case "tag":
-			tag, err = gitlog.GetTagCatfile(c, refName)
+			tag, err = gitlog.GetTagCatfile(c, tagID, refName)
 			if err != nil {
 				return fmt.Errorf("getting annotated tag: %v", err)
 			}
diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go
index 5db6a232e..8c4357c11 100644
--- a/internal/service/ref/refs_test.go
+++ b/internal/service/ref/refs_test.go
@@ -485,6 +485,9 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 	bigMessage := strings.Repeat("a", 11*1024)
 	bigMessageTag1ID := testhelper.CreateTag(t, testRepoCopyPath, "v1.7.0", commitID, &testhelper.CreateTagOpts{Message: bigMessage})
 
+	// A tag with a commit id as its name
+	commitTagID := testhelper.CreateTag(t, testRepoCopyPath, commitID, commitID, &testhelper.CreateTagOpts{Message: "commit tag with a commit sha as the name"})
+
 	client, conn := newRefServiceClient(t, serverSocketPath)
 	defer conn.Close()
 
@@ -505,6 +508,13 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 
 	expectedTags := []*gitalypb.Tag{
 		{
+			Name:         []byte(commitID),
+			Id:           string(commitTagID),
+			TargetCommit: gitCommit,
+			Message:      []byte("commit tag with a commit sha as the name"),
+			MessageSize:  40,
+		},
+		{
 			Name:         []byte("v1.0.0"),
 			Id:           "f4e6814c3e4e7a0de82a9e7cd20c626cc963a2f8",
 			TargetCommit: gitCommit,
diff --git a/internal/testhelper/tag.go b/internal/testhelper/tag.go
index 29e01e78c..21e2e9a21 100644
--- a/internal/testhelper/tag.go
+++ b/internal/testhelper/tag.go
@@ -40,6 +40,6 @@ func CreateTag(t *testing.T, repoPath, tagName, targetID string, opts *CreateTag
 
 	MustRunCommand(t, stdin, "git", args...)
 
-	tagID := MustRunCommand(t, nil, "git", "-C", repoPath, "rev-parse", tagName)
+	tagID := MustRunCommand(t, nil, "git", "-C", repoPath, "show-ref", "-s", tagName)
 	return strings.TrimSpace(string(tagID))
 }