diff --git a/server/events/working_dir.go b/server/events/working_dir.go index c8f006aa30..fa4087d6ad 100644 --- a/server/events/working_dir.go +++ b/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 { diff --git a/server/events/working_dir_test.go b/server/events/working_dir_test.go index 458329140b..2622c67f9a 100644 --- a/server/events/working_dir_test.go +++ b/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) {