libgit2: add remaining checkout strategy tests

This commit is a follow up on 4dc3185c5f
and adds tests for the remaining checkout strategies, while
consolidating some of the logic.

The consolidated logic ensures that (SemVer) tag and commit checkouts
happen using the same "checkout detached HEAD" logic.
The branch checkout is left unmodified, and simply checks out at the
current HEAD of the given branch.

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

pkg/git/libgit2/checkout.go

@@ -98,31 +98,12 @@ func (c *CheckoutTag) Checkout(ctx context.Context, path, url string, auth *git.
 		},
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
-	ref, err := repo.References.Dwim(c.tag)
+	commit, err := checkoutDetachedDwim(repo, c.tag)
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to find tag '%s': %w", c.tag, err)
+		return nil, "", err
 	}
-	err = repo.SetHeadDetached(ref.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-	head, err := repo.Head()
-	if err != nil {
-		return nil, "", fmt.Errorf("git resolve HEAD error: %w", err)
-	}
-	commit, err := repo.LookupCommit(head.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target(), err)
-	}
-	err = repo.CheckoutHead(&git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", c.tag, commit.Id().String()), nil
 }
 
@@ -140,30 +121,19 @@ func (c *CheckoutCommit) Checkout(ctx context.Context, path, url string, auth *g
 				CertificateCheckCallback: auth.CertCallback,
 			},
 		},
-		CheckoutBranch: c.branch,
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
+
 	oid, err := git2go.NewOid(c.commit)
 	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' could not be parsed", c.commit)
+		return nil, "", fmt.Errorf("could not create oid for '%s': %w", c.commit, err)
 	}
-	commit, err := repo.LookupCommit(oid)
+	commit, err := checkoutDetachedHEAD(repo, oid)
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", c.commit, err)
-	}
-	tree, err := repo.LookupTree(commit.TreeId())
-	if err != nil {
-		return nil, "", fmt.Errorf("git worktree error: %w", err)
-	}
-	err = repo.CheckoutTree(tree, &git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
 	if err != nil {
 		return nil, "", fmt.Errorf("git checkout error: %w", err)
 	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", c.branch, commit.Id().String()), nil
 }
 
@@ -187,7 +157,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 		},
 	})
 	if err != nil {
-		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, err)
+		return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err))
 	}
 
 	tags := make(map[string]string)
@@ -255,28 +225,62 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g
 	v := matchedVersions[len(matchedVersions)-1]
 	t := v.Original()
 
-	ref, err := repo.References.Dwim(t)
+	commit, err := checkoutDetachedDwim(repo, t)
-	if err != nil {
-		return nil, "", fmt.Errorf("unable to find tag '%s': %w", t, err)
-	}
-	err = repo.SetHeadDetached(ref.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-	head, err := repo.Head()
-	if err != nil {
-		return nil, "", fmt.Errorf("git resolve HEAD error: %w", err)
-	}
-	commit, err := repo.LookupCommit(head.Target())
-	if err != nil {
-		return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target().String(), err)
-	}
-	err = repo.CheckoutHead(&git2go.CheckoutOptions{
-		Strategy: git2go.CheckoutForce,
-	})
-	if err != nil {
-		return nil, "", fmt.Errorf("git checkout error: %w", err)
-	}
-
 	return &Commit{commit}, fmt.Sprintf("%s/%s", t, commit.Id().String()), nil
 }
+
+// checkoutDetachedDwim attempts to perform a detached HEAD checkout by first DWIMing the short name
+// to get a concrete reference, and then calling checkoutDetachedHEAD.
+func checkoutDetachedDwim(repo *git2go.Repository, name string) (*git2go.Commit, error) {
+	ref, err := repo.References.Dwim(name)
+	if err != nil {
+		return nil, fmt.Errorf("unable to find '%s': %w", name, err)
+	}
+	defer ref.Free()
+	c, err := ref.Peel(git2go.ObjectCommit)
+	if err != nil {
+		return nil, fmt.Errorf("could not get commit for ref '%s': %w", ref.Name(), err)
+	}
+	defer c.Free()
+	commit, err := c.AsCommit()
+	if err != nil {
+		return nil, fmt.Errorf("could not get commit object for ref '%s': %w", ref.Name(), err)
+	}
+	defer commit.Free()
+	return checkoutDetachedHEAD(repo, commit.Id())
+}
+
+// checkoutDetachedHEAD attempts to perform a detached HEAD checkout for the given commit.
+func checkoutDetachedHEAD(repo *git2go.Repository, oid *git2go.Oid) (*git2go.Commit, error) {
+	commit, err := repo.LookupCommit(oid)
+	if err != nil {
+		return nil, fmt.Errorf("git commit '%s' not found: %w", oid.String(), err)
+	}
+	if err = repo.SetHeadDetached(commit.Id()); err != nil {
+		commit.Free()
+		return nil, fmt.Errorf("could not detach HEAD at '%s': %w", oid.String(), err)
+	}
+	if err = repo.CheckoutHead(&git2go.CheckoutOptions{
+		Strategy: git2go.CheckoutForce,
+	}); err != nil {
+		commit.Free()
+		return nil, fmt.Errorf("git checkout error: %w", err)
+	}
+	return commit, nil
+}
+
+// headCommit returns the current HEAD of the repository, or an error.
+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
+}

pkg/git/libgit2/checkout_test.go

@@ -31,59 +31,239 @@ import (
 	"github.com/fluxcd/source-controller/pkg/git"
 )
 
+func TestCheckoutBranch_Checkout(t *testing.T) {
+	repo, err := initBareRepo()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	firstCommit, err := commitFile(repo, "branch", "init", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = createBranch(repo, "test", nil); err != nil {
+		t.Fatal(err)
+	}
+
+	secondCommit, err := commitFile(repo, "branch", "second", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	tests := []struct {
+		name           string
+		branch         string
+		expectedCommit string
+		expectedErr    string
+	}{
+		{
+			name:           "Default branch",
+			branch:         "master",
+			expectedCommit: secondCommit.String(),
+		},
+		{
+			name:           "Other branch",
+			branch:         "test",
+			expectedCommit: firstCommit.String(),
+		},
+		{
+			name:        "Non existing branch",
+			branch:      "invalid",
+			expectedErr: "reference 'refs/remotes/origin/invalid' not found",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			branch := CheckoutBranch{
+				branch: tt.branch,
+			}
+			tmpDir, _ := os.MkdirTemp("", "test")
+			defer os.RemoveAll(tmpDir)
+
+			_, ref, err := branch.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+			if tt.expectedErr != "" {
+				g.Expect(err.Error()).To(ContainSubstring(tt.expectedErr))
+				g.Expect(ref).To(BeEmpty())
+				return
+			}
+			g.Expect(ref).To(Equal(tt.branch + "/" + tt.expectedCommit))
+			g.Expect(err).To(BeNil())
+		})
+	}
+}
+
+func TestCheckoutTag_Checkout(t *testing.T) {
+	tests := []struct {
+		name        string
+		tag         string
+		annotated   bool
+		checkoutTag string
+		expectTag   string
+		expectErr   string
+	}{
+		{
+			name:        "Tag",
+			tag:         "tag-1",
+			checkoutTag: "tag-1",
+			expectTag:   "tag-1",
+		},
+		{
+			name:        "Annotated",
+			tag:         "annotated",
+			annotated:   true,
+			checkoutTag: "annotated",
+			expectTag:   "annotated",
+		},
+		{
+			name:        "Non existing tag",
+			checkoutTag: "invalid",
+			expectErr:   "unable to find 'invalid': no reference found for shorthand 'invalid'",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			repo, err := initBareRepo()
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			var commit *git2go.Commit
+			if tt.tag != "" {
+				c, err := commitFile(repo, "tag", tt.tag, time.Now())
+				if err != nil {
+					t.Fatal(err)
+				}
+				if commit, err = repo.LookupCommit(c); err != nil {
+					t.Fatal(err)
+				}
+				_, err = tag(repo, c, !tt.annotated, tt.tag, time.Now())
+				if err != nil {
+					t.Fatal(err)
+				}
+			}
+
+			tag := CheckoutTag{
+				tag: tt.checkoutTag,
+			}
+			tmpDir, _ := os.MkdirTemp("", "test")
+			defer os.RemoveAll(tmpDir)
+
+			_, ref, err := tag.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+			if tt.expectErr != "" {
+				g.Expect(err.Error()).To(Equal(tt.expectErr))
+				g.Expect(ref).To(BeEmpty())
+				return
+			}
+			if tt.expectTag != "" {
+				g.Expect(ref).To(Equal(tt.expectTag + "/" + commit.Id().String()))
+				g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile())
+				g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.tag))
+			}
+		})
+	}
+}
+
+func TestCheckoutCommit_Checkout(t *testing.T) {
+	g := NewWithT(t)
+
+	repo, err := initBareRepo()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer repo.Free()
+	defer os.RemoveAll(repo.Path())
+
+	c, err := commitFile(repo, "commit", "init", time.Now())
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err = commitFile(repo, "commit", "second", time.Now()); err != nil {
+		t.Fatal(err)
+	}
+
+	commit := CheckoutCommit{
+		commit: c.String(),
+		branch: "main",
+	}
+	tmpDir, _ := os.MkdirTemp("", "git2go")
+	defer os.RemoveAll(tmpDir)
+
+	_, ref, err := commit.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
+	g.Expect(err).To(BeNil())
+	g.Expect(ref).To(Equal("main/" + c.String()))
+	g.Expect(filepath.Join(tmpDir, "commit")).To(BeARegularFile())
+	g.Expect(os.ReadFile(filepath.Join(tmpDir, "commit"))).To(BeEquivalentTo("init"))
+
+	commit = CheckoutCommit{
+		commit: "4dc3185c5fc94eb75048376edeb44571cece25f4",
+	}
+	tmpDir2, _ := os.MkdirTemp("", "git2go")
+	defer os.RemoveAll(tmpDir)
+
+	_, ref, err = commit.Checkout(context.TODO(), tmpDir2, repo.Path(), &git.Auth{})
+	g.Expect(err.Error()).To(HavePrefix("git checkout error: git commit '4dc3185c5fc94eb75048376edeb44571cece25f4' not found:"))
+	g.Expect(ref).To(BeEmpty())
+}
+
 func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 	g := NewWithT(t)
 	now := time.Now()
 
-	tags := []struct{
+	tags := []struct {
 		tag        string
-		simple     bool
+		annotated  bool
 		commitTime time.Time
 		tagTime    time.Time
 	}{
 		{
-			tag: "v0.0.1",
+			tag:        "v0.0.1",
-			simple: true,
+			annotated:  false,
 			commitTime: now,
 		},
 		{
-			tag: "v0.1.0+build-1",
+			tag:        "v0.1.0+build-1",
-			simple: false,
+			annotated:  true,
 			commitTime: now.Add(1 * time.Minute),
-			tagTime: now.Add(1 * time.Hour), // This should be ignored during TS comparisons
+			tagTime:    now.Add(1 * time.Hour), // This should be ignored during TS comparisons
 		},
 		{
-			tag: "v0.1.0+build-2",
+			tag:        "v0.1.0+build-2",
-			simple: true,
+			annotated:  false,
 			commitTime: now.Add(2 * time.Minute),
 		},
 		{
-			tag: "0.2.0",
+			tag:        "0.2.0",
-			simple: false,
+			annotated:  true,
 			commitTime: now,
-			tagTime: now,
+			tagTime:    now,
 		},
 	}
-	tests := []struct{
+	tests := []struct {
-		name        string
+		name       string
-		constraint  string
+		constraint string
-		expectError error
+		expectErr  error
-		expectTag   string
+		expectTag  string
 	}{
 		{
-			name: "Orders by SemVer",
+			name:       "Orders by SemVer",
 			constraint: ">0.1.0",
-			expectTag: "0.2.0",
+			expectTag:  "0.2.0",
 		},
 		{
-			name: "Orders by SemVer and timestamp",
+			name:       "Orders by SemVer and timestamp",
 			constraint: "<0.2.0",
-			expectTag: "v0.1.0+build-2",
+			expectTag:  "v0.1.0+build-2",
 		},
 		{
-			name: "Errors without match",
+			name:       "Errors without match",
 			constraint: ">=1.0.0",
-			expectError: errors.New("no match found for semver: >=1.0.0"),
+			expectErr:  errors.New("no match found for semver: >=1.0.0"),
 		},
 	}
 
@@ -94,12 +274,19 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 	defer repo.Free()
 	defer os.RemoveAll(repo.Path())
 
+	refs := make(map[string]string, len(tags))
 	for _, tt := range tags {
-		cId, err := commit(repo, "tag.txt", tt.tag, tt.commitTime)
+		ref, err := commitFile(repo, "tag", tt.tag, tt.commitTime)
 		if err != nil {
 			t.Fatal(err)
 		}
-		_, err = tag(repo, cId, tt.simple, tt.tag, tt.tagTime)
+		commit, err := repo.LookupCommit(ref)
+		if err != nil {
+			t.Fatal(err)
+		}
+		defer commit.Free()
+		refs[tt.tag] = commit.Id().String()
+		_, err = tag(repo, ref, tt.annotated, tt.tag, tt.tagTime)
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -111,6 +298,8 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
 			semVer := CheckoutSemVer{
 				semVer: tt.constraint,
 			}
@@ -118,16 +307,15 @@ func TestCheckoutTagSemVer_Checkout(t *testing.T) {
 			defer os.RemoveAll(tmpDir)
 
 			_, ref, err := semVer.Checkout(context.TODO(), tmpDir, repo.Path(), &git.Auth{})
-			if tt.expectError != nil {
+			if tt.expectErr != nil {
-				g.Expect(err).To(Equal(tt.expectError))
+				g.Expect(err).To(Equal(tt.expectErr))
 				g.Expect(ref).To(BeEmpty())
 				return
 			}
 			g.Expect(err).ToNot(HaveOccurred())
-			g.Expect(ref).To(HavePrefix(tt.expectTag + "/"))
+			g.Expect(ref).To(Equal(tt.expectTag + "/" + refs[tt.expectTag]))
-			content, err := os.ReadFile(filepath.Join(tmpDir, "tag.txt"))
+			g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile())
-			g.Expect(err).ToNot(HaveOccurred())
+			g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.expectTag))
-			g.Expect(content).To(BeEquivalentTo(tt.expectTag))
 		})
 	}
 }
@@ -145,22 +333,20 @@ func initBareRepo() (*git2go.Repository, error) {
 	return repo, nil
 }
 
-func headCommit(repo *git2go.Repository) (*git2go.Commit, error) {
+func createBranch(repo *git2go.Repository, branch string, commit *git2go.Commit) error {
-	head, err := repo.Head()
+	if commit == nil {
-	if err != nil {
+		var err error
-		return nil, err
+		commit, err = headCommit(repo)
+		if err != nil {
+			return err
+		}
+		defer commit.Free()
 	}
-	defer head.Free()
+	_, err := repo.CreateBranch(branch, commit, false)
-
+	return err
-	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) {
+func commitFile(repo *git2go.Repository, path, content string, time time.Time) (*git2go.Oid, error) {
 	var parentC []*git2go.Commit
 	head, err := headCommit(repo)
 	if err == nil {
@@ -192,12 +378,12 @@ func commit(repo *git2go.Repository, path, content string, time time.Time) (*git
 		return nil, err
 	}
 
-	newTreeOID, err := index.WriteTree()
+	treeID, err := index.WriteTree()
 	if err != nil {
 		return nil, err
 	}
 
-	tree, err := repo.LookupTree(newTreeOID)
+	tree, err := repo.LookupTree(treeID)
 	if err != nil {
 		return nil, err
 	}
@@ -207,19 +393,18 @@ func commit(repo *git2go.Repository, path, content string, time time.Time) (*git
 	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) {
+func tag(repo *git2go.Repository, cId *git2go.Oid, annotated bool, tag string, time time.Time) (*git2go.Oid, error) {
 	commit, err := repo.LookupCommit(cId)
 	if err != nil {
 		return nil, err
 	}
-	if simple {
+	if annotated {
-		return repo.Tags.CreateLightweight(tag, commit, false)
+		return repo.Tags.Create(tag, commit, signature(time), fmt.Sprintf("Annotated tag for %s", tag))
 	}
-	return repo.Tags.Create(tag, commit, signature(time), fmt.Sprintf("Annotated tag for %s", tag))
+	return repo.Tags.CreateLightweight(tag, commit, false)
 }
 
 func signature(time time.Time) *git2go.Signature {