Merge branch 'jc-fix-find-all-tags-for-tags' into 'master'

Fix FindAllTags to dereference tags that point to other tags

Closes #1605

See merge request gitlab-org/gitaly!1193

3 files changed, 169 insertions, 3 deletions

diff --git a/changelogs/unreleased/jc-fix-find-all-tags-for-tags.yml b/changelogs/unreleased/jc-fix-find-all-tags-for-tags.yml
new file mode 100644
index 000000000..d11109ebe
--- /dev/null
+++ b/changelogs/unreleased/jc-fix-find-all-tags-for-tags.yml
@@ -0,0 +1,5 @@
+---
+title: Fix FindAllTags to dereference tags that point to other tags
+merge_request: 1193
+author:
+type: fixed
diff --git a/internal/git/log/tag.go b/internal/git/log/tag.go
index 5465d8a09..a9b9cbbcb 100644
--- a/internal/git/log/tag.go
+++ b/internal/git/log/tag.go
@@ -13,6 +13,11 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
+const (
+	// MaxTagReferenceDepth is the maximum depth of tag references we will dereference
+	MaxTagReferenceDepth = 10
+)
+
 // 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
@@ -88,14 +93,58 @@ func buildAnnotatedTag(b *catfile.Batch, tagID, name string, header *tagHeader,
 		tag.Message = tag.Message[:max]
 	}
 
-	if header.tagType == "commit" {
-		commit, err := GetCommitCatfile(b, header.oid)
+	var err error
+	switch header.tagType {
+	case "commit":
+		tag.TargetCommit, err = GetCommitCatfile(b, header.oid)
 		if err != nil {
 			return nil, fmt.Errorf("buildAnnotatedTag error when getting target commit: %v", err)
 		}
 
-		tag.TargetCommit = commit
+	case "tag":
+		tag.TargetCommit, err = dereferenceTag(b, header.oid)
+		if err != nil {
+			return nil, fmt.Errorf("buildAnnotatedTag error when dereferencing tag: %v", err)
+		}
 	}
 
 	return tag, nil
 }
+
+// dereferenceTag recursively dereferences annotated tags until it finds a commit.
+// This matches the original behavior in the ruby implementation.
+// we also protect against circular tag references. Even though this is not possible in git,
+// we still want to protect against an infinite looop
+
+func dereferenceTag(b *catfile.Batch, Oid string) (*gitalypb.GitCommit, error) {
+	for depth := 0; depth < MaxTagReferenceDepth; depth++ {
+		i, err := b.Info(Oid)
+		if err != nil {
+			return nil, err
+		}
+
+		switch i.Type {
+		case "tag":
+			r, err := b.Tag(Oid)
+			if err != nil {
+				return nil, err
+			}
+
+			header, _, err := splitRawTag(r)
+			if err != nil {
+				return nil, err
+			}
+
+			Oid = header.oid
+			continue
+		case "commit":
+			return GetCommitCatfile(b, Oid)
+		default: // This current tag points to a tree or a blob
+			return nil, nil
+		}
+	}
+
+	// at this point the tag nesting has gone too deep. We want to return silently here however, as we don't
+	// want to fail the entire request if one tag is nested too deeply.
+	return nil, nil
+}
diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go
index 2fae85498..3aafe4149 100644
--- a/internal/service/ref/refs_test.go
+++ b/internal/service/ref/refs_test.go
@@ -11,8 +11,10 @@ import (
 	"testing"
 
 	"github.com/golang/protobuf/ptypes/timestamp"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -486,6 +488,9 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 	// 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"})
 
+	// a tag of a tag
+	tagOfTagID := testhelper.CreateTag(t, testRepoCopyPath, "tag-of-tag", commitTagID, &testhelper.CreateTagOpts{Message: "tag of a tag"})
+
 	client, conn := newRefServiceClient(t, serverSocketPath)
 	defer conn.Close()
 
@@ -513,6 +518,13 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 			MessageSize:  40,
 		},
 		{
+			Name:         []byte("tag-of-tag"),
+			Id:           string(tagOfTagID),
+			TargetCommit: gitCommit,
+			Message:      []byte("tag of a tag"),
+			MessageSize:  12,
+		},
+		{
 			Name:         []byte("v1.0.0"),
 			Id:           "f4e6814c3e4e7a0de82a9e7cd20c626cc963a2f8",
 			TargetCommit: gitCommit,
@@ -582,6 +594,105 @@ func TestSuccessfulFindAllTagsRequest(t *testing.T) {
 		require.Equal(t, expectedTag, receivedTags[i])
 	}
 }
+func TestFindAllTagNestedTags(t *testing.T) {
+	server, serverSocketPath := runRefServiceServer(t)
+	defer server.Stop()
+
+	testRepoCopy, testRepoCopyPath, cleanupFn := testhelper.NewTestRepoWithWorktree(t)
+	defer cleanupFn()
+
+	blobID := "faaf198af3a36dbf41961466703cc1d47c61d051"
+	commitID := "6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9"
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testCases := []struct {
+		description string
+		depth       int
+		originalOid string
+	}{
+		{
+			description: "nested 1 deep, points to a commit",
+			depth:       1,
+			originalOid: commitID,
+		},
+		{
+			description: "nested 4 deep, points to a commit",
+			depth:       4,
+			originalOid: commitID,
+		},
+		{
+			description: "nested 3 deep, points to a blob",
+			depth:       3,
+			originalOid: blobID,
+		},
+		{
+			description: "nested 20 deep, points to a commit",
+			depth:       20,
+			originalOid: commitID,
+		},
+	}
+
+	for _, tc := range testCases {
+		tags := bytes.NewReader(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoCopyPath, "tag"))
+		testhelper.MustRunCommand(t, tags, "xargs", "git", "-C", testRepoCopyPath, "tag", "-d")
+
+		batch, err := catfile.New(ctx, testRepoCopy)
+		require.NoError(t, err)
+
+		info, err := batch.Info(tc.originalOid)
+		require.NoError(t, err)
+
+		expectedTags := make(map[string]*gitalypb.Tag)
+		tagID := tc.originalOid
+
+		for depth := 0; depth < tc.depth; depth++ {
+			tagName := fmt.Sprintf("tag-depth-%d", depth)
+			tagMessage := fmt.Sprintf("a commit %d deep", depth)
+			tagID = string(testhelper.CreateTag(t, testRepoCopyPath, tagName, tagID, &testhelper.CreateTagOpts{Message: tagMessage}))
+
+			expectedTag := &gitalypb.Tag{
+				Name:        []byte(tagName),
+				Id:          string(tagID),
+				Message:     []byte(tagMessage),
+				MessageSize: int64(len([]byte(tagMessage))),
+			}
+
+			// only expect the TargetCommit to be populated if it is a commit and if its less than 10 tags deep
+			if info.Type == "commit" && depth < log.MaxTagReferenceDepth {
+				commit, err := log.GetCommitCatfile(batch, tc.originalOid)
+				require.NoError(t, err)
+				expectedTag.TargetCommit = commit
+			}
+
+			expectedTags[string(expectedTag.Name)] = expectedTag
+		}
+
+		client, conn := newRefServiceClient(t, serverSocketPath)
+		defer conn.Close()
+
+		rpcRequest := &gitalypb.FindAllTagsRequest{Repository: testRepoCopy}
+
+		c, err := client.FindAllTags(ctx, rpcRequest)
+		require.NoError(t, err)
+
+		var receivedTags []*gitalypb.Tag
+		for {
+			r, err := c.Recv()
+			if err == io.EOF {
+				break
+			}
+			require.NoError(t, err)
+			receivedTags = append(receivedTags, r.GetTags()...)
+		}
+
+		require.Len(t, receivedTags, len(expectedTags))
+		for _, receivedTag := range receivedTags {
+			assert.Equal(t, expectedTags[string(receivedTag.Name)], receivedTag)
+		}
+	}
+}
 
 func TestInvalidFindAllTagsRequest(t *testing.T) {
 	server, serverSocketPath := runRefServiceServer(t)
@@ -652,6 +763,7 @@ func TestSuccessfulFindLocalBranches(t *testing.T) {
 		if err == io.EOF {
 			break
 		}
+		require.NoError(t, err)
 		if err != nil {
 			t.Fatal(err)
 		}