Adjust error reporting from merge failures and use LC_ALL=C for git (#8548)

There are two major components to this PR:

* This PR handles merge and rebase failures from merging a little more nicely with Flash errors rather a 500.
* All git commands are run in the LC_ALL="C" environment to ensure that error messages are in English. This DefaultLocale is defined in a way that if necessary (due to platform weirdness) it can be overridden at build time using LDFLAGS="-X "code.gitea.io/gitea/modules/git.DefaultLocale=C"" with C changed for the locale as necessary.

integrations/pull_merge_test.go

@@ -5,15 +5,22 @@
 package integrations
 
 import (
+	"bytes"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"os"
 	"path"
 	"strings"
 	"testing"
+	"time"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/modules/git"
+	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/test"
+	"code.gitea.io/gitea/services/pull"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/unknwon/i18n"
@@ -202,3 +209,133 @@ func TestCantMergeWorkInProgress(t *testing.T) {
 		assert.Equal(t, replacer.Replace(expected), text, "Unable to find WIP text")
 	})
 }
+
+func TestCantMergeConflict(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		prepareTestEnv(t)
+		session := loginUser(t, "user1")
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
+		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
+		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
+
+		// Use API to create a conflicting pr
+		token := getTokenForLoggedInUser(t, session)
+		req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", "user1", "repo1", token), &api.CreatePullRequestOption{
+			Head:  "conflict",
+			Base:  "base",
+			Title: "create a conflicting pr",
+		})
+		session.MakeRequest(t, req, 201)
+
+		// Now this PR will be marked conflict - or at least a race will do - so drop down to pure code at this point...
+		user1 := models.AssertExistsAndLoadBean(t, &models.User{
+			Name: "user1",
+		}).(*models.User)
+		repo1 := models.AssertExistsAndLoadBean(t, &models.Repository{
+			OwnerID: user1.ID,
+			Name:    "repo1",
+		}).(*models.Repository)
+
+		pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{
+			HeadRepoID: repo1.ID,
+			BaseRepoID: repo1.ID,
+			HeadBranch: "conflict",
+			BaseBranch: "base",
+		}).(*models.PullRequest)
+
+		gitRepo, err := git.OpenRepository(models.RepoPath(user1.Name, repo1.Name))
+		assert.NoError(t, err)
+
+		err = pull.Merge(pr, user1, gitRepo, models.MergeStyleMerge, "CONFLICT")
+		assert.Error(t, err, "Merge should return an error due to conflict")
+		assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error")
+
+		err = pull.Merge(pr, user1, gitRepo, models.MergeStyleRebase, "CONFLICT")
+		assert.Error(t, err, "Merge should return an error due to conflict")
+		assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error")
+	})
+}
+
+func TestCantMergeUnrelated(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
+		prepareTestEnv(t)
+		session := loginUser(t, "user1")
+		testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
+		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
+
+		// Now we want to create a commit on a branch that is totally unrelated to our current head
+		// Drop down to pure code at this point
+		user1 := models.AssertExistsAndLoadBean(t, &models.User{
+			Name: "user1",
+		}).(*models.User)
+		repo1 := models.AssertExistsAndLoadBean(t, &models.Repository{
+			OwnerID: user1.ID,
+			Name:    "repo1",
+		}).(*models.Repository)
+		path := models.RepoPath(user1.Name, repo1.Name)
+
+		_, err := git.NewCommand("read-tree", "--empty").RunInDir(path)
+		assert.NoError(t, err)
+
+		stdin := bytes.NewBufferString("Unrelated File")
+		var stdout strings.Builder
+		err = git.NewCommand("hash-object", "-w", "--stdin").RunInDirFullPipeline(path, &stdout, nil, stdin)
+		assert.NoError(t, err)
+		sha := strings.TrimSpace(stdout.String())
+
+		_, err = git.NewCommand("update-index", "--add", "--replace", "--cacheinfo", "100644", sha, "somewher-over-the-rainbow").RunInDir(path)
+		assert.NoError(t, err)
+
+		treeSha, err := git.NewCommand("write-tree").RunInDir(path)
+		assert.NoError(t, err)
+		treeSha = strings.TrimSpace(treeSha)
+
+		commitTimeStr := time.Now().Format(time.RFC3339)
+		doerSig := user1.NewGitSig()
+		env := append(os.Environ(),
+			"GIT_AUTHOR_NAME="+doerSig.Name,
+			"GIT_AUTHOR_EMAIL="+doerSig.Email,
+			"GIT_AUTHOR_DATE="+commitTimeStr,
+			"GIT_COMMITTER_NAME="+doerSig.Name,
+			"GIT_COMMITTER_EMAIL="+doerSig.Email,
+			"GIT_COMMITTER_DATE="+commitTimeStr,
+		)
+
+		messageBytes := new(bytes.Buffer)
+		_, _ = messageBytes.WriteString("Unrelated")
+		_, _ = messageBytes.WriteString("\n")
+
+		stdout.Reset()
+		err = git.NewCommand("commit-tree", treeSha).RunInDirTimeoutEnvFullPipeline(env, -1, path, &stdout, nil, messageBytes)
+		assert.NoError(t, err)
+		commitSha := strings.TrimSpace(stdout.String())
+
+		_, err = git.NewCommand("branch", "unrelated", commitSha).RunInDir(path)
+		assert.NoError(t, err)
+
+		testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
+
+		// Use API to create a conflicting pr
+		token := getTokenForLoggedInUser(t, session)
+		req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", "user1", "repo1", token), &api.CreatePullRequestOption{
+			Head:  "unrelated",
+			Base:  "base",
+			Title: "create an unrelated pr",
+		})
+		session.MakeRequest(t, req, 201)
+
+		// Now this PR could be marked conflict - or at least a race may occur - so drop down to pure code at this point...
+		gitRepo, err := git.OpenRepository(path)
+		assert.NoError(t, err)
+		pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{
+			HeadRepoID: repo1.ID,
+			BaseRepoID: repo1.ID,
+			HeadBranch: "unrelated",
+			BaseBranch: "base",
+		}).(*models.PullRequest)
+
+		err = pull.Merge(pr, user1, gitRepo, models.MergeStyleMerge, "UNRELATED")
+		assert.Error(t, err, "Merge should return an error due to unrelated")
+		assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error")
+	})
+}

models/error.go

@@ -1206,6 +1206,79 @@ func (err ErrInvalidMergeStyle) Error() string {
 		err.ID, err.Style)
 }
 
+// ErrMergeConflicts represents an error if merging fails with a conflict
+type ErrMergeConflicts struct {
+	Style  MergeStyle
+	StdOut string
+	StdErr string
+	Err    error
+}
+
+// IsErrMergeConflicts checks if an error is a ErrMergeConflicts.
+func IsErrMergeConflicts(err error) bool {
+	_, ok := err.(ErrMergeConflicts)
+	return ok
+}
+
+func (err ErrMergeConflicts) Error() string {
+	return fmt.Sprintf("Merge Conflict Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
+}
+
+// ErrMergeUnrelatedHistories represents an error if merging fails due to unrelated histories
+type ErrMergeUnrelatedHistories struct {
+	Style  MergeStyle
+	StdOut string
+	StdErr string
+	Err    error
+}
+
+// IsErrMergeUnrelatedHistories checks if an error is a ErrMergeUnrelatedHistories.
+func IsErrMergeUnrelatedHistories(err error) bool {
+	_, ok := err.(ErrMergeUnrelatedHistories)
+	return ok
+}
+
+func (err ErrMergeUnrelatedHistories) Error() string {
+	return fmt.Sprintf("Merge UnrelatedHistories Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
+}
+
+// ErrMergePushOutOfDate represents an error if merging fails due to unrelated histories
+type ErrMergePushOutOfDate struct {
+	Style  MergeStyle
+	StdOut string
+	StdErr string
+	Err    error
+}
+
+// IsErrMergePushOutOfDate checks if an error is a ErrMergePushOutOfDate.
+func IsErrMergePushOutOfDate(err error) bool {
+	_, ok := err.(ErrMergePushOutOfDate)
+	return ok
+}
+
+func (err ErrMergePushOutOfDate) Error() string {
+	return fmt.Sprintf("Merge PushOutOfDate Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
+}
+
+// ErrRebaseConflicts represents an error if rebase fails with a conflict
+type ErrRebaseConflicts struct {
+	Style     MergeStyle
+	CommitSHA string
+	StdOut    string
+	StdErr    string
+	Err       error
+}
+
+// IsErrRebaseConflicts checks if an error is a ErrRebaseConflicts.
+func IsErrRebaseConflicts(err error) bool {
+	_, ok := err.(ErrRebaseConflicts)
+	return ok
+}
+
+func (err ErrRebaseConflicts) Error() string {
+	return fmt.Sprintf("Rebase Error: %v: Whilst Rebasing: %s\n%s\n%s", err.Err, err.CommitSHA, err.StdErr, err.StdOut)
+}
+
 // _________                                       __
 // \_   ___ \  ____   _____   _____   ____   _____/  |_
 // /    \  \/ /  _ \ /     \ /     \_/ __ \ /    \   __\

modules/git/command.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
 	"strings"
 	"time"
@@ -24,6 +25,9 @@ var (
 	DefaultCommandExecutionTimeout = 60 * time.Second
 )
 
+// DefaultLocale is the default LC_ALL to run git commands in.
+const DefaultLocale = "C"
+
 // Command represents a command with its subcommands or arguments.
 type Command struct {
 	name string
@@ -77,7 +81,12 @@ func (c *Command) RunInDirTimeoutEnvFullPipeline(env []string, timeout time.Dura
 	defer cancel()
 
 	cmd := exec.CommandContext(ctx, c.name, c.args...)
-	cmd.Env = env
+	if env == nil {
+		cmd.Env = append(os.Environ(), fmt.Sprintf("LC_ALL=%s", DefaultLocale))
+	} else {
+		cmd.Env = env
+		cmd.Env = append(cmd.Env, fmt.Sprintf("LC_ALL=%s", DefaultLocale))
+	}
 	cmd.Dir = dir
 	cmd.Stdout = stdout
 	cmd.Stderr = stderr

options/locale/locale_en-US.ini

@@ -1026,6 +1026,10 @@ pulls.rebase_merge_pull_request = Rebase and Merge
 pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)
 pulls.squash_merge_pull_request = Squash and Merge
 pulls.invalid_merge_option = You cannot use this merge option for this pull request.
+pulls.merge_conflict = Merge Failed: There was a conflict whilst merging: %[1]s<br>%[2]s<br>Hint: Try a different strategy
+pulls.rebase_conflict = Merge Failed: There was a conflict whilst rebasing commit: %s[1]<br>%[1]s<br>%[2]s<br>Hint:Try a different strategy
+pulls.unrelated_histories = Merge Failed: The merge head and base do not share a common history. Hint: Try a different strategy
+pulls.merge_out_of_date = Merge Failed: Whilst generating the merge, the base was updated. Hint: Try again.
 pulls.open_unmerged_pull_exists = `You cannot perform a reopen operation because there is a pending pull request (#%d) with identical properties.`
 pulls.status_checking = Some checks are pending
 pulls.status_checks_success = All checks were successful

routers/api/v1/repo/pull.go

@@ -626,6 +626,18 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
 		if models.IsErrInvalidMergeStyle(err) {
 			ctx.Status(405)
 			return
+		} else if models.IsErrMergeConflicts(err) {
+			conflictError := err.(models.ErrMergeConflicts)
+			ctx.JSON(http.StatusConflict, conflictError)
+		} else if models.IsErrRebaseConflicts(err) {
+			conflictError := err.(models.ErrRebaseConflicts)
+			ctx.JSON(http.StatusConflict, conflictError)
+		} else if models.IsErrMergeUnrelatedHistories(err) {
+			conflictError := err.(models.ErrMergeUnrelatedHistories)
+			ctx.JSON(http.StatusConflict, conflictError)
+		} else if models.IsErrMergePushOutOfDate(err) {
+			ctx.Status(http.StatusConflict)
+			return
 		}
 		ctx.Error(500, "Merge", err)
 		return

routers/repo/pull.go

@@ -10,6 +10,7 @@ import (
 	"container/list"
 	"crypto/subtle"
 	"fmt"
+	"html"
 	"io"
 	"path"
 	"strings"
@@ -660,10 +661,39 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
 	}
 
 	if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, models.MergeStyle(form.Do), message); err != nil {
+		sanitize := func(x string) string {
+			runes := []rune(x)
+
+			if len(runes) > 512 {
+				x = "..." + string(runes[len(runes)-512:])
+			}
+
+			return strings.Replace(html.EscapeString(x), "\n", "<br>", -1)
+		}
 		if models.IsErrInvalidMergeStyle(err) {
 			ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
 			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
 			return
+		} else if models.IsErrMergeConflicts(err) {
+			conflictError := err.(models.ErrMergeConflicts)
+			ctx.Flash.Error(ctx.Tr("repo.pulls.merge_conflict", sanitize(conflictError.StdErr), sanitize(conflictError.StdOut)))
+			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
+			return
+		} else if models.IsErrRebaseConflicts(err) {
+			conflictError := err.(models.ErrRebaseConflicts)
+			ctx.Flash.Error(ctx.Tr("repo.pulls.rebase_conflict", sanitize(conflictError.CommitSHA), sanitize(conflictError.StdErr), sanitize(conflictError.StdOut)))
+			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
+			return
+		} else if models.IsErrMergeUnrelatedHistories(err) {
+			log.Debug("MergeUnrelatedHistories error: %v", err)
+			ctx.Flash.Error(ctx.Tr("repo.pulls.unrelated_histories"))
+			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
+			return
+		} else if models.IsErrMergePushOutOfDate(err) {
+			log.Debug("MergePushOutOfDate error: %v", err)
+			ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date"))
+			ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
+			return
 		}
 		ctx.ServerError("Merge", err)
 		return