From d9473d008c92f2c6bf8f00fd90a1bbcfe644dace Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Thu, 21 Oct 2021 19:25:55 +0200 Subject: [PATCH 1/2] libgit2: add remaining checkout strategy tests This commit is a follow up on 4dc3185c5fc94eb75048376edeb44571cece25f4 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 --- pkg/git/libgit2/checkout.go | 104 +++++------ pkg/git/libgit2/checkout_test.go | 287 +++++++++++++++++++++++++------ 2 files changed, 290 insertions(+), 101 deletions(-) diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index 1f6bb72d94..cff1059edd 100644 --- a/pkg/git/libgit2/checkout.go +++ b/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) - } - ref, err := repo.References.Dwim(c.tag) - if err != nil { - return nil, "", fmt.Errorf("unable to find tag '%s': %w", c.tag, 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) + return nil, "", fmt.Errorf("unable to clone '%s', error: %w", url, gitutil.LibGit2Error(err)) } - commit, err := repo.LookupCommit(head.Target()) + commit, err := checkoutDetachedDwim(repo, c.tag) 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 nil, "", 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) - } - commit, err := repo.LookupCommit(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) + return nil, "", fmt.Errorf("could not create oid for '%s': %w", c.commit, err) } - err = repo.CheckoutTree(tree, &git2go.CheckoutOptions{ - Strategy: git2go.CheckoutForce, - }) + commit, err := checkoutDetachedHEAD(repo, oid) 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) + 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 tag '%s': %w", t, err) + return nil, fmt.Errorf("unable to find '%s': %w", name, err) } - err = repo.SetHeadDetached(ref.Target()) + defer ref.Free() + c, err := ref.Peel(git2go.ObjectCommit) if err != nil { - return nil, "", fmt.Errorf("git checkout error: %w", err) + return nil, fmt.Errorf("could not get commit for ref '%s': %w", ref.Name(), err) } - head, err := repo.Head() + defer c.Free() + commit, err := c.AsCommit() if err != nil { - return nil, "", fmt.Errorf("git resolve HEAD error: %w", err) + return nil, fmt.Errorf("could not get commit object for ref '%s': %w", ref.Name(), err) } - commit, err := repo.LookupCommit(head.Target()) + 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", head.Target().String(), err) + return nil, fmt.Errorf("git commit '%s' not found: %w", oid.String(), err) } - err = repo.CheckoutHead(&git2go.CheckoutOptions{ + 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, "", fmt.Errorf("git checkout error: %w", err) + return nil, err } + defer head.Free() - return &Commit{commit}, fmt.Sprintf("%s/%s", t, commit.Id().String()), nil + commit, err := repo.LookupCommit(head.Target()) + if err != nil { + return nil, err + } + + return commit, nil } diff --git a/pkg/git/libgit2/checkout_test.go b/pkg/git/libgit2/checkout_test.go index 9772b2d048..5ad2b25234 100644 --- a/pkg/git/libgit2/checkout_test.go +++ b/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", - simple: true, + tag: "v0.0.1", + annotated: false, commitTime: now, }, { - tag: "v0.1.0+build-1", - simple: false, + tag: "v0.1.0+build-1", + 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", - simple: true, + tag: "v0.1.0+build-2", + annotated: false, commitTime: now.Add(2 * time.Minute), }, { - tag: "0.2.0", - simple: false, + tag: "0.2.0", + annotated: true, commitTime: now, - tagTime: now, + tagTime: now, }, } - tests := []struct{ - name string - constraint string - expectError error - expectTag string + tests := []struct { + name string + constraint string + expectErr error + 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) + } + commit, err := repo.LookupCommit(ref) if err != nil { t.Fatal(err) } - _, err = tag(repo, cId, tt.simple, tt.tag, tt.tagTime) + 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 { - g.Expect(err).To(Equal(tt.expectError)) + if tt.expectErr != nil { + 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 + "/")) - content, err := os.ReadFile(filepath.Join(tmpDir, "tag.txt")) - g.Expect(err).ToNot(HaveOccurred()) - g.Expect(content).To(BeEquivalentTo(tt.expectTag)) + g.Expect(ref).To(Equal(tt.expectTag + "/" + refs[tt.expectTag])) + g.Expect(filepath.Join(tmpDir, "tag")).To(BeARegularFile()) + g.Expect(os.ReadFile(filepath.Join(tmpDir, "tag"))).To(BeEquivalentTo(tt.expectTag)) }) } } @@ -145,22 +333,20 @@ func initBareRepo() (*git2go.Repository, error) { 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 +func createBranch(repo *git2go.Repository, branch string, commit *git2go.Commit) error { + if commit == nil { + var err error + commit, err = headCommit(repo) + if err != nil { + return err + } + defer commit.Free() } - - return commit, nil + _, err := repo.CreateBranch(branch, commit, false) + return err } -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 { - return repo.Tags.CreateLightweight(tag, commit, false) + if annotated { + 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 { From 56201f30fe78a062899b213c10a481f239b2adf8 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Fri, 22 Oct 2021 11:52:10 +0200 Subject: [PATCH 2/2] libgit2: Free most objects This commit ensures most of the `git2go` objects `Free` themselves from the underlying C object. Ensuring all objects are freed is not possible yet, due to the way commits are wired in to facilitate verification later on. In a later follow up, we should change this and e.g. validate as part of the checkout process, and move the implementation specific authentication configuration from `git` into `libgit2`. Signed-off-by: Hidde Beydals --- pkg/git/libgit2/checkout.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/pkg/git/libgit2/checkout.go b/pkg/git/libgit2/checkout.go index cff1059edd..7fcfe45128 100644 --- a/pkg/git/libgit2/checkout.go +++ b/pkg/git/libgit2/checkout.go @@ -76,6 +76,7 @@ func (c *CheckoutBranch) Checkout(ctx context.Context, path, url string, auth *g if err != nil { return nil, "", fmt.Errorf("git resolve HEAD error: %w", err) } + defer head.Free() commit, err := repo.LookupCommit(head.Target()) if err != nil { return nil, "", fmt.Errorf("git commit '%s' not found: %w", head.Target(), err) @@ -168,6 +169,7 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g // 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 { + defer c.Free() // Use the commit metadata as the decisive timestamp. tagTimestamps[cleanName] = c.Committer().When tags[cleanName] = name @@ -177,14 +179,17 @@ func (c *CheckoutSemVer) Checkout(ctx context.Context, path, url string, auth *g if err != nil { return fmt.Errorf("could not lookup '%s' as simple or annotated tag: %w", cleanName, err) } + defer t.Free() commit, err := t.Peel(git2go.ObjectCommit) if err != nil { return fmt.Errorf("could not get commit for tag '%s': %w", t.Name(), err) } + defer commit.Free() c, err := commit.AsCommit() if err != nil { return fmt.Errorf("could not get commit object for tag '%s': %w", t.Name(), err) } + defer c.Free() tagTimestamps[t.Name()] = c.Committer().When tags[t.Name()] = name return nil