Merge pull request ActiveState#3318 from ActiveState/mitchell/dx-2842

Do not update to merge commit if there was a buildscript conflict.

Author: mitchell-as

internal/runners/pull/pull.go

@@ -2,7 +2,6 @@ package pull
 
 import (
 	"errors"
-	"path/filepath"
 	"strings"
 	"time"
 
@@ -82,6 +81,14 @@ func (o *pullOutput) MarshalStructured(format output.Format) interface{} {
 	return o
 }
 
+type ErrBuildScriptMergeConflict struct {
+	ProjectDir string
+}
+
+func (e *ErrBuildScriptMergeConflict) Error() string {
+	return "build script merge conflict"
+}
+
 func (p *Pull) Run(params *PullParams) (rerr error) {
 	defer rationalizeError(&rerr)
 
@@ -157,18 +164,24 @@ func (p *Pull) Run(params *PullParams) (rerr error) {
 	}
 
 	if commitID != *resultingCommit {
-		err := localcommit.Set(p.project.Dir(), resultingCommit.String())
-		if err != nil {
-			return errs.Wrap(err, "Unable to set local commit")
-		}
-
 		if p.cfg.GetBool(constants.OptinBuildscriptsConfig) {
 			err := p.mergeBuildScript(*remoteCommit, *localCommit)
 			if err != nil {
+				if errs.Matches(err, &ErrBuildScriptMergeConflict{}) {
+					err2 := localcommit.Set(p.project.Dir(), remoteCommit.String())
+					if err2 != nil {
+						err = errs.Pack(err, errs.Wrap(err2, "Could not set local commit to remote commit after build script merge conflict"))
+					}
+				}
 				return errs.Wrap(err, "Could not merge local build script with remote changes")
 			}
 		}
 
+		err := localcommit.Set(p.project.Dir(), resultingCommit.String())
+		if err != nil {
+			return errs.Wrap(err, "Unable to set local commit")
+		}
+
 		p.out.Print(&pullOutput{
 			locale.Tr("pull_updated", remoteProject.String(), resultingCommit.String()),
 			true,
@@ -260,10 +273,7 @@ func (p *Pull) mergeBuildScript(remoteCommit, localCommit strfmt.UUID) error {
 		if err != nil {
 			return locale.WrapError(err, "err_diff_build_script", "Unable to generate differences between local and remote build script")
 		}
-		return locale.NewInputError(
-			"err_build_script_merge",
-			"Unable to automatically merge build scripts. Please resolve conflicts manually in '{{.V0}}' and then run '[ACTIONABLE]state commit[/RESET]'",
-			filepath.Join(p.project.Dir(), constants.BuildScriptFileName))
+		return &ErrBuildScriptMergeConflict{p.project.Dir()}
 	}
 
 	// For now, pick the later of the script AtTimes.

internal/runners/pull/rationalize.go

@@ -2,7 +2,9 @@ package pull
 
 import (
 	"errors"
+	"path/filepath"
 
+	"github.com/ActiveState/cli/internal/constants"
 	"github.com/ActiveState/cli/internal/errs"
 	"github.com/ActiveState/cli/internal/locale"
 	"github.com/ActiveState/cli/pkg/platform/api/buildplanner/model"
@@ -14,6 +16,7 @@ func rationalizeError(err *error) {
 	}
 
 	var mergeCommitErr *model.MergedCommitError
+	var buildscriptMergeCommitErr *ErrBuildScriptMergeConflict
 
 	switch {
 	case errors.As(*err, &mergeCommitErr):
@@ -44,5 +47,12 @@ func rationalizeError(err *error) {
 				),
 			)
 		}
+
+	case errors.As(*err, &buildscriptMergeCommitErr):
+		*err = errs.WrapUserFacing(*err,
+			locale.Tl("err_build_script_merge",
+				"Unable to automatically merge build scripts. Please resolve conflicts manually in '{{.V0}}' and then run '[ACTIONABLE]state commit[/RESET]'",
+				filepath.Join(buildscriptMergeCommitErr.ProjectDir, constants.BuildScriptFileName)),
+			errs.SetInput())
 	}
 }

test/integration/pull_int_test.go

@@ -114,12 +114,14 @@ func (suite *PullIntegrationTestSuite) TestMergeBuildScript() {
 	suite.Assert().Contains(string(bytes), "=======", "No merge conflict markers are in build script")
 	suite.Assert().Contains(string(bytes), ">>>>>>>", "No merge conflict markers are in build script")
 
-	// Verify the local commit was updated to the merge commit.
-	// Note: even though the buildscript merge failed, a merge commit was still created. After resolving
-	// buildscript conflicts, `state commit` should have something new to commit.
+	// Verify the local commit was updated to the remote commit, not the merge commit.
+	// Note: even though the buildscript merge failed, a merge commit was still created (we just
+	// ignore it). After resolving buildscript conflicts, `state commit` should always have something
+	// new to commit.
+	remoteHeadCommit := "d908a758-6a81-40d4-b0eb-87069cd7f07d"
 	commit, err := localcommit.Get(ts.Dirs.Work)
 	suite.Require().NoError(err)
-	suite.Assert().NotEqual(commit.String(), "447b8363-024c-4143-bf4e-c96989314fdf", "localcommit not updated to merged commit")
+	suite.Assert().Equal(remoteHeadCommit, commit.String(), "localcommit should have been updated to remote commit")
 }
 
 func (suite *PullIntegrationTestSuite) assertMergeStrategyNotification(ts *e2e.Session, strategy string) {