fix: Check for upstream changes to merge only when planning (runatlan…

…tis#3493)

* whitespace fixes

* Only re-clone on upstream updates just before planning

When using the merge checkout strategy, we would peviously
re-clone on upstream updates on any call to Clone(), for example
when running workflow hooks or runing import commands.

If we had already ran a plan, and master was updated, this
would delete the plan and apply would not have anything to
apply.

Modified to only re-clone on upstream updates in doPlan()

* Apply suggestions from code review

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
Co-authored-by: nitrocode <7775707+nitrocode@users.noreply.github.com>

server/events/project_command_runner.go

@@ -528,6 +528,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 	}
 	defer unlockFn()
 
+	p.WorkingDir.SetSafeToReClone()
 	// Clone is idempotent so okay to run even if the repo was already cloned.
 	repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.Log, ctx.HeadRepo, ctx.Pull, ctx.Workspace)
 	if cloneErr != nil {

server/events/working_dir.go

@@ -49,6 +49,10 @@ type WorkingDir interface {
 	// Delete deletes the workspace for this repo and pull.
 	Delete(r models.Repo, p models.PullRequest) error
 	DeleteForWorkspace(r models.Repo, p models.PullRequest, workspace string) error
+	// Set a flag in the workingdir so Clone() can know that it is safe to re-clone the workingdir if
+	// the upstream branch has been modified. This is only safe after grabbing the project lock
+	// and before running any plans
+	SetSafeToReClone()
 }
 
 // FileWorkspace implements WorkingDir with the file system.
@@ -75,6 +79,8 @@ type FileWorkspace struct {
 	GithubAppEnabled bool
 	// use the global setting without overriding
 	GpgNoSigningEnabled bool
+	// flag indicating if a re-clone will be safe (project lock held, about to run plan)
+	SafeToReClone bool
 }
 
 // Clone git clones headRepo, checks out the branch and then returns the absolute
@@ -90,6 +96,7 @@ func (w *FileWorkspace) Clone(
 	workspace string) (string, bool, error) {
 	cloneDir := w.cloneDir(p.BaseRepo, p, workspace)
 	hasDiverged := false
+	defer func() { w.SafeToReClone = false }()
 
 	// If the directory already exists, check if it's at the right commit.
 	// If so, then we do nothing.
@@ -117,7 +124,7 @@ 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) {
-			if w.CheckoutMerge && w.recheckDiverged(log, p, headRepo, cloneDir) {
+			if w.SafeToReClone && 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 {
@@ -260,17 +267,17 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 	if !w.CheckoutMerge {
 		return runGit("clone", "--depth=1", "--branch", p.HeadBranch, "--single-branch", headCloneURL, cloneDir)
 	}
-	
+
 	// if merge strategy...
 
 	// if no checkout depth, omit depth arg
 	if w.CheckoutDepth == 0 {
 		if err := runGit("clone", "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
-			 return err
+			return err
 		}
 	} else {
-	 	if err := runGit("clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
-			 return err
+		if err := runGit("clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
+			return err
 		}
 	}
 
@@ -285,16 +292,16 @@ func (w *FileWorkspace) forceClone(log logging.SimpleLogging,
 		fetchRemote = "origin"
 	}
 
-        // if no checkout depth, omit depth arg
-        if w.CheckoutDepth == 0 {
-                if err := runGit("fetch", fetchRemote, fetchRef); err != nil {
-                         return err
-                }
-        } else {
-                if err := runGit("fetch", "--depth", fmt.Sprint(w.CheckoutDepth), fetchRemote, fetchRef); err != nil {
-                         return err
-                }
-        }
+	// if no checkout depth, omit depth arg
+	if w.CheckoutDepth == 0 {
+		if err := runGit("fetch", fetchRemote, fetchRef); err != nil {
+			return err
+		}
+	} else {
+		if err := runGit("fetch", "--depth", fmt.Sprint(w.CheckoutDepth), fetchRemote, fetchRef); err != nil {
+			return err
+		}
+	}
 
 	if w.GpgNoSigningEnabled {
 		if err := runGit("config", "--local", "commit.gpgsign", "false"); err != nil {
@@ -361,3 +368,8 @@ func (w *FileWorkspace) sanitizeGitCredentials(s string, base models.Repo, head
 	baseReplaced := strings.Replace(s, base.CloneURL, base.SanitizedCloneURL, -1)
 	return strings.Replace(baseReplaced, head.CloneURL, head.SanitizedCloneURL, -1)
 }
+
+// Set the flag that indicates it is safe to re-clone if necessary
+func (w *FileWorkspace) SetSafeToReClone() {
+	w.SafeToReClone = true
+}

server/events/working_dir_test.go

@@ -480,6 +480,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Assert(t, hasDiverged == false, "Clone with CheckoutMerge=false should not merge")
 
 	wd.CheckoutMerge = true
+	wd.SetSafeToReClone()
 	// 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.
@@ -491,6 +492,7 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 	Ok(t, err)
 	Assert(t, hasDiverged == true, "First clone with CheckoutMerge=true with diverged base should have merged")
 
+	wd.SetSafeToReClone()
 	_, hasDiverged, err = wd.Clone(logging.NewNoopLogger(t), models.Repo{CloneURL: repoDir}, models.PullRequest{
 		BaseRepo:   models.Repo{CloneURL: repoDir},
 		HeadBranch: "second-pr",