Skip to content

Commit 8eeb287

Browse files
zeripathzeripath
authored
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.
1 parent 31416a5 commit 8eeb287

File tree

7 files changed

+480
-92
lines changed

7 files changed

+480
-92
lines changed

‎integrations/pull_merge_test.go‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,22 @@
55
package integrations
66

77
import (
8+
"bytes"
9+
"fmt"
810
"net/http"
911
"net/http/httptest"
1012
"net/url"
13+
"os"
1114
"path"
1215
"strings"
1316
"testing"
17+
"time"
1418

1519
"code.gitea.io/gitea/models"
20+
"code.gitea.io/gitea/modules/git"
21+
api "code.gitea.io/gitea/modules/structs"
1622
"code.gitea.io/gitea/modules/test"
23+
"code.gitea.io/gitea/services/pull"
1724

1825
"github.com/stretchr/testify/assert"
1926
"github.com/unknwon/i18n"
@@ -202,3 +209,133 @@ func TestCantMergeWorkInProgress(t *testing.T) {
202209
assert.Equal(t, replacer.Replace(expected), text, "Unable to find WIP text")
203210
})
204211
}
212+
213+
func TestCantMergeConflict(t *testing.T) {
214+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
215+
prepareTestEnv(t)
216+
session := loginUser(t, "user1")
217+
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
218+
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
219+
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
220+
221+
// Use API to create a conflicting pr
222+
token := getTokenForLoggedInUser(t, session)
223+
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", "user1", "repo1", token), &api.CreatePullRequestOption{
224+
Head: "conflict",
225+
Base: "base",
226+
Title: "create a conflicting pr",
227+
})
228+
session.MakeRequest(t, req, 201)
229+
230+
// Now this PR will be marked conflict - or at least a race will do - so drop down to pure code at this point...
231+
user1 := models.AssertExistsAndLoadBean(t, &models.User{
232+
Name: "user1",
233+
}).(*models.User)
234+
repo1 := models.AssertExistsAndLoadBean(t, &models.Repository{
235+
OwnerID: user1.ID,
236+
Name: "repo1",
237+
}).(*models.Repository)
238+
239+
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{
240+
HeadRepoID: repo1.ID,
241+
BaseRepoID: repo1.ID,
242+
HeadBranch: "conflict",
243+
BaseBranch: "base",
244+
}).(*models.PullRequest)
245+
246+
gitRepo, err := git.OpenRepository(models.RepoPath(user1.Name, repo1.Name))
247+
assert.NoError(t, err)
248+
249+
err = pull.Merge(pr, user1, gitRepo, models.MergeStyleMerge, "CONFLICT")
250+
assert.Error(t, err, "Merge should return an error due to conflict")
251+
assert.True(t, models.IsErrMergeConflicts(err), "Merge error is not a conflict error")
252+
253+
err = pull.Merge(pr, user1, gitRepo, models.MergeStyleRebase, "CONFLICT")
254+
assert.Error(t, err, "Merge should return an error due to conflict")
255+
assert.True(t, models.IsErrRebaseConflicts(err), "Merge error is not a conflict error")
256+
})
257+
}
258+
259+
func TestCantMergeUnrelated(t *testing.T) {
260+
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
261+
prepareTestEnv(t)
262+
session := loginUser(t, "user1")
263+
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
264+
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")
265+
266+
// Now we want to create a commit on a branch that is totally unrelated to our current head
267+
// Drop down to pure code at this point
268+
user1 := models.AssertExistsAndLoadBean(t, &models.User{
269+
Name: "user1",
270+
}).(*models.User)
271+
repo1 := models.AssertExistsAndLoadBean(t, &models.Repository{
272+
OwnerID: user1.ID,
273+
Name: "repo1",
274+
}).(*models.Repository)
275+
path := models.RepoPath(user1.Name, repo1.Name)
276+
277+
_, err := git.NewCommand("read-tree", "--empty").RunInDir(path)
278+
assert.NoError(t, err)
279+
280+
stdin := bytes.NewBufferString("Unrelated File")
281+
var stdout strings.Builder
282+
err = git.NewCommand("hash-object", "-w", "--stdin").RunInDirFullPipeline(path, &stdout, nil, stdin)
283+
assert.NoError(t, err)
284+
sha := strings.TrimSpace(stdout.String())
285+
286+
_, err = git.NewCommand("update-index", "--add", "--replace", "--cacheinfo", "100644", sha, "somewher-over-the-rainbow").RunInDir(path)
287+
assert.NoError(t, err)
288+
289+
treeSha, err := git.NewCommand("write-tree").RunInDir(path)
290+
assert.NoError(t, err)
291+
treeSha = strings.TrimSpace(treeSha)
292+
293+
commitTimeStr := time.Now().Format(time.RFC3339)
294+
doerSig := user1.NewGitSig()
295+
env := append(os.Environ(),
296+
"GIT_AUTHOR_NAME="+doerSig.Name,
297+
"GIT_AUTHOR_EMAIL="+doerSig.Email,
298+
"GIT_AUTHOR_DATE="+commitTimeStr,
299+
"GIT_COMMITTER_NAME="+doerSig.Name,
300+
"GIT_COMMITTER_EMAIL="+doerSig.Email,
301+
"GIT_COMMITTER_DATE="+commitTimeStr,
302+
)
303+
304+
messageBytes := new(bytes.Buffer)
305+
_, _ = messageBytes.WriteString("Unrelated")
306+
_, _ = messageBytes.WriteString("\n")
307+
308+
stdout.Reset()
309+
err = git.NewCommand("commit-tree", treeSha).RunInDirTimeoutEnvFullPipeline(env, -1, path, &stdout, nil, messageBytes)
310+
assert.NoError(t, err)
311+
commitSha := strings.TrimSpace(stdout.String())
312+
313+
_, err = git.NewCommand("branch", "unrelated", commitSha).RunInDir(path)
314+
assert.NoError(t, err)
315+
316+
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
317+
318+
// Use API to create a conflicting pr
319+
token := getTokenForLoggedInUser(t, session)
320+
req := NewRequestWithJSON(t, http.MethodPost, fmt.Sprintf("/api/v1/repos/%s/%s/pulls?token=%s", "user1", "repo1", token), &api.CreatePullRequestOption{
321+
Head: "unrelated",
322+
Base: "base",
323+
Title: "create an unrelated pr",
324+
})
325+
session.MakeRequest(t, req, 201)
326+
327+
// Now this PR could be marked conflict - or at least a race may occur - so drop down to pure code at this point...
328+
gitRepo, err := git.OpenRepository(path)
329+
assert.NoError(t, err)
330+
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{
331+
HeadRepoID: repo1.ID,
332+
BaseRepoID: repo1.ID,
333+
HeadBranch: "unrelated",
334+
BaseBranch: "base",
335+
}).(*models.PullRequest)
336+
337+
err = pull.Merge(pr, user1, gitRepo, models.MergeStyleMerge, "UNRELATED")
338+
assert.Error(t, err, "Merge should return an error due to unrelated")
339+
assert.True(t, models.IsErrMergeUnrelatedHistories(err), "Merge error is not a unrelated histories error")
340+
})
341+
}

‎models/error.go‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,6 +1206,79 @@ func (err ErrInvalidMergeStyle) Error() string {
12061206
err.ID, err.Style)
12071207
}
12081208

1209+
// ErrMergeConflicts represents an error if merging fails with a conflict
1210+
type ErrMergeConflicts struct {
1211+
Style MergeStyle
1212+
StdOut string
1213+
StdErr string
1214+
Err error
1215+
}
1216+
1217+
// IsErrMergeConflicts checks if an error is a ErrMergeConflicts.
1218+
func IsErrMergeConflicts(err error) bool {
1219+
_, ok := err.(ErrMergeConflicts)
1220+
return ok
1221+
}
1222+
1223+
func (err ErrMergeConflicts) Error() string {
1224+
return fmt.Sprintf("Merge Conflict Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
1225+
}
1226+
1227+
// ErrMergeUnrelatedHistories represents an error if merging fails due to unrelated histories
1228+
type ErrMergeUnrelatedHistories struct {
1229+
Style MergeStyle
1230+
StdOut string
1231+
StdErr string
1232+
Err error
1233+
}
1234+
1235+
// IsErrMergeUnrelatedHistories checks if an error is a ErrMergeUnrelatedHistories.
1236+
func IsErrMergeUnrelatedHistories(err error) bool {
1237+
_, ok := err.(ErrMergeUnrelatedHistories)
1238+
return ok
1239+
}
1240+
1241+
func (err ErrMergeUnrelatedHistories) Error() string {
1242+
return fmt.Sprintf("Merge UnrelatedHistories Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
1243+
}
1244+
1245+
// ErrMergePushOutOfDate represents an error if merging fails due to unrelated histories
1246+
type ErrMergePushOutOfDate struct {
1247+
Style MergeStyle
1248+
StdOut string
1249+
StdErr string
1250+
Err error
1251+
}
1252+
1253+
// IsErrMergePushOutOfDate checks if an error is a ErrMergePushOutOfDate.
1254+
func IsErrMergePushOutOfDate(err error) bool {
1255+
_, ok := err.(ErrMergePushOutOfDate)
1256+
return ok
1257+
}
1258+
1259+
func (err ErrMergePushOutOfDate) Error() string {
1260+
return fmt.Sprintf("Merge PushOutOfDate Error: %v: %s\n%s", err.Err, err.StdErr, err.StdOut)
1261+
}
1262+
1263+
// ErrRebaseConflicts represents an error if rebase fails with a conflict
1264+
type ErrRebaseConflicts struct {
1265+
Style MergeStyle
1266+
CommitSHA string
1267+
StdOut string
1268+
StdErr string
1269+
Err error
1270+
}
1271+
1272+
// IsErrRebaseConflicts checks if an error is a ErrRebaseConflicts.
1273+
func IsErrRebaseConflicts(err error) bool {
1274+
_, ok := err.(ErrRebaseConflicts)
1275+
return ok
1276+
}
1277+
1278+
func (err ErrRebaseConflicts) Error() string {
1279+
return fmt.Sprintf("Rebase Error: %v: Whilst Rebasing: %s\n%s\n%s", err.Err, err.CommitSHA, err.StdErr, err.StdOut)
1280+
}
1281+
12091282
// _________ __
12101283
// \_ ___ \ ____ _____ _____ ____ _____/ |_
12111284
// / \ \/ / _ \ / \ / \_/ __ \ / \ __\

‎modules/git/command.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"context"
1010
"fmt"
1111
"io"
12+
"os"
1213
"os/exec"
1314
"strings"
1415
"time"
@@ -24,6 +25,9 @@ var (
2425
DefaultCommandExecutionTimeout = 60 * time.Second
2526
)
2627

28+
// DefaultLocale is the default LC_ALL to run git commands in.
29+
const DefaultLocale = "C"
30+
2731
// Command represents a command with its subcommands or arguments.
2832
type Command struct {
2933
name string
@@ -77,7 +81,12 @@ func (c *Command) RunInDirTimeoutEnvFullPipeline(env []string, timeout time.Dura
7781
defer cancel()
7882

7983
cmd := exec.CommandContext(ctx, c.name, c.args...)
80-
cmd.Env = env
84+
if env == nil {
85+
cmd.Env = append(os.Environ(), fmt.Sprintf("LC_ALL=%s", DefaultLocale))
86+
} else {
87+
cmd.Env = env
88+
cmd.Env = append(cmd.Env, fmt.Sprintf("LC_ALL=%s", DefaultLocale))
89+
}
8190
cmd.Dir = dir
8291
cmd.Stdout = stdout
8392
cmd.Stderr = stderr

‎options/locale/locale_en-US.ini‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1026,6 +1026,10 @@ pulls.rebase_merge_pull_request = Rebase and Merge
10261026
pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)
10271027
pulls.squash_merge_pull_request = Squash and Merge
10281028
pulls.invalid_merge_option = You cannot use this merge option for this pull request.
1029+
pulls.merge_conflict = Merge Failed: There was a conflict whilst merging: %[1]s<br>%[2]s<br>Hint: Try a different strategy
1030+
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
1031+
pulls.unrelated_histories = Merge Failed: The merge head and base do not share a common history. Hint: Try a different strategy
1032+
pulls.merge_out_of_date = Merge Failed: Whilst generating the merge, the base was updated. Hint: Try again.
10291033
pulls.open_unmerged_pull_exists = `You cannot perform a reopen operation because there is a pending pull request (#%d) with identical properties.`
10301034
pulls.status_checking = Some checks are pending
10311035
pulls.status_checks_success = All checks were successful

‎routers/api/v1/repo/pull.go‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -626,6 +626,18 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
626626
if models.IsErrInvalidMergeStyle(err) {
627627
ctx.Status(405)
628628
return
629+
} else if models.IsErrMergeConflicts(err) {
630+
conflictError := err.(models.ErrMergeConflicts)
631+
ctx.JSON(http.StatusConflict, conflictError)
632+
} else if models.IsErrRebaseConflicts(err) {
633+
conflictError := err.(models.ErrRebaseConflicts)
634+
ctx.JSON(http.StatusConflict, conflictError)
635+
} else if models.IsErrMergeUnrelatedHistories(err) {
636+
conflictError := err.(models.ErrMergeUnrelatedHistories)
637+
ctx.JSON(http.StatusConflict, conflictError)
638+
} else if models.IsErrMergePushOutOfDate(err) {
639+
ctx.Status(http.StatusConflict)
640+
return
629641
}
630642
ctx.Error(500, "Merge", err)
631643
return

‎routers/repo/pull.go‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"container/list"
1111
"crypto/subtle"
1212
"fmt"
13+
"html"
1314
"io"
1415
"path"
1516
"strings"
@@ -660,10 +661,39 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
660661
}
661662

662663
if err = pull_service.Merge(pr, ctx.User, ctx.Repo.GitRepo, models.MergeStyle(form.Do), message); err != nil {
664+
sanitize := func(x string) string {
665+
runes := []rune(x)
666+
667+
if len(runes) > 512 {
668+
x = "..." + string(runes[len(runes)-512:])
669+
}
670+
671+
return strings.Replace(html.EscapeString(x), "\n", "<br>", -1)
672+
}
663673
if models.IsErrInvalidMergeStyle(err) {
664674
ctx.Flash.Error(ctx.Tr("repo.pulls.invalid_merge_option"))
665675
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
666676
return
677+
} else if models.IsErrMergeConflicts(err) {
678+
conflictError := err.(models.ErrMergeConflicts)
679+
ctx.Flash.Error(ctx.Tr("repo.pulls.merge_conflict", sanitize(conflictError.StdErr), sanitize(conflictError.StdOut)))
680+
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
681+
return
682+
} else if models.IsErrRebaseConflicts(err) {
683+
conflictError := err.(models.ErrRebaseConflicts)
684+
ctx.Flash.Error(ctx.Tr("repo.pulls.rebase_conflict", sanitize(conflictError.CommitSHA), sanitize(conflictError.StdErr), sanitize(conflictError.StdOut)))
685+
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
686+
return
687+
} else if models.IsErrMergeUnrelatedHistories(err) {
688+
log.Debug("MergeUnrelatedHistories error: %v", err)
689+
ctx.Flash.Error(ctx.Tr("repo.pulls.unrelated_histories"))
690+
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
691+
return
692+
} else if models.IsErrMergePushOutOfDate(err) {
693+
log.Debug("MergePushOutOfDate error: %v", err)
694+
ctx.Flash.Error(ctx.Tr("repo.pulls.merge_out_of_date"))
695+
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
696+
return
667697
}
668698
ctx.ServerError("Merge", err)
669699
return

0 commit comments

Comments
 (0)