From 3d0a4a34194cff4bd4d5845d63bbd28d1cab66dd Mon Sep 17 00:00:00 2001 From: Paulo Gomes Date: Fri, 16 Dec 2022 08:15:22 +0000 Subject: [PATCH] gogit: Add WithSingleBranch At present go-git does not support the MULTI_ACK capability, which means that follow-up fetches on a given remote will fail. To support Image Automation Controller use cases, the SwitchBranch was initially short-circuited to avoid additional fetches. However, this has the side effect of the controller pushing the same change to the target repository multiple times. (fluxcd/flux2#3384) In order to avoid this, a new WithSingleBranch option was created to enable the download of all references at the initial clone. From now on SwitchBranch has the single responsibility of switching branches, and no longer pulling references. The package git/gogit's primary goal is to support Flux use cases, currently there is no need to expand the current API to expose ways for users to refresh repository references outside the initial clone. Signed-off-by: Paulo Gomes --- git/gogit/client.go | 120 ++++++++++++++++++++++------------ git/gogit/client_test.go | 136 +++++++++++++++++++++++++++++++++++---- git/gogit/clone.go | 6 +- 3 files changed, 204 insertions(+), 58 deletions(-) diff --git a/git/gogit/client.go b/git/gogit/client.go index b14952d1..af08bd55 100644 --- a/git/gogit/client.go +++ b/git/gogit/client.go @@ -55,6 +55,7 @@ type Client struct { forcePush bool credentialsOverHTTP bool useDefaultKnownHosts bool + singleBranch bool } var _ repository.Client = &Client{} @@ -71,6 +72,8 @@ func NewClient(path string, authOpts *git.AuthOptions, clientOpts ...ClientOptio g := &Client{ path: securePath, authOpts: authOpts, + // Default to single branch as it is the most performant option. + singleBranch: true, } if len(clientOpts) == 0 { @@ -107,6 +110,25 @@ func WithWorkTreeFS(wt billy.Filesystem) ClientOption { } } +// WithSingleBranch indicates whether only the references of a single +// branch will be fetched during cloning operations. +// For read-only clones, and for single branch write operations, +// a single branch is advised for performance reasons. +// +// For write operations that require multiple branches, for example, +// cloning from main and pushing into a feature branch, this should be +// disabled. Otherwise a second fetch will be required to get the state +// of the target branch, which won't work against some Git servers due +// to MULTI_ACK not being implemented in go-git. +// +// By default this is enabled. +func WithSingleBranch(singleBranch bool) ClientOption { + return func(c *Client) error { + c.singleBranch = singleBranch + return nil + } +} + func WithDiskStorage() ClientOption { return func(c *Client) error { wt := fs.New(c.path) @@ -337,6 +359,32 @@ func (g *Client) Push(ctx context.Context) error { }) } +// SwitchBranch switches the current branch to the given branch name. +// +// No new references are fetched from the remote during the process, +// this is to ensure that the same flow can be used across all Git +// servers, regardless of them requiring MULTI_ACK or not. Once MULTI_ACK +// is implemented in go-git, this can be revisited. +// +// If more than one remote branch state is required, create the gogit +// client using WithSingleBranch(false). This will fetch all remote +// branches as part of the initial clone. Note that this is fully +// compatible with shallow clones. +// +// The following cases are handled: +// - Branch does not exist results in one being created using HEAD +// of the worktree. +// - Branch exists only remotely, results in a local branch being +// created tracking the remote HEAD. +// - Branch exists only locally, results in a checkout to the +// existing branch. +// - Branch exists locally and remotely, the local branch will take +// precendece. +// +// To override a remote branch with the state from the current branch, +// (i.e. image automation controller), use WithForcePush(true) in +// combination with WithSingleBranch(true). This will ignore the +// remote branch's existence. func (g *Client) SwitchBranch(ctx context.Context, branchName string) error { if g.repository == nil { return git.ErrNoGitRepository @@ -346,59 +394,49 @@ func (g *Client) SwitchBranch(ctx context.Context, branchName string) error { if err != nil { return fmt.Errorf("failed to load worktree: %w", err) } - authMethod, err := transportAuth(g.authOpts, g.useDefaultKnownHosts) - if err != nil { - return fmt.Errorf("failed to construct auth method with options: %w", err) - } - _, err = g.repository.Branch(branchName) - var create bool - if err == extgogit.ErrBranchNotFound { - create = true + // Assumes both local and remote branches exists until proven otherwise. + remote, local := true, true + remRefName := plumbing.NewRemoteReferenceName(extgogit.DefaultRemoteName, branchName) + remRef, err := g.repository.Reference(remRefName, true) + if errors.Is(err, plumbing.ErrReferenceNotFound) { + remote = false } else if err != nil { - return err - } - - err = wt.Checkout(&extgogit.CheckoutOptions{ - Branch: plumbing.NewBranchReferenceName(branchName), - Create: create, - }) - if err != nil { - return fmt.Errorf("could not checkout to branch '%s': %w", branchName, err) + return fmt.Errorf("could not fetch remote reference '%s': %w", branchName, err) } - // When force push is enabled, we always override the push branch. - // No need to fetch additional refs from that branch. - if g.forcePush { - return nil + refName := plumbing.NewBranchReferenceName(branchName) + _, err = g.repository.Reference(refName, true) + if errors.Is(err, plumbing.ErrReferenceNotFound) { + local = false + } else if err != nil { + return fmt.Errorf("could not fetch local reference '%s': %w", branchName, err) } - err = g.repository.FetchContext(ctx, &extgogit.FetchOptions{ - RemoteName: extgogit.DefaultRemoteName, - RefSpecs: []config.RefSpec{ - config.RefSpec(fmt.Sprintf("+refs/heads/%s:refs/remotes/%s/%[1]s", branchName, extgogit.DefaultRemoteName)), - }, - Auth: authMethod, - }) - if err != nil && !errors.Is(err, extgogit.NoErrAlreadyUpToDate) && !errors.Is(err, extgogit.NoMatchingRefSpecError{}) { - return fmt.Errorf("could not fetch context: %w", err) - } - ref, err := g.repository.Reference(plumbing.NewRemoteReferenceName(extgogit.DefaultRemoteName, branchName), true) + create := false + // If the remote branch exists, but not the local branch, create a local + // reference to the remote's HEAD. + if remote && !local { + branchRef := plumbing.NewHashReference(refName, remRef.Hash()) - // If remote ref doesn't exist, no need to reset to remote target commit, exit early. - if err == plumbing.ErrReferenceNotFound { - return nil - } else if err != nil { - return fmt.Errorf("could not fetch remote reference '%s': %w", branchName, err) + err = g.repository.Storer.SetReference(branchRef) + if err != nil { + return fmt.Errorf("could not create reference to remote HEAD '%s': %w", branchRef.Hash().String(), err) + } + } else if !remote && !local { + // If the target branch does not exist locally or remotely, create a new + // branch using the current worktree HEAD. + create = true } - err = wt.Reset(&extgogit.ResetOptions{ - Commit: ref.Hash(), - Mode: extgogit.HardReset, + err = wt.Checkout(&extgogit.CheckoutOptions{ + Branch: refName, + Create: create, }) if err != nil { - return fmt.Errorf("could not reset branch to be at commit '%s': %w", ref.Hash().String(), err) + return fmt.Errorf("could not checkout to branch '%s': %w", branchName, err) } + return nil } diff --git a/git/gogit/client_test.go b/git/gogit/client_test.go index 5d9b0c29..50b80433 100644 --- a/git/gogit/client_test.go +++ b/git/gogit/client_test.go @@ -307,10 +307,12 @@ func TestForcePush(t *testing.T) { func TestSwitchBranch(t *testing.T) { tests := []struct { - name string - setupFunc func(g *WithT, path string) string - branch string - forcePush bool + name string + setupFunc func(g *WithT, path string) string + changeRepo func(g *WithT, c *Client) string + branch string + forcePush bool + singleBranch bool }{ { name: "switch to a branch ahead of the current branch", @@ -336,6 +338,79 @@ func TestSwitchBranch(t *testing.T) { }, branch: "ahead", }, + { + name: "switch to a branch that exists locally and remotely", + setupFunc: func(g *WithT, repoURL string) string { + tmp := t.TempDir() + repo, err := extgogit.PlainClone(tmp, false, &extgogit.CloneOptions{ + URL: repoURL, + ReferenceName: plumbing.NewBranchReferenceName(git.DefaultBranch), + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + + err = createBranch(repo, "ahead") + g.Expect(err).ToNot(HaveOccurred()) + + cc, err := commitFile(repo, "test", "I live in the remote branch", time.Now()) + g.Expect(err).ToNot(HaveOccurred()) + err = repo.Push(&extgogit.PushOptions{ + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + return cc.String() + }, + changeRepo: func(g *WithT, c *Client) string { + wt, err := c.repository.Worktree() + g.Expect(err).ToNot(HaveOccurred()) + + err = wt.Checkout(&extgogit.CheckoutOptions{ + Branch: plumbing.NewBranchReferenceName("ahead"), + Create: true, + }) + g.Expect(err).ToNot(HaveOccurred()) + + cc, err := commitFile(c.repository, "new change", "local branch is warmer though", time.Now()) + g.Expect(err).ToNot(HaveOccurred()) + + err = wt.Checkout(&extgogit.CheckoutOptions{ + Branch: plumbing.Master, + }) + g.Expect(err).ToNot(HaveOccurred()) + + return cc.String() + }, + branch: "ahead", + }, + { + name: "singlebranch: ignore a branch that exists in the remote", + setupFunc: func(g *WithT, repoURL string) string { + tmp := t.TempDir() + repo, err := extgogit.PlainClone(tmp, false, &extgogit.CloneOptions{ + URL: repoURL, + ReferenceName: plumbing.NewBranchReferenceName(git.DefaultBranch), + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + + head, err := repo.Head() + g.Expect(err).ToNot(HaveOccurred()) + + err = createBranch(repo, "singlebranch-ahead") + g.Expect(err).ToNot(HaveOccurred()) + + _, err = commitFile(repo, "test", "I am going to be treated as stale", time.Now()) + g.Expect(err).ToNot(HaveOccurred()) + err = repo.Push(&extgogit.PushOptions{ + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + + return head.Hash().String() + }, + branch: "singlebranch-ahead", + singleBranch: true, + }, { name: "switch to a branch behind the current branch", setupFunc: func(g *WithT, repoURL string) string { @@ -387,20 +462,16 @@ func TestSwitchBranch(t *testing.T) { }) g.Expect(err).ToNot(HaveOccurred()) - ref, err := repo.Head() - g.Expect(err).ToNot(HaveOccurred()) - hash := ref.Hash().String() - err = createBranch(repo, "ahead") g.Expect(err).ToNot(HaveOccurred()) - _, err = commitFile(repo, "test", "testing gogit switch ahead branch", time.Now()) + cc, err := commitFile(repo, "test", "testing gogit switch ahead branch", time.Now()) g.Expect(err).ToNot(HaveOccurred()) err = repo.Push(&extgogit.PushOptions{ RemoteName: git.DefaultRemote, }) g.Expect(err).ToNot(HaveOccurred()) - return hash + return cc.String() }, branch: "ahead", forcePush: true, @@ -447,6 +518,36 @@ func TestSwitchBranch(t *testing.T) { branch: "new", forcePush: true, }, + { + name: "force: ignore a branch that exists in the remote", + setupFunc: func(g *WithT, repoURL string) string { + tmp := t.TempDir() + repo, err := extgogit.PlainClone(tmp, false, &extgogit.CloneOptions{ + URL: repoURL, + ReferenceName: plumbing.NewBranchReferenceName(git.DefaultBranch), + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + + head, err := repo.Head() + g.Expect(err).ToNot(HaveOccurred()) + + err = createBranch(repo, "singlebranch-ahead") + g.Expect(err).ToNot(HaveOccurred()) + + _, err = commitFile(repo, "test", "remote change that will be overwritten", time.Now()) + g.Expect(err).ToNot(HaveOccurred()) + err = repo.Push(&extgogit.PushOptions{ + RemoteName: git.DefaultRemote, + }) + g.Expect(err).ToNot(HaveOccurred()) + + return head.Hash().String() + }, + branch: "singlebranch-ahead", + singleBranch: true, + forcePush: true, + }, } for _, tt := range tests { @@ -464,19 +565,22 @@ func TestSwitchBranch(t *testing.T) { g.Expect(err).ToNot(HaveOccurred()) defer server.StopHTTP() + var expectedHash string + if tt.setupFunc != nil { + expectedHash = tt.setupFunc(g, filepath.Join(server.Root(), "test.git")) + } + repoURL := server.HTTPAddressWithCredentials() + "/" + "test.git" tmp := t.TempDir() repo, err := extgogit.PlainClone(tmp, false, &extgogit.CloneOptions{ URL: repoURL, ReferenceName: plumbing.NewBranchReferenceName(git.DefaultBranch), RemoteName: git.DefaultRemote, + SingleBranch: tt.singleBranch, }) g.Expect(err).ToNot(HaveOccurred()) - var expectedHash string - if tt.setupFunc != nil { - expectedHash = tt.setupFunc(g, filepath.Join(server.Root(), "test.git")) - } else { + if tt.setupFunc == nil { head, err := repo.Head() g.Expect(err).ToNot(HaveOccurred()) expectedHash = head.Hash().String() @@ -487,6 +591,10 @@ func TestSwitchBranch(t *testing.T) { ggc.repository = repo ggc.forcePush = tt.forcePush + if tt.changeRepo != nil { + expectedHash = tt.changeRepo(g, ggc) + } + err = ggc.SwitchBranch(context.TODO(), tt.branch) g.Expect(err).ToNot(HaveOccurred()) diff --git a/git/gogit/clone.go b/git/gogit/clone.go index f4aba8ab..9ed96c07 100644 --- a/git/gogit/clone.go +++ b/git/gogit/clone.go @@ -83,7 +83,7 @@ func (g *Client) cloneBranch(ctx context.Context, url, branch string, opts repos Auth: authMethod, RemoteName: git.DefaultRemote, ReferenceName: plumbing.NewBranchReferenceName(branch), - SingleBranch: true, + SingleBranch: g.singleBranch, NoCheckout: false, Depth: depth, RecurseSubmodules: recurseSubmodules(opts.RecurseSubmodules), @@ -173,7 +173,7 @@ func (g *Client) cloneTag(ctx context.Context, url, tag string, opts repository. Auth: authMethod, RemoteName: git.DefaultRemote, ReferenceName: plumbing.NewTagReferenceName(tag), - SingleBranch: true, + SingleBranch: g.singleBranch, NoCheckout: false, Depth: depth, RecurseSubmodules: recurseSubmodules(opts.RecurseSubmodules), @@ -222,7 +222,7 @@ func (g *Client) cloneCommit(ctx context.Context, url, commit string, opts repos CABundle: caBundle(g.authOpts), } if opts.Branch != "" { - cloneOpts.SingleBranch = true + cloneOpts.SingleBranch = g.singleBranch cloneOpts.ReferenceName = plumbing.NewBranchReferenceName(opts.Branch) }