fix: safer re-merging with updated upstream

server/events/markdown_renderer_test.go

@@ -191,7 +191,7 @@ $$$
 						LockURL:         "lock-url",
 						RePlanCmd:       "atlantis plan -d path -w workspace",
 						ApplyCmd:        "atlantis apply -d path -w workspace",
-						HasDiverged:     true,
+						MergedAgain:     true,
 					},
 					Workspace:  "workspace",
 					RepoRelDir: "path",
@@ -210,7 +210,7 @@ $$$
 * :repeat: To **plan** this project again, comment:
     * $atlantis plan -d path -w workspace$
 
-:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.
+:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
 
 ---
 * :fast_forward: To **apply** all unapplied plans from this pull request, comment:
@@ -1974,7 +1974,7 @@ $$$
 						LockURL:         "lock-url",
 						RePlanCmd:       "atlantis plan -d path -w workspace",
 						ApplyCmd:        "atlantis apply -d path -w workspace",
-						HasDiverged:     true,
+						MergedAgain:     true,
 					},
 					Workspace:  "workspace",
 					RepoRelDir: "path",
@@ -1992,7 +1992,7 @@ $$$
 * :repeat: To **plan** this project again, comment:
     * $atlantis plan -d path -w workspace$
 
-:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.
+:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
 
 ---
 * :fast_forward: To **apply** all unapplied plans from this pull request, comment:

server/events/models/models.go

@@ -361,10 +361,10 @@ type PlanSuccess struct {
 	RePlanCmd string
 	// ApplyCmd is the command that users should run to apply this plan.
 	ApplyCmd string
-	// HasDiverged is true if we're using the checkout merge strategy and the
-	// branch we're merging into has been updated since we cloned and merged
-	// it.
-	HasDiverged bool
+	// MergedAgain is true if we're using the checkout merge strategy and the
+	// branch we're merging into had been updated, and we had to merge again
+	// before planning
+	MergedAgain bool
 }
 
 type PolicySetResult struct {

server/events/project_command_runner.go

@@ -543,9 +543,9 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 	}
 	defer unlockFn()
 
-	p.WorkingDir.SetSafeToReClone()
+	p.WorkingDir.SetCheckForUpstreamChanges()
 	// Clone is idempotent so okay to run even if the repo was already cloned.
-	repoDir, hasDiverged, cloneErr := p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, ctx.Workspace)
+	repoDir, mergedAgain, cloneErr := p.WorkingDir.Clone(ctx.HeadRepo, ctx.Pull, ctx.Workspace)
 	if cloneErr != nil {
 		if unlockErr := lockAttempt.UnlockFn(); unlockErr != nil {
 			ctx.Log.Err("error unlocking state after plan error: %v", unlockErr)
@@ -576,7 +576,7 @@ func (p *DefaultProjectCommandRunner) doPlan(ctx command.ProjectContext) (*model
 		TerraformOutput: strings.Join(outputs, "\n"),
 		RePlanCmd:       ctx.RePlanCmd,
 		ApplyCmd:        ctx.ApplyCmd,
-		HasDiverged:     hasDiverged,
+		MergedAgain:     mergedAgain,
 	}, "", nil
 }
 

server/events/templates/merged_again.tmpl

@@ -0,0 +1,5 @@
+{{ define "mergedAgain" -}}
+{{ if .MergedAgain }}
+:twisted_rightwards_arrows: Upstream was modified, a new merge was performed.
+{{ end -}}
+{{ end -}}

server/events/templates/plan_success_unwrapped.tmpl

@@ -16,5 +16,5 @@ This plan was not saved because one or more projects failed and automerge requir
 * :repeat: To **plan** this project again, comment:
     * `{{ .RePlanCmd }}`
 {{ end -}}
-{{ template "diverged" . }}
+{{ template "mergedAgain" . }}
 {{ end -}}

server/events/templates/plan_success_wrapped.tmpl

@@ -20,5 +20,5 @@ This plan was not saved because one or more projects failed and automerge requir
 {{ end -}}
 </details>
 {{ .PlanSummary -}}
-{{ template "diverged" . -}}
+{{ template "mergedAgain" . -}}
 {{ end -}}

server/events/working_dir.go

@@ -53,7 +53,7 @@
 	// 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()
+	SetCheckForUpstreamChanges()
 	// DeletePlan deletes the plan for this repo, pull, workspace path and project name
 	DeletePlan(r models.Repo, p models.PullRequest, workspace string, path string, projectName string) error
 }
@@ -82,9 +82,9 @@
 	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
-	Logger        logging.SimpleLogging
+	// flag indicating if we have to merge with potential new changes upstream (directly after grabbing project lock)
+	CheckForUpstreamChanges bool
+	Logger                  logging.SimpleLogging
 }
 
 // Clone git clones headRepo, checks out the branch and then returns the absolute
@@ -98,8 +98,7 @@
 	p models.PullRequest,
 	workspace string) (string, bool, error) {
 	cloneDir := w.cloneDir(p.BaseRepo, p, workspace)
-	hasDiverged := false
-	defer func() { w.SafeToReClone = false }()
+	defer func() { w.CheckForUpstreamChanges = false }()
 
 	// If the directory already exists, check if it's at the right commit.
 	// If so, then we do nothing.
@@ -127,9 +126,9 @@
 		// 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.SafeToReClone && w.CheckoutMerge && w.recheckDiverged(p, headRepo, cloneDir) {
+			if w.CheckForUpstreamChanges && w.CheckoutMerge && w.recheckDiverged(p, headRepo, cloneDir) {
 				w.Logger.Info("base branch has been updated, using merge strategy and will clone again")
-				hasDiverged = true
+				return cloneDir, true, w.mergeAgain(cloneDir, headRepo, p)
 			} else {
 				w.Logger.Debug("repo is at correct commit %q so will not re-clone", p.HeadCommit)
 				return cloneDir, false, nil
@@ -141,7 +140,7 @@
 	}
 
 	// Otherwise we clone the repo.
-	return cloneDir, hasDiverged, w.forceClone(cloneDir, headRepo, p)
+	return cloneDir, false, w.forceClone(cloneDir, headRepo, p)
 }
 
 // recheckDiverged returns true if the branch we're merging into has diverged
@@ -241,49 +240,79 @@
 		baseCloneURL = w.TestingOverrideBaseCloneURL
 	}
 
-	runGit := func(args ...string) error {
-		cmd := exec.Command("git", args...) // nolint: gosec
-		cmd.Dir = cloneDir
-		// The git merge command requires these env vars are set.
-		cmd.Env = append(os.Environ(), []string{
-			"[email protected]",
-			"GIT_AUTHOR_NAME=atlantis",
-			"GIT_COMMITTER_NAME=atlantis",
-		}...)
-
-		cmdStr := w.sanitizeGitCredentials(strings.Join(cmd.Args, " "), p.BaseRepo, headRepo)
-		output, err := cmd.CombinedOutput()
-		sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo)
-		if err != nil {
-			sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo)
-			return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
-		}
-		w.Logger.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n"))
-		return nil
-	}
-
 	// if branch strategy, use depth=1
 	if !w.CheckoutMerge {
-		return runGit("clone", "--depth=1", "--branch", p.HeadBranch, "--single-branch", headCloneURL, cloneDir)
+		return w.runGit(cloneDir, headRepo, p, "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 {
+		if err := w.runGit(cloneDir, headRepo, p, "clone", "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
 			return err
 		}
 	} else {
-		if err := runGit("clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
+		if err := w.runGit(cloneDir, headRepo, p, "clone", "--depth", fmt.Sprint(w.CheckoutDepth), "--branch", p.BaseBranch, "--single-branch", baseCloneURL, cloneDir); err != nil {
 			return err
 		}
 	}
 
-	if err := runGit("remote", "add", "head", headCloneURL); err != nil {
+	if err := w.runGit(cloneDir, headRepo, p, "remote", "add", "head", headCloneURL); err != nil {
 		return err
 	}
+	if w.GpgNoSigningEnabled {
+		if err := w.runGit(cloneDir, headRepo, p, "config", "--local", "commit.gpgsign", "false"); err != nil {
+			return err
+		}
+	}
 
+	return w.mergeToBaseBranch(cloneDir, headRepo, p)
+}
+
+// There is a new upstream update that we need, and we want to update to it
+// without deleting any existing plans
+func (w *FileWorkspace) mergeAgain(cloneDir string, headRepo models.Repo, p models.PullRequest) error {
+	value, _ := cloneLocks.LoadOrStore(cloneDir, new(sync.Mutex))
+	mutex := value.(*sync.Mutex)
+
+	defer mutex.Unlock()
+	if locked := mutex.TryLock(); !locked {
+		mutex.Lock()
+		return nil
+	}
+
+	// Reset branch as if it was cloned again
+	if err := w.runGit(cloneDir, headRepo, p, "reset", "--hard", fmt.Sprintf("refs/remotes/head/%s", p.BaseBranch)); err != nil {
+		return err
+	}
+
+	return w.mergeToBaseBranch(cloneDir, headRepo, p)
+}
+
+func (w *FileWorkspace) runGit(cloneDir string, headRepo models.Repo, p models.PullRequest, args ...string) error {
+	cmd := exec.Command("git", args...) // nolint: gosec
+	cmd.Dir = cloneDir
+	// The git merge command requires these env vars are set.
+	cmd.Env = append(os.Environ(), []string{
+		"[email protected]",
+		"GIT_AUTHOR_NAME=atlantis",
+		"GIT_COMMITTER_NAME=atlantis",
+	}...)
+
+	cmdStr := w.sanitizeGitCredentials(strings.Join(cmd.Args, " "), p.BaseRepo, headRepo)
+	output, err := cmd.CombinedOutput()
+	sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo)
+	if err != nil {
+		sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo)
+		return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
+	}
+	w.Logger.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n"))
+	return nil
+}
+
+// Merge the PR into the base branch.
+func (w *FileWorkspace) mergeToBaseBranch(cloneDir string, headRepo models.Repo, p models.PullRequest) error {
 	fetchRef := fmt.Sprintf("+refs/heads/%s:", p.HeadBranch)
 	fetchRemote := "head"
 	if w.GithubAppEnabled {
@@ -293,24 +322,19 @@
 
 	// if no checkout depth, omit depth arg
 	if w.CheckoutDepth == 0 {
-		if err := runGit("fetch", fetchRemote, fetchRef); err != nil {
+		if err := w.runGit(cloneDir, headRepo, p, "fetch", fetchRemote, fetchRef); err != nil {
 			return err
 		}
 	} else {
-		if err := runGit("fetch", "--depth", fmt.Sprint(w.CheckoutDepth), fetchRemote, fetchRef); err != nil {
+		if err := w.runGit(cloneDir, headRepo, p, "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 {
-			return err
-		}
-	}
-	if err := runGit("merge-base", p.BaseBranch, "FETCH_HEAD"); err != nil {
+	if err := w.runGit(cloneDir, headRepo, p, "merge-base", p.BaseBranch, "FETCH_HEAD"); err != nil {
 		// git merge-base returning error means that we did not receive enough commits in shallow clone.
 		// Fall back to retrieving full repo history.
-		if err := runGit("fetch", "--unshallow"); err != nil {
+		if err := w.runGit(cloneDir, headRepo, p, "fetch", "--unshallow"); err != nil {
 			return err
 		}
 	}
@@ -321,7 +345,7 @@
 	// git rev-parse HEAD^2 to get the head commit because it will
 	// always succeed whereas without --no-ff, if the merge was fast
 	// forwarded then git rev-parse HEAD^2 would fail.
-	return runGit("merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD")
+	return w.runGit(cloneDir, headRepo, p, "merge", "-q", "--no-ff", "-m", "atlantis-merge", "FETCH_HEAD")
 }
 
 // GetWorkingDir returns the path to the workspace for this repo and pull.
@@ -372,9 +396,9 @@
 	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
+// Set the flag that indicates we need to check for upstream changes (if using merge checkout strategy)
+func (w *FileWorkspace) SetCheckForUpstreamChanges() {
+	w.CheckForUpstreamChanges = true
 }
 
 func (w *FileWorkspace) DeletePlan(r models.Repo, p models.PullRequest, workspace string, projectPath string, projectName string) error {