Refactor branch diverged warning.

- only check for divergence if using checkout strategy merge

server/events/markdown_renderer.go

@@ -229,7 +229,7 @@ var planSuccessUnwrappedTmpl = template.Must(template.New("").Parse(
 	"```diff\n" +
 		"{{.TerraformOutput}}\n" +
 		"```\n\n" + planNextSteps +
-		"{{ if .HasDiverged }}\n\n:warning: Master branch is ahead, it is recommended to pull new commits first.{{end}}"))
+		"{{ if .HasDiverged }}\n\n:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.{{end}}"))
 
 var planSuccessWrappedTmpl = template.Must(template.New("").Parse(
 	"<details><summary>Show Output</summary>\n\n" +
@@ -238,7 +238,7 @@ var planSuccessWrappedTmpl = template.Must(template.New("").Parse(
 		"```\n\n" +
 		planNextSteps + "\n" +
 		"</details>" +
-		"{{ if .HasDiverged }}\n\n:warning: Master branch is ahead, it is recommended to pull new commits first.{{end}}"))
+		"{{ if .HasDiverged }}\n\n:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.{{end}}"))
 
 // planNextSteps are instructions appended after successful plans as to what
 // to do next.

server/events/markdown_renderer_test.go

@@ -190,7 +190,7 @@ $$$
 * :repeat: To **plan** this project again, comment:
     * $atlantis plan -d path -w workspace$
 
-:warning: Master branch is ahead, it is recommended to pull new commits first.
+:warning: The branch we're merging into is ahead, it is recommended to pull new commits first.
 
 ---
 * :fast_forward: To **apply** all unapplied plans from this pull request, comment:

server/events/models/models.go

@@ -431,7 +431,9 @@ type PlanSuccess struct {
 	RePlanCmd string
 	// ApplyCmd is the command that users should run to apply this plan.
 	ApplyCmd string
-	// Indicates if remote master has diverged
+	// 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
 }
 

server/events/working_dir.go

@@ -34,7 +34,9 @@ const workingDirPrefix = "repos"
 // WorkingDir handles the workspace on disk for running commands.
 type WorkingDir interface {
 	// Clone git clones headRepo, checks out the branch and then returns the
-	// absolute path to the root of the cloned repo as well as if master branch has diverged.
+	// 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.
 	Clone(log *logging.SimpleLogger, baseRepo models.Repo, headRepo models.Repo, p models.PullRequest, workspace string) (string, bool, error)
 	// GetWorkingDir returns the path to the workspace for this repo and pull.
 	// If workspace does not exist on disk, error will be of type os.IsNotExist.
@@ -92,61 +94,81 @@ func (w *FileWorkspace) Clone(
 		outputRevParseCmd, err := revParseCmd.CombinedOutput()
 		if err != nil {
 			log.Warn("will re-clone repo, could not determine if was at correct commit: %s: %s: %s", strings.Join(revParseCmd.Args, " "), err, string(outputRevParseCmd))
-			return w.forceClone(log, cloneDir, headRepo, p)
+			return cloneDir, false, w.forceClone(log, cloneDir, headRepo, p)
 		}
 		currCommit := strings.Trim(string(outputRevParseCmd), "\n")
 
-		// Bring our remote refs up to date.
-		remoteUpdateCmd := exec.Command("git", "remote", "update")
-		remoteUpdateCmd.Dir = cloneDir
-		outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput()
-		if err != nil {
-			log.Warn("getting remote update failed: %s", string(outputRemoteUpdate))
-		}
-
-		// Check if remote master branch has diverged.
-		statusUnoCmd := exec.Command("git", "status", "--untracked-files=no")
-		statusUnoCmd.Dir = cloneDir
-		outputStatusUno, err := statusUnoCmd.CombinedOutput()
-		if err != nil {
-			log.Warn("getting repo status has failed: %s", string(outputStatusUno))
-		}
-		status := strings.Trim(string(outputStatusUno), "\n")
-		hasDiverged := strings.Contains(status, "have diverged")
-		if hasDiverged {
-			log.Info("remote master branch is ahead and thereby has new commits, it is recommended to pull new commits")
-		} else {
-			log.Debug("remote master branch has no new commits")
-		}
-
 		// 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, hasDiverged, nil
+			return cloneDir, w.warnDiverged(log, cloneDir), nil
 		}
+
 		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 w.forceClone(log, cloneDir, headRepo, p)
+	return cloneDir, false, 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.
+// This matters in the case of the merge checkout strategy because after
+// cloning the repo and doing the merge, it's possible master was updated.
+// Then users won't be getting the merge functionality they expected.
+// 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.SimpleLogger, cloneDir string) bool {
+	if !w.CheckoutMerge {
+		// It only makes sense to warn that master has diverged if we're using
+		// the checkout merge strategy. If we're just checking out the branch,
+		// then it doesn't matter what's going on with master because we've
+		// decided to always run off the branch.
+		return false
+	}
+
+	// Bring our remote refs up to date.
+	remoteUpdateCmd := exec.Command("git", "remote", "update")
+	remoteUpdateCmd.Dir = cloneDir
+	outputRemoteUpdate, err := remoteUpdateCmd.CombinedOutput()
+	if err != nil {
+		log.Warn("getting remote update failed: %s", string(outputRemoteUpdate))
+		return false
+	}
+
+	// Check if remote master branch has diverged.
+	statusUnoCmd := exec.Command("git", "status", "--untracked-files=no")
+	statusUnoCmd.Dir = cloneDir
+	outputStatusUno, err := statusUnoCmd.CombinedOutput()
+	if err != nil {
+		log.Warn("getting repo status has failed: %s", string(outputStatusUno))
+		return false
+	}
+	hasDiverged := strings.Contains(string(outputStatusUno), "have diverged")
+	if hasDiverged {
+		log.Info("remote master branch is ahead and thereby has new commits, it is recommended to pull new commits")
+	} else {
+		log.Debug("remote master branch has no new commits")
+	}
+	return hasDiverged
 }
 
 func (w *FileWorkspace) forceClone(log *logging.SimpleLogger,
 	cloneDir string,
 	headRepo models.Repo,
-	p models.PullRequest) (string, bool, error) {
+	p models.PullRequest) error {
 
 	err := os.RemoveAll(cloneDir)
 	if err != nil {
-		return "", false, errors.Wrapf(err, "deleting dir %q before cloning", cloneDir)
+		return errors.Wrapf(err, "deleting dir %q before cloning", cloneDir)
 	}
 
 	// Create the directory and parents if necessary.
 	log.Info("creating dir %q", cloneDir)
 	if err := os.MkdirAll(cloneDir, 0700); err != nil {
-		return "", false, errors.Wrap(err, "creating new workspace")
+		return errors.Wrap(err, "creating new workspace")
 	}
 
 	// During testing, we mock some of this out.
@@ -208,11 +230,11 @@ func (w *FileWorkspace) forceClone(log *logging.SimpleLogger,
 		sanitizedOutput := w.sanitizeGitCredentials(string(output), p.BaseRepo, headRepo)
 		if err != nil {
 			sanitizedErrMsg := w.sanitizeGitCredentials(err.Error(), p.BaseRepo, headRepo)
-			return "", false, fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
+			return fmt.Errorf("running %s: %s: %s", cmdStr, sanitizedOutput, sanitizedErrMsg)
 		}
 		log.Debug("ran: %s. Output: %s", cmdStr, strings.TrimSuffix(sanitizedOutput, "\n"))
 	}
-	return cloneDir, false, nil
+	return nil
 }
 
 // GetWorkingDir returns the path to the workspace for this repo and pull.

server/events/working_dir_test.go

@@ -27,14 +27,11 @@ func TestClone_NoneExisting(t *testing.T) {
 		TestingOverrideHeadCloneURL: fmt.Sprintf("file://%s", repoDir),
 	}
 
-	cloneDir, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
+	cloneDir, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
 		HeadBranch: "branch",
 	}, "default")
 	Ok(t, err)
 
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
-
 	// Use rev-parse to verify at correct commit.
 	actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD")
 	Equals(t, expCommit, actCommit)
@@ -84,9 +81,7 @@ func TestClone_CheckoutMergeNoneExisting(t *testing.T) {
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Check the commits.
 	actBaseCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD~1")
@@ -134,9 +129,7 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) {
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Create a file that we can use to check if the repo was recloned.
 	runCmd(t, dataDir, "touch", "repos/0/default/proof")
@@ -147,9 +140,7 @@ func TestClone_CheckoutMergeNoReclone(t *testing.T) {
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Check that our proof file is still there, proving that we didn't reclone.
 	_, err = os.Stat(filepath.Join(cloneDir, "proof"))
@@ -186,9 +177,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) {
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Create a file that we can use to check if the repo was recloned.
 	runCmd(t, dataDir, "touch", "repos/0/default/proof")
@@ -199,9 +188,7 @@ func TestClone_CheckoutMergeNoRecloneFastForward(t *testing.T) {
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Check that our proof file is still there, proving that we didn't reclone.
 	_, err = os.Stat(filepath.Join(cloneDir, "proof"))
@@ -238,14 +225,11 @@ func TestClone_CheckoutMergeConflict(t *testing.T) {
 		TestingOverrideBaseCloneURL: overrideURL,
 	}
 
-	_, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
+	_, _, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
 		HeadBranch: "branch",
 		BaseBranch: "master",
 	}, "default")
 
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
-
 	ErrContains(t, "running git merge -q --no-ff -m atlantis-merge FETCH_HEAD", err)
 	ErrContains(t, "Auto-merging file", err)
 	ErrContains(t, "CONFLICT (add/add)", err)
@@ -275,9 +259,7 @@ func TestClone_NoReclone(t *testing.T) {
 		HeadBranch: "branch",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Check that our proof file is still there.
 	_, err = os.Stat(filepath.Join(cloneDir, "proof"))
@@ -313,16 +295,15 @@ func TestClone_RecloneWrongCommit(t *testing.T) {
 		HeadCommit: expCommit,
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo has not diverged
-	Equals(t, hasDiverged, false)
+	Equals(t, false, hasDiverged)
 
 	// Use rev-parse to verify at correct commit.
 	actCommit := runCmd(t, cloneDir, "git", "rev-parse", "HEAD")
 	Equals(t, expCommit, actCommit)
 }
 
-// Test that if master (remote) repo has diverged, see issue 804
+// Test that if the branch we're merging into has diverged and we're using
+// checkout-strategy=merge, we warn the user (see #804).
 func TestClone_MasterHasDiverged(t *testing.T) {
 	// Initialize the git repo.
 	repoDir, cleanup := initRepo(t)
@@ -374,15 +355,22 @@ func TestClone_MasterHasDiverged(t *testing.T) {
 		DataDir:       repoDir,
 		CheckoutMerge: true,
 	}
-
 	_, hasDiverged, err := wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
 		HeadBranch: "second-pr",
 		BaseBranch: "master",
 	}, "default")
 	Ok(t, err)
-
-	// Check if master repo is ahead and has diverged
 	Equals(t, hasDiverged, true)
+
+	// Run it again but without the checkout merge strategy. It should return
+	// false.
+	wd.CheckoutMerge = false
+	_, hasDiverged, err = wd.Clone(nil, models.Repo{}, models.Repo{}, models.PullRequest{
+		HeadBranch: "second-pr",
+		BaseBranch: "master",
+	}, "default")
+	Ok(t, err)
+	Equals(t, hasDiverged, false)
 }
 
 func initRepo(t *testing.T) (string, func()) {