fix(checkout-strategy): On merge strategy, reclone when necessary on base update

server/events/working_dir.go

@@ -79,8 +79,7 @@ type FileWorkspace struct {
 
 // Clone git clones headRepo, checks out the branch and then returns the absolute
 // path to the root of the cloned repo. It also returns
-// a boolean indicating if we should warn users that the branch we're
-// merging into has been updated since we cloned it.
+// a boolean indicating whether we had to merge with upstream again.
 // If the repo already exists and is at
 // the right commit it does nothing. This is to support running commands in
 // multiple dirs of the same repo without deleting existing plans.
@@ -90,6 +89,7 @@ func (w *FileWorkspace) Clone(
 	p models.PullRequest,
 	workspace string) (string, bool, error) {
 	cloneDir := w.cloneDir(p.BaseRepo, p, workspace)
+	hasDiverged := false
 
 	// If the directory already exists, check if it's at the right commit.
 	// If so, then we do nothing.
@@ -117,26 +117,31 @@ func (w *FileWorkspace) Clone(
 		// We're prefix matching here because BitBucket doesn't give us the full
 		// commit, only a 12 character prefix.
 		if strings.HasPrefix(currCommit, p.HeadCommit) {
-			log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
-			return cloneDir, w.warnDiverged(log, p, headRepo, cloneDir), nil
+			if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
+				log.Info("base branch has been updated, using merge strategy and will clone again")
+				hasDiverged = true
+			} else {
+				log.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
+				return cloneDir, false, nil
+			}
+		} else {
+			log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit)
 		}
-
-		log.Debug("repo was already cloned but is not at correct commit, wanted %q got %q", p.HeadCommit, currCommit)
 		// We'll fall through to re-clone.
 	}
 
 	// Otherwise we clone the repo.
-	return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p)
+	return cloneDir, hasDiverged, w.forceClone(log, cloneDir, headRepo, p)
 }
 
-// warnDiverged returns true if we should warn the user that the branch we're
-// merging into has diverged from what we currently have checked out.
+// recheckDiverged returns true if the branch we're merging into has diverged
+// from what we currently have checked out.
 // This matters in the case of the merge checkout strategy because after
-// cloning the repo and doing the merge, it's possible main was updated.
-// Then users won't be getting the merge functionality they expected.
+// cloning the repo and doing the merge, it's possible main was updated
+// and we have to perform a new merge.
 // If there are any errors we return false since we prefer things to succeed
 // vs. stopping the plan/apply.
-func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool {
+func (w *FileWorkspace) recheckDiverged(log logging.SimpleLogging, p models.PullRequest, headRepo models.Repo, cloneDir string) bool {
 	if !w.CheckoutMerge {
 		// It only makes sense to warn that main has diverged if we're using
 		// the checkout merge strategy. If we're just checking out the branch,
@@ -174,13 +179,7 @@ func (w *FileWorkspace) warnDiverged(log logging.SimpleLogging, p models.PullReq
 		}
 	}
 
-	hasDiverged := w.HasDiverged(log, cloneDir)
-	if hasDiverged {
-		log.Info("remote main branch is ahead and thereby has new commits, it is recommended to pull new commits")
-	} else {
-		log.Debug("remote main branch has no new commits")
-	}
-	return hasDiverged
+	return w.HasDiverged(log, cloneDir)
 }
 
 func (w *FileWorkspace) HasDiverged(log logging.SimpleLogging, cloneDir string) bool {

server/events/working_dir_test.go

@@ -412,7 +412,8 @@ func TestClone_RecloneWrongCommit(t *testing.T) {
 }
 
 // Test that if the branch we're merging into has diverged and we're using
-// checkout-strategy=merge, we warn the user (see #804).
+// checkout-strategy=merge, we actually merge the branch.
+// Also check that we do not merge if we are not using the merge strategy.
 func TestClone_MasterHasDiverged(t *testing.T) {
 	// Initialize the git repo.
 	repoDir := initRepo(t)
@@ -463,28 +464,40 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	// Run the clone.
 	wd := &events.FileWorkspace{
 		DataDir:             repoDir,
-		CheckoutMerge:       true,
+		CheckoutMerge:       false,
 		CheckoutDepth:       50,
 		GpgNoSigningEnabled: true,
 	}
-	_, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
+
+	// Run the clone without the checkout merge strategy. It should return
+	// false for hasDiverged
+	_, hasDiverged, err := wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{
+		BaseRepo:   models.Repo{},
+		HeadBranch: "second-pr",
+		BaseBranch: "main",
+	}, "default")
+	Ok(t, err)
+	Assert(t, hasDiverged == false, "Clone with CheckoutMerge=false should not merge")
+
+	wd.CheckoutMerge = true
+	// Run the clone twice with the merge strategy, the first run should
+	// return true for hasDiverged, subsequent runs should
+	// return false since the first call is supposed to merge.
+	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
 		BaseRepo:   models.Repo{CloneURL: repoDir},
 		HeadBranch: "second-pr",
 		BaseBranch: "main",
 	}, "default")
 	Ok(t, err)
-	Equals(t, hasDiverged, true)
+	Assert(t, hasDiverged == true, "First clone with CheckoutMerge=true with diverged base should have merged")
 
-	// Run it again but without the checkout merge strategy. It should return
-	// false.
-	wd.CheckoutMerge = false
-	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{}, models.PullRequest{
-		BaseRepo:   models.Repo{},
+	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
+		BaseRepo:   models.Repo{CloneURL: repoDir},
 		HeadBranch: "second-pr",
 		BaseBranch: "main",
 	}, "default")
 	Ok(t, err)
-	Equals(t, hasDiverged, false)
+	Assert(t, hasDiverged == false, "Second clone with CheckoutMerge=true and initially diverged base should not merge again")
 }
 
 func TestHasDiverged_MasterHasDiverged(t *testing.T) {