fix regression in switchToBranch

Fixes regression in which we fail to push to a branch after switching to
a branch, if origin is ahead of local. Fixed by setting the upstream
commit as the local branch target.

Regression introduced in fluxcd#330, and partially addressed in fluxcd#369.

Signed-off-by: Sanskar Jaiswal <[email protected]>

controllers/git_test.go

@@ -10,6 +10,9 @@ import (
 
 	"github.com/go-logr/logr"
 	libgit2 "github.com/libgit2/git2go/v33"
+	"k8s.io/apimachinery/pkg/types"
+
+	. "github.com/onsi/gomega"
 
 	"github.com/fluxcd/pkg/gittestserver"
 	"github.com/fluxcd/source-controller/pkg/git/libgit2/managed"
@@ -165,3 +168,88 @@ func TestPushRejected(t *testing.T) {
 		t.Error("push to a forbidden branch is expected to fail, but succeeded")
 	}
 }
+
+func Test_switchToBranch(t *testing.T) {
+	g := NewWithT(t)
+	gitServer, err := gittestserver.NewTempGitServer()
+	g.Expect(err).ToNot(HaveOccurred())
+	gitServer.AutoCreate()
+
+	g.Expect(gitServer.StartHTTP()).To(Succeed())
+	g.Expect(initGitRepo(gitServer, "testdata/appconfig", "master", "/appconfig.git")).To(Succeed())
+
+	repoURL := gitServer.HTTPAddressWithCredentials() + "/appconfig.git"
+	repo, err := clone(repoURL, "master")
+	g.Expect(err).ToNot(HaveOccurred())
+	defer repo.Free()
+
+	head, err := repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	defer head.Free()
+	target := head.Target()
+
+	// register transport options and update remote to transport url
+	transportOptsURL := "http://" + randStringRunes(5)
+	managed.AddTransportOptions(transportOptsURL, managed.TransportOptions{
+		TargetURL: repoURL,
+	})
+	defer managed.RemoveTransportOptions(transportOptsURL)
+	repo.Remotes.SetUrl("origin", transportOptsURL)
+
+	// calling switchToBranch with a branch that doesn't exist on origin
+	// should result in the branch being created and switched to.
+	switchToBranch(repo, context.TODO(), "not-on-origin", repoAccess{})
+
+	head, err = repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	name, err := head.Branch().Name()
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(name).To(Equal("not-on-origin"))
+
+	cc, err := repo.LookupCommit(head.Target())
+	g.Expect(err).ToNot(HaveOccurred())
+	defer cc.Free()
+	g.Expect(cc.Id().String()).To(Equal(target.String()))
+
+	// create a branch with the HEAD commit and push it to origin
+	_, err = repo.CreateBranch("test", cc, false)
+	g.Expect(err).ToNot(HaveOccurred())
+	origin, err := repo.Remotes.Lookup("origin")
+	g.Expect(err).ToNot(HaveOccurred())
+	defer origin.Free()
+
+	g.Expect(origin.Push([]string{"refs/heads/test:refs/heads/test"}, &libgit2.PushOptions{})).To(Succeed())
+
+	// push a new commit to the test branch. this is done to test whether we properly
+	// sync our local branch with the remote branch, before switching.
+	policyKey := types.NamespacedName{
+		Name:      "policy",
+		Namespace: "ns",
+	}
+	commitID := commitInRepo(g, repoURL, "test", "Install setter marker", func(tmp string) {
+		g.Expect(replaceMarker(tmp, policyKey)).To(Succeed())
+	})
+
+	// calling switchToBranch with a branch that exists should make sure to fetch latest
+	// for that branch from origin, and then switch to it.
+	switchToBranch(repo, context.TODO(), "test", repoAccess{})
+	head, err = repo.Head()
+	g.Expect(err).ToNot(HaveOccurred())
+	name, err = head.Branch().Name()
+
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(name).To(Equal("test"))
+	g.Expect(head.Target().String()).To(Equal(commitID.String()))
+
+	// push a commit after switching to the test branch, to check if the local
+	// branch is synced with origin.
+	replaceMarker(repo.Workdir(), policyKey)
+	sig := &libgit2.Signature{
+		Name:  "Testbot",
+		Email: "[email protected]",
+		When:  time.Now(),
+	}
+	_, err = commitWorkDir(repo, "test", "update policy", sig)
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(push(context.TODO(), repo.Workdir(), "test", repoAccess{})).To(Succeed())
+}

controllers/imageupdateautomation_controller.go

@@ -749,7 +749,6 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		callbacks = managed.RemoteCallbacks()
 	}
 
-	branchRef := fmt.Sprintf("origin/%s", branch)
 	// Force the fetching of the remote branch.
 	err = origin.Fetch([]string{branch}, &libgit2.FetchOptions{
 		RemoteCallbacks: callbacks,
@@ -758,7 +757,7 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		return fmt.Errorf("cannot fetch remote branch: %w", err)
 	}
 
-	remoteBranch, err := repo.LookupBranch(branchRef, libgit2.BranchRemote)
+	remoteBranch, err := repo.References.Lookup(fmt.Sprintf("refs/remotes/origin/%s", branch))
 	if err != nil && !libgit2.IsErrorCode(err, libgit2.ErrorCodeNotFound) {
 		return err
 	}
@@ -785,15 +784,28 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 	}
 	defer commit.Free()
 
-	localBranch, err := repo.LookupBranch(branch, libgit2.BranchLocal)
+	localBranch, err := repo.References.Lookup(fmt.Sprintf("refs/heads/%s", branch))
 	if err != nil && !libgit2.IsErrorCode(err, libgit2.ErrorCodeNotFound) {
 		return fmt.Errorf("cannot lookup branch '%s': %w", branch, err)
 	}
 	if localBranch == nil {
-		localBranch, err = repo.CreateBranch(branch, commit, false)
-	}
-	if localBranch == nil {
-		return fmt.Errorf("cannot create local branch '%s': %w", branch, err)
+		// We could've done something like:
+		// lb, err := repo.CreateBranch()
+		// if lb != nil {
+		//	 defer lb.Free()
+		//	 localBranch = lb.Reference
+		// }
+		// But for some reasone, calling `lb.Free()` under any circumstances, causes a really
+		// nasty crash. Since not calling `lb.Free()` could result in memory leaks, we avoid
+		// assigning a variable like `lb` and get the branch reference manually.
+		_, err := repo.CreateBranch(branch, commit, false)
+		if err != nil {
+			return fmt.Errorf("cannot create local branch '%s': %w", branch, err)
+		}
+		localBranch, err = repo.References.Lookup(fmt.Sprintf("refs/heads/%s", branch))
+		if err != nil {
+			return fmt.Errorf("cannot lookup branch '%s': %w", branch, err)
+		}
 	}
 	defer localBranch.Free()
 
@@ -811,6 +823,11 @@ func switchToBranch(repo *libgit2.Repository, ctx context.Context, branch string
 		return fmt.Errorf("cannot checkout tree for branch '%s': %w", branch, err)
 	}
 
+	_, err = localBranch.SetTarget(commit.Id(), "")
+	if err != nil {
+		return fmt.Errorf("cannot update branch '%s' to be at target commit: %w", branch, err)
+	}
+
 	return repo.SetHead("refs/heads/" + branch)
 }
 

controllers/update_test.go

@@ -904,7 +904,7 @@ func configureManagedTransportOptions(repo *libgit2.Repository, repoURL string)
 	}, nil
 }
 
-func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) {
+func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path string)) *libgit2.Oid {
 	repo, err := clone(repoURL, branch)
 	g.Expect(err).ToNot(HaveOccurred())
 	defer repo.Free()
@@ -916,21 +916,18 @@ func commitInRepo(g *WithT, repoURL, branch, msg string, changeFiles func(path s
 		Email: "[email protected]",
 		When:  time.Now(),
 	}
-	_, err = commitWorkDir(repo, branch, msg, sig)
+	id, err := commitWorkDir(repo, branch, msg, sig)
 	g.Expect(err).ToNot(HaveOccurred())
 
 	cleanup, err := configureManagedTransportOptions(repo, repoURL)
-	if err != nil {
-		panic(err)
-	}
+	g.Expect(err).ToNot(HaveOccurred())
 	defer cleanup()
 	origin, err := repo.Remotes.Lookup(originRemote)
-	if err != nil {
-		panic(fmt.Errorf("cannot find origin: %v", err))
-	}
+	g.Expect(err).ToNot(HaveOccurred())
 	defer origin.Free()
 
 	g.Expect(origin.Push([]string{branchRefName(branch)}, &libgit2.PushOptions{})).To(Succeed())
+	return id
 }
 
 // Initialise a git server with a repo including the files in dir.