From 8eeb2877d5803d0501815466d651a519b32bbd3a Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 10 Nov 2019 08:42:51 +0000 Subject: [PATCH] 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 | 137 ++++++++++++++ models/error.go | 73 ++++++++ modules/git/command.go | 11 +- options/locale/locale_en-US.ini | 4 + routers/api/v1/repo/pull.go | 12 ++ routers/repo/pull.go | 30 +++ services/pull/merge.go | 311 ++++++++++++++++++++++---------- 7 files changed, 483 insertions(+), 95 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index 27f9fc6bb9..a63e27e149 100644 --- a/integrations/pull_merge_test.go +++ b/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") + }) +} diff --git a/models/error.go b/models/error.go index 505df28868..313c36354d 100644 --- a/models/error.go +++ b/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) +} + // _________ __ // \_ ___ \ ____ _____ _____ ____ _____/ |_ // / \ \/ / _ \ / \ / \_/ __ \ / \ __\ diff --git a/modules/git/command.go b/modules/git/command.go index 347dcfe39f..2b5288aeab 100644 --- a/modules/git/command.go +++ b/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 diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 4433c5bb2a..43b56f4ec7 100644 --- a/options/locale/locale_en-US.ini +++ b/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
%[2]s
Hint: Try a different strategy +pulls.rebase_conflict = Merge Failed: There was a conflict whilst rebasing commit: %s[1]
%[1]s
%[2]s
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 diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 6d86105a15..6af1ba1b04 100644 --- a/routers/api/v1/repo/pull.go +++ b/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 diff --git a/routers/repo/pull.go b/routers/repo/pull.go index cb9c7c1164..0eea5fcbe6 100644 --- a/routers/repo/pull.go +++ b/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", "
", -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 diff --git a/services/pull/merge.go b/services/pull/merge.go index c6607910a2..5fbf550ad2 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -32,22 +32,27 @@ import ( func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repository, mergeStyle models.MergeStyle, message string) (err error) { binVersion, err := git.BinVersion() if err != nil { + log.Error("git.BinVersion: %v", err) return fmt.Errorf("Unable to get git version: %v", err) } if err = pr.GetHeadRepo(); err != nil { + log.Error("GetHeadRepo: %v", err) return fmt.Errorf("GetHeadRepo: %v", err) } else if err = pr.GetBaseRepo(); err != nil { + log.Error("GetBaseRepo: %v", err) return fmt.Errorf("GetBaseRepo: %v", err) } prUnit, err := pr.BaseRepo.GetUnit(models.UnitTypePullRequests) if err != nil { + log.Error("pr.BaseRepo.GetUnit(models.UnitTypePullRequests): %v", err) return err } prConfig := prUnit.PullRequestsConfig() if err := pr.CheckUserAllowedToMerge(doer); err != nil { + log.Error("CheckUserAllowedToMerge(%v): %v", doer, err) return fmt.Errorf("CheckUserAllowedToMerge: %v", err) } @@ -63,6 +68,7 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor // Clone base repo. tmpBasePath, err := models.CreateTemporaryPath("merge") if err != nil { + log.Error("CreateTemporaryPath: %v", err) return err } @@ -75,7 +81,8 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor headRepoPath := pr.HeadRepo.RepoPath() if err := git.InitRepository(tmpBasePath, false); err != nil { - return fmt.Errorf("git init: %v", err) + log.Error("git init tmpBasePath: %v", err) + return err } remoteRepoName := "head_repo" @@ -86,102 +93,141 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor p := filepath.Join(staging, ".git", "objects", "info", "alternates") f, err := os.OpenFile(p, os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0600) if err != nil { + log.Error("Could not create .git/objects/info/alternates file in %s: %v", staging, err) return err } defer f.Close() data := filepath.Join(cache, "objects") if _, err := fmt.Fprintln(f, data); err != nil { + log.Error("Could not write to .git/objects/info/alternates file in %s: %v", staging, err) return err } return nil } if err := addCacheRepo(tmpBasePath, baseGitRepo.Path); err != nil { - return fmt.Errorf("addCacheRepo [%s -> %s]: %v", headRepoPath, tmpBasePath, err) + log.Error("Unable to add base repository to temporary repo [%s -> %s]: %v", pr.BaseRepo.FullName(), tmpBasePath, err) + return fmt.Errorf("Unable to add base repository to temporary repo [%s -> tmpBasePath]: %v", pr.BaseRepo.FullName(), err) } - var errbuf strings.Builder - if err := git.NewCommand("remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseGitRepo.Path).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git remote add [%s -> %s]: %s", baseGitRepo.Path, tmpBasePath, errbuf.String()) + var outbuf, errbuf strings.Builder + if err := git.NewCommand("remote", "add", "-t", pr.BaseBranch, "-m", pr.BaseBranch, "origin", baseGitRepo.Path).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to add base repository as origin [%s -> %s]: %v\n%s\n%s", pr.BaseRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to add base repository as origin [%s -> tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() - if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git fetch [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + if err := git.NewCommand("fetch", "origin", "--no-tags", pr.BaseBranch+":"+baseBranch, pr.BaseBranch+":original_"+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to fetch origin base branch [%s:%s -> base, original_base in %s]: %v:\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to fetch origin base branch [%s:%s -> base, original_base in tmpBasePath]: %v\n%s\n%s", pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() - if err := git.NewCommand("symbolic-ref", "HEAD", git.BranchPrefix+baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git symbolic-ref HEAD base [%s]: %s", tmpBasePath, errbuf.String()) + if err := git.NewCommand("symbolic-ref", "HEAD", git.BranchPrefix+baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to set HEAD as base branch [%s]: %v\n%s\n%s", tmpBasePath, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to set HEAD as base branch [tmpBasePath]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() if err := addCacheRepo(tmpBasePath, headRepoPath); err != nil { - return fmt.Errorf("addCacheRepo [%s -> %s]: %v", headRepoPath, tmpBasePath, err) + log.Error("Unable to add head repository to temporary repo [%s -> %s]: %v", pr.HeadRepo.FullName(), tmpBasePath, err) + return fmt.Errorf("Unable to head base repository to temporary repo [%s -> tmpBasePath]: %v", pr.HeadRepo.FullName(), err) } - if err := git.NewCommand("remote", "add", remoteRepoName, headRepoPath).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git remote add [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + if err := git.NewCommand("remote", "add", remoteRepoName, headRepoPath).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to add head repository as head_repo [%s -> %s]: %v\n%s\n%s", pr.HeadRepo.FullName(), tmpBasePath, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to add head repository as head_repo [%s -> tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() trackingBranch := "tracking" // Fetch head branch - if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git fetch [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + if err := git.NewCommand("fetch", "--no-tags", remoteRepoName, pr.HeadBranch+":"+trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("Unable to fetch head_repo head branch [%s:%s -> tracking in %s]: %v:\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, tmpBasePath, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to fetch head_repo head branch [%s:%s -> tracking in tmpBasePath]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() stagingBranch := "staging" // Enable sparse-checkout sparseCheckoutList, err := getDiffTree(tmpBasePath, baseBranch, trackingBranch) if err != nil { + log.Error("getDiffTree(%s, %s, %s): %v", tmpBasePath, baseBranch, trackingBranch, err) return fmt.Errorf("getDiffTree: %v", err) } infoPath := filepath.Join(tmpBasePath, ".git", "info") if err := os.MkdirAll(infoPath, 0700); err != nil { - return fmt.Errorf("creating directory failed [%s]: %v", infoPath, err) + log.Error("Unable to create .git/info in %s: %v", tmpBasePath, err) + return fmt.Errorf("Unable to create .git/info in tmpBasePath: %v", err) } + sparseCheckoutListPath := filepath.Join(infoPath, "sparse-checkout") if err := ioutil.WriteFile(sparseCheckoutListPath, []byte(sparseCheckoutList), 0600); err != nil { - return fmt.Errorf("Writing sparse-checkout file to %s: %v", sparseCheckoutListPath, err) + log.Error("Unable to write .git/info/sparse-checkout file in %s: %v", tmpBasePath, err) + return fmt.Errorf("Unable to write .git/info/sparse-checkout file in tmpBasePath: %v", err) } - gitConfigCommand := func() func() *git.Command { - binVersion, err := git.BinVersion() - if err != nil { - log.Fatal("Error retrieving git version: %v", err) + var gitConfigCommand func() *git.Command + if version.Compare(binVersion, "1.8.0", ">=") { + gitConfigCommand = func() *git.Command { + return git.NewCommand("config", "--local") } - - if version.Compare(binVersion, "1.8.0", ">=") { - return func() *git.Command { - return git.NewCommand("config", "--local") - } - } - return func() *git.Command { + } else { + gitConfigCommand = func() *git.Command { return git.NewCommand("config") } - }() + } // Switch off LFS process (set required, clean and smudge here also) - if err := gitConfigCommand().AddArguments("filter.lfs.process", "").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git config [filter.lfs.process -> <> ]: %v", errbuf.String()) - } - if err := gitConfigCommand().AddArguments("filter.lfs.required", "false").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git config [filter.lfs.required -> ]: %v", errbuf.String()) - } - if err := gitConfigCommand().AddArguments("filter.lfs.clean", "").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git config [filter.lfs.clean -> <> ]: %v", errbuf.String()) - } - if err := gitConfigCommand().AddArguments("filter.lfs.smudge", "").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git config [filter.lfs.smudge -> <> ]: %v", errbuf.String()) + if err := gitConfigCommand().AddArguments("filter.lfs.process", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git config [filter.lfs.process -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git config [filter.lfs.process -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() - if err := gitConfigCommand().AddArguments("core.sparseCheckout", "true").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git config [core.sparsecheckout -> true]: %v", errbuf.String()) + if err := gitConfigCommand().AddArguments("filter.lfs.required", "false").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git config [filter.lfs.required -> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git config [filter.lfs.required -> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() + + if err := gitConfigCommand().AddArguments("filter.lfs.clean", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git config [filter.lfs.clean -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git config [filter.lfs.clean -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + if err := gitConfigCommand().AddArguments("filter.lfs.smudge", "").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git config [filter.lfs.smudge -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git config [filter.lfs.smudge -> <> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() + + if err := gitConfigCommand().AddArguments("core.sparseCheckout", "true").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git config [core.sparseCheckout -> true ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git config [core.sparsecheckout -> true]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + } + outbuf.Reset() + errbuf.Reset() // Read base branch index - if err := git.NewCommand("read-tree", "HEAD").RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git read-tree HEAD: %s", errbuf.String()) + if err := git.NewCommand("read-tree", "HEAD").RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git read-tree HEAD: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) + return fmt.Errorf("Unable to read base branch in to the index: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() // Determine if we should sign signArg := "" @@ -210,80 +256,102 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor // Merge commits. switch mergeStyle { case models.MergeStyleMerge: - if err := git.NewCommand("merge", "--no-ff", "--no-commit", trackingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git merge --no-ff --no-commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) + cmd := git.NewCommand("merge", "--no-ff", "--no-commit", trackingBranch) + if err := runMergeCommand(pr, mergeStyle, cmd, tmpBasePath); err != nil { + log.Error("Unable to merge tracking into base: %v", err) + return err } - if signArg == "" { - if err := git.NewCommand("commit", "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) - } - } else { - if err := git.NewCommand("commit", signArg, "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) - } + if err := commitAndSignNoAuthor(pr, message, signArg, tmpBasePath, env); err != nil { + log.Error("Unable to make final commit: %v", err) + return err } case models.MergeStyleRebase: - // Checkout head branch - if err := git.NewCommand("checkout", "-b", stagingBranch, trackingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git checkout: %s", errbuf.String()) - } - // Rebase before merging - if err := git.NewCommand("rebase", "-q", baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git rebase [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) - } - // Checkout base branch again - if err := git.NewCommand("checkout", baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git checkout: %s", errbuf.String()) - } - // Merge fast forward - if err := git.NewCommand("merge", "--ff-only", "-q", stagingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git merge --ff-only [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) - } + fallthrough case models.MergeStyleRebaseMerge: // Checkout head branch - if err := git.NewCommand("checkout", "-b", stagingBranch, trackingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git checkout: %s", errbuf.String()) + if err := git.NewCommand("checkout", "-b", stagingBranch, trackingBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() + // Rebase before merging - if err := git.NewCommand("rebase", "-q", baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git rebase [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + if err := git.NewCommand("rebase", baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + // Rebase will leave a REBASE_HEAD file in .git if there is a conflict + if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "REBASE_HEAD")); statErr == nil { + // The original commit SHA1 that is failing will be in .git/rebase-apply/original-commit + commitShaBytes, readErr := ioutil.ReadFile(filepath.Join(tmpBasePath, ".git", "rebase-apply", "original-commit")) + if readErr != nil { + // Abandon this attempt to handle the error + log.Error("git rebase staging on to base [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git rebase staging on to base [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + } + log.Debug("RebaseConflict at %s [%s:%s -> %s:%s]: %v\n%s\n%s", strings.TrimSpace(string(commitShaBytes)), pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return models.ErrRebaseConflicts{ + Style: mergeStyle, + CommitSHA: strings.TrimSpace(string(commitShaBytes)), + StdOut: outbuf.String(), + StdErr: errbuf.String(), + Err: err, + } + } + log.Error("git rebase staging on to base [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git rebase staging on to base [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() + // Checkout base branch again - if err := git.NewCommand("checkout", baseBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git checkout: %s", errbuf.String()) - } - // Prepare merge with commit - if err := git.NewCommand("merge", "--no-ff", "--no-commit", "-q", stagingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git merge --no-ff [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + if err := git.NewCommand("checkout", baseBranch).RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git checkout base prior to merge post staging rebase [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } + outbuf.Reset() + errbuf.Reset() - // Set custom message and author and create merge commit - if signArg == "" { - if err := git.NewCommand("commit", "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) - } + cmd := git.NewCommand("merge") + if mergeStyle == models.MergeStyleRebase { + cmd.AddArguments("--ff-only") } else { - if err := git.NewCommand("commit", signArg, "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) + cmd.AddArguments("--no-ff", "--no-commit") + } + cmd.AddArguments(stagingBranch) + + // Prepare merge with commit + if err := runMergeCommand(pr, mergeStyle, cmd, tmpBasePath); err != nil { + log.Error("Unable to merge staging into base: %v", err) + return err + } + if mergeStyle == models.MergeStyleRebaseMerge { + if err := commitAndSignNoAuthor(pr, message, signArg, tmpBasePath, env); err != nil { + log.Error("Unable to make final commit: %v", err) + return err } } - case models.MergeStyleSquash: // Merge with squash - if err := git.NewCommand("merge", "-q", "--squash", trackingBranch).RunInDirPipeline(tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git merge --squash [%s -> %s]: %s", headRepoPath, tmpBasePath, errbuf.String()) + cmd := git.NewCommand("merge", "--squash", trackingBranch) + if err := runMergeCommand(pr, mergeStyle, cmd, tmpBasePath); err != nil { + log.Error("Unable to merge --squash tracking into base: %v", err) + return err } + sig := pr.Issue.Poster.NewGitSig() if signArg == "" { - if err := git.NewCommand("commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) + if err := git.NewCommand("commit", fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } } else { - if err := git.NewCommand("commit", signArg, fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { - return fmt.Errorf("git commit [%s]: %v - %s", tmpBasePath, err, errbuf.String()) + if err := git.NewCommand("commit", signArg, fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email), "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) } } + outbuf.Reset() + errbuf.Reset() default: return models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} } @@ -329,9 +397,19 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor ) // Push back to upstream. - if err := git.NewCommand("push", "origin", baseBranch+":"+pr.BaseBranch).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, nil, &errbuf); err != nil { + if err := git.NewCommand("push", "origin", baseBranch+":"+pr.BaseBranch).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + if strings.Contains(errbuf.String(), "non-fast-forward") { + return models.ErrMergePushOutOfDate{ + Style: mergeStyle, + StdOut: outbuf.String(), + StdErr: errbuf.String(), + Err: err, + } + } return fmt.Errorf("git push: %s", errbuf.String()) } + outbuf.Reset() + errbuf.Reset() pr.MergedCommitID, err = baseGitRepo.GetBranchCommitID(pr.BaseBranch) if err != nil { @@ -364,6 +442,51 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor return nil } +func commitAndSignNoAuthor(pr *models.PullRequest, message, signArg, tmpBasePath string, env []string) error { + var outbuf, errbuf strings.Builder + if signArg == "" { + if err := git.NewCommand("commit", "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + } + } else { + if err := git.NewCommand("commit", signArg, "-m", message).RunInDirTimeoutEnvPipeline(env, -1, tmpBasePath, &outbuf, &errbuf); err != nil { + log.Error("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git commit [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + } + } + return nil +} + +func runMergeCommand(pr *models.PullRequest, mergeStyle models.MergeStyle, cmd *git.Command, tmpBasePath string) error { + var outbuf, errbuf strings.Builder + if err := cmd.RunInDirPipeline(tmpBasePath, &outbuf, &errbuf); err != nil { + // Merge will leave a MERGE_HEAD file in the .git folder if there is a conflict + if _, statErr := os.Stat(filepath.Join(tmpBasePath, ".git", "MERGE_HEAD")); statErr == nil { + // We have a merge conflict error + log.Debug("MergeConflict [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return models.ErrMergeConflicts{ + Style: mergeStyle, + StdOut: outbuf.String(), + StdErr: errbuf.String(), + Err: err, + } + } else if strings.Contains(errbuf.String(), "refusing to merge unrelated histories") { + log.Debug("MergeUnrelatedHistories [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return models.ErrMergeUnrelatedHistories{ + Style: mergeStyle, + StdOut: outbuf.String(), + StdErr: errbuf.String(), + Err: err, + } + } + log.Error("git merge [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + return fmt.Errorf("git merge [%s:%s -> %s:%s]: %v\n%s\n%s", pr.HeadRepo.FullName(), pr.HeadBranch, pr.BaseRepo.FullName(), pr.BaseBranch, err, outbuf.String(), errbuf.String()) + } + + return nil +} + var escapedSymbols = regexp.MustCompile(`([*[?! \\])`) func getDiffTree(repoPath, baseBranch, headBranch string) (string, error) {