libgit2: correctly resolve (annotated) tags

In d0560e5 the SemVer implementations were aligned, and the logic was
simplified a bit (or so I thought). This did however result in the
introduction of a regression, as it failed to take "simple tags" into
account.

This commit ensures both are taken into account again, and ensures it
is now covered by a proper test.

Signed-off-by: Hidde Beydals <hello@hidde.co>

pkg/git/libgit2/checkout.go

@@ -20,13 +20,14 @@ import (
 	"context"
 	"fmt"
 	"sort"
+	"strings"
 	"time"
 
 	"github.com/Masterminds/semver/v3"
-	"github.com/fluxcd/pkg/version"
 	git2go "github.com/libgit2/git2go/v31"
 
 	"github.com/fluxcd/pkg/gitutil"
+	"github.com/fluxcd/pkg/version"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
 	"github.com/fluxcd/source-controller/pkg/git"
@@ -115,7 +116,7 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, auth *git.
 	if err != nil {
 		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target(), err)
 	}
-	err = repo.CheckoutHead(&git2go.CheckoutOpts{
+	err = repo.CheckoutHead(&git2go.CheckoutOptions{
 		Strategy: git2go.CheckoutForce,
 	})
 	if err != nil {
@@ -192,28 +193,37 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 	tags := make(map[string]string)
 	tagTimestamps := make(map[string]time.Time)
 	if err := repo.Tags.Foreach(func(name string, id *git2go.Oid) error {
-		tag, err := repo.LookupTag(id)
+		cleanName := strings.TrimPrefix(name, "refs/tags/")
-		if err != nil {
+		// The given ID can refer to both a commit and a tag, as annotated tags contain additional metadata.
+		// Due to this, first attempt to resolve it as a simple tag (commit), but fallback to attempting to
+		// resolve it as an annotated tag in case this results in an error.
+		if c, err := repo.LookupCommit(id); err == nil {
+			// Use the commit metadata as the decisive timestamp.
+			tagTimestamps[cleanName] = c.Committer().When
+			tags[cleanName] = name
 			return nil
 		}
-
+		t, err := repo.LookupTag(id)
-		commit, err := tag.Peel(git2go.ObjectCommit)
 		if err != nil {
-			return fmt.Errorf("can't get commit for tag %s: %w", name, err)
+			return fmt.Errorf("could not lookup '%s' as simple or annotated tag: %w", cleanName, err)
+		}
+		commit, err := t.Peel(git2go.ObjectCommit)
+		if err != nil {
+			return fmt.Errorf("could not get commit for tag '%s': %w", t.Name(), err)
 		}
 		c, err := commit.AsCommit()
 		if err != nil {
-			return err
+			return fmt.Errorf("could not get commit object for tag '%s': %w", t.Name(), err)
 		}
-		tagTimestamps[tag.Name()] = c.Committer().When
+		tagTimestamps[t.Name()] = c.Committer().When
-		tags[tag.Name()] = name
+		tags[t.Name()] = name
 		return nil
 	}); err != nil {
 		return nil, "", err
 	}
 
 	var matchedVersions semver.Collection
-	for tag, _ := range tags {
+	for tag := range tags {
 		v, err := version.ParseVersion(tag)
 		if err != nil {
 			continue
@@ -261,7 +271,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 	if err != nil {
 		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target().String(), err)
 	}
-	err = repo.CheckoutHead(&git2go.CheckoutOpts{
+	err = repo.CheckoutHead(&git2go.CheckoutOptions{
 		Strategy: git2go.CheckoutForce,
 	})
 	if err != nil {

pkg/git/libgit2/checkout_test.go

@@ -18,63 +18,214 @@ package libgit2
 
 import (
 	"context"
-	"crypto/sha256"
+	"errors"
-	"encoding/hex"
+	"fmt"
-	"io"
 	"os"
-	"path"
+	"path/filepath"
 	"testing"
+	"time"
 
 	git2go "github.com/libgit2/git2go/v31"
+	. "github.com/onsi/gomega"
 
 	"github.com/fluxcd/source-controller/pkg/git"
 )
 
 func TestCheckoutTagSemVer_Checkout(t *testing.T) {
-	certCallback := func(cert *git2go.Certificate, valid bool, hostname string) git2go.ErrorCode {
+	g := NewWithT(t)
-		return git2go.ErrorCodeOK
+	now := time.Now()
-	}
-	auth := &git.Auth{CertCallback: certCallback}
 
-	tag := CheckoutTag{
+	tags := []struct{
-		tag: "v1.7.0",
+		tag        string
+		simple     bool
+		commitTime time.Time
+		tagTime    time.Time
+	}{
+		{
+			tag: "v0.0.1",
+			simple: true,
+			commitTime: now,
+		},
+		{
+			tag: "v0.1.0+build-1",
+			simple: false,
+			commitTime: now.Add(1 * time.Minute),
+			tagTime: now.Add(1 * time.Hour), // This should be ignored during TS comparisons
+		},
+		{
+			tag: "v0.1.0+build-2",
+			simple: true,
+			commitTime: now.Add(2 * time.Minute),
+		},
+		{
+			tag: "0.2.0",
+			simple: false,
+			commitTime: now,
+			tagTime: now,
+		},
+	}
+	tests := []struct{
+		name        string
+		constraint  string
+		expectError error
+		expectTag   string
+	}{
+		{
+			name: "Orders by SemVer",
+			constraint: ">0.1.0",
+			expectTag: "0.2.0",
+		},
+		{
+			name: "Orders by SemVer and timestamp",
+			constraint: "<0.2.0",
+			expectTag: "v0.1.0+build-2",
+		},
+		{
+			name: "Errors without match",
+			constraint: ">=1.0.0",
+			expectError: errors.New("no match found for semver: >=1.0.0"),
+		},
 	}
-	tmpDir, _ := os.MkdirTemp("", "test")
-	defer os.RemoveAll(tmpDir)
 
-	cTag, _, err := tag.Checkout(context.TODO(), tmpDir, "https://github.com/projectcontour/contour", auth)
+	repo, err := initBareRepo()
 	if err != nil {
-		t.Error(err)
+		t.Fatal(err)
+	}
+	defer repo.Free()
+	defer os.RemoveAll(repo.Path())
+
+	for _, tt := range tags {
+		cId, err := commit(repo, "tag.txt", tt.tag, tt.commitTime)
+		if err != nil {
+			t.Fatal(err)
+		}
+		_, err = tag(repo, cId, tt.simple, tt.tag, tt.tagTime)
+		if err != nil {
+			t.Fatal(err)
+		}
 	}
 
-	// Ensure the correct files are checked out on disk
+	c, err := repo.Tags.List()
-	f, err := os.Open(path.Join(tmpDir, "README.md"))
+	g.Expect(err).ToNot(HaveOccurred())
-	if err != nil {
+	g.Expect(c).To(HaveLen(len(tags)))
-		t.Error(err)
-	}
-	defer f.Close()
-	h := sha256.New()
-	if _, err := io.Copy(h, f); err != nil {
-		t.Error(err)
-	}
-	const expectedHash = "2bd1707542a11f987ee24698dcc095a9f57639f401133ef6a29da97bf8f3f302"
-	fileHash := hex.EncodeToString(h.Sum(nil))
-	if fileHash != expectedHash {
-		t.Errorf("expected files not checked out. Expected hash %s, got %s", expectedHash, fileHash)
-	}
 
-	semVer := CheckoutSemVer{
+	for _, tt := range tests {
-		semVer: ">=1.0.0 <=1.7.0",
+		t.Run(tt.name, func(t *testing.T) {
-	}
+			semVer := CheckoutSemVer{
-	tmpDir2, _ := os.MkdirTemp("", "test")
+				semVer: tt.constraint,
-	defer os.RemoveAll(tmpDir2)
+			}
+			tmpDir, _ := os.MkdirTemp("", "test")
+			defer os.RemoveAll(tmpDir)
 
-	cSemVer, _, err := semVer.Checkout(context.TODO(), tmpDir2, "https://github.com/projectcontour/contour", auth)
+			_, ref, err := semVer.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
-	if err != nil {
+			if tt.expectError != nil {
-		t.Error(err)
+				g.Expect(err).To(Equal(tt.expectError))
-	}
+				g.Expect(ref).To(BeEmpty())
-
+				return
-	if cTag.Hash() != cSemVer.Hash() {
+			}
-		t.Errorf("expected semver hash %s, got %s", cTag.Hash(), cSemVer.Hash())
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(ref).To(HavePrefix(tt.expectTag + "/"))
+			content, err := os.ReadFile(filepath.Join(tmpDir, "tag.txt"))
+			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(content).To(BeEquivalentTo(tt.expectTag))
+		})
+	}
+}
+
+func initBareRepo() (*git2go.Repository, error) {
+	tmpDir, err := os.MkdirTemp("", "git2go-")
+	if err != nil {
+		return nil, err
+	}
+	repo, err := git2go.InitRepository(tmpDir, false)
+	if err != nil {
+		_ = os.RemoveAll(tmpDir)
+		return nil, err
+	}
+	return repo, nil
+}
+
+func headCommit(repo *git2go.Repository) (*git2go.Commit, error) {
+	head, err := repo.Head()
+	if err != nil {
+		return nil, err
+	}
+	defer head.Free()
+
+	commit, err := repo.LookupCommit(head.Target())
+	if err != nil {
+		return nil, err
+	}
+
+	return commit, nil
+}
+
+func commit(repo *git2go.Repository, path, content string, time time.Time) (*git2go.Oid, error) {
+	var parentC []*git2go.Commit
+	head, err := headCommit(repo)
+	if err == nil {
+		defer head.Free()
+		parentC = append(parentC, head)
+	}
+
+	index, err := repo.Index()
+	if err != nil {
+		return nil, err
+	}
+	defer index.Free()
+
+	blobOID, err := repo.CreateBlobFromBuffer([]byte(content))
+	if err != nil {
+		return nil, err
+	}
+
+	entry := &git2go.IndexEntry{
+		Mode: git2go.FilemodeBlob,
+		Id:   blobOID,
+		Path: path,
+	}
+
+	if err := index.Add(entry); err != nil {
+		return nil, err
+	}
+	if err := index.Write(); err != nil {
+		return nil, err
+	}
+
+	newTreeOID, err := index.WriteTree()
+	if err != nil {
+		return nil, err
+	}
+
+	tree, err := repo.LookupTree(newTreeOID)
+	if err != nil {
+		return nil, err
+	}
+	defer tree.Free()
+
+	commit, err := repo.CreateCommit("HEAD", signature(time), signature(time), "Committing "+path, tree, parentC...)
+	if err != nil {
+		return nil, err
+	}
+
+	return commit, nil
+}
+
+func tag(repo *git2go.Repository, cId *git2go.Oid, simple bool, tag string, time time.Time) (*git2go.Oid, error) {
+	commit, err := repo.LookupCommit(cId)
+	if err != nil {
+		return nil, err
+	}
+	if simple {
+		return repo.Tags.CreateLightweight(tag, commit, false)
+	}
+	return repo.Tags.Create(tag, commit, signature(time), fmt.Sprintf("Annotated tag for %s", tag))
+}
+
+func signature(time time.Time) *git2go.Signature {
+	return &git2go.Signature{
+		Name:  "Jane Doe",
+		Email: "author@example.com",
+		When:  time,
 	}
 }