From 875c5e13050e4f7b95773c3b8ef5deac8b4b581b Mon Sep 17 00:00:00 2001 From: zeripath Date: Sun, 9 Feb 2020 23:09:31 +0000 Subject: [PATCH] Only check for conflicts/merging if the PR has not been merged in the interim (#10132) * Only check for merging if the PR has not been merged in the interim * fixup! Only check for merging if the PR has not been merged in the interim * Try to fix test failure * Use PR2 not PR1 in tests as PR1 merges automatically * return already merged error * enforce locking * enforce locking - fix-test * enforce locking - fix-testx2 * enforce locking - fix-testx3 * move pullrequest checking to after merge This might improve the chance that the race does not affect us but does not prevent it. * Remove minor race with getting merge commit id * fixup * move check pr after merge * Remove unnecessary prepareTestEnv - onGiteaRun does this for us * Add information about when merging occuring * fix fmt * More logging * Attempt to fix mysql * Try MySQL fix again * try again * Try again?! * Try again?! * Sigh * remove the count - perhaps that will help * next remove the update id * next remove the update id - make it updated_unix instead * On failure to merge ensure that the pr is rechecked for conflict errors * On failure to merge ensure that the pr is rechecked for conflict errors * Update models/pull.go * Update models/pull.go Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> * Apply suggestions from code review Co-Authored-By: guillep2k <18600385+guillep2k@users.noreply.github.com> Co-authored-by: Lauris BH Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> --- integrations/pull_merge_test.go | 10 +--- models/pull.go | 68 +++++++++++++++++-------- services/pull/check.go | 8 +-- services/pull/check_test.go | 6 +-- services/pull/merge.go | 89 ++++++++++++++++++--------------- services/pull/update.go | 4 +- 6 files changed, 110 insertions(+), 75 deletions(-) diff --git a/integrations/pull_merge_test.go b/integrations/pull_merge_test.go index 218f0e4da..5a9283edc 100644 --- a/integrations/pull_merge_test.go +++ b/integrations/pull_merge_test.go @@ -105,8 +105,6 @@ func TestPullRebase(t *testing.T) { func TestPullRebaseMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer prepareTestEnv(t)() - hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -129,8 +127,6 @@ func TestPullRebaseMerge(t *testing.T) { func TestPullSquash(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer prepareTestEnv(t)() - hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number assert.NoError(t, err) hookTasksLenBefore := len(hookTasks) @@ -154,10 +150,9 @@ func TestPullSquash(t *testing.T) { func TestPullCleanUpAfterMerge(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer prepareTestEnv(t)() session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") - testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited)\n") + testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n") resp := testPullCreate(t, session, "user1", "repo1", "feature/test", "This is a pull title") @@ -190,7 +185,6 @@ func TestPullCleanUpAfterMerge(t *testing.T) { func TestCantMergeWorkInProgress(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer prepareTestEnv(t)() session := loginUser(t, "user1") testRepoFork(t, session, "user2", "repo1", "user1", "repo1") testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n") @@ -212,7 +206,6 @@ func TestCantMergeWorkInProgress(t *testing.T) { func TestCantMergeConflict(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer 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") @@ -258,7 +251,6 @@ func TestCantMergeConflict(t *testing.T) { func TestCantMergeUnrelated(t *testing.T) { onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) { - defer 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") diff --git a/models/pull.go b/models/pull.go index 8289c53cf..46c50986b 100644 --- a/models/pull.go +++ b/models/pull.go @@ -498,44 +498,66 @@ const ( ) // SetMerged sets a pull request to merged and closes the corresponding issue -func (pr *PullRequest) SetMerged() (err error) { +func (pr *PullRequest) SetMerged() (bool, error) { if pr.HasMerged { - return fmt.Errorf("PullRequest[%d] already merged", pr.Index) + return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index) } if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil { - return fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index) + return false, fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index) } pr.HasMerged = true sess := x.NewSession() defer sess.Close() - if err = sess.Begin(); err != nil { - return err + if err := sess.Begin(); err != nil { + return false, err } - if err = pr.loadIssue(sess); err != nil { - return err + if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil { + return false, err } - if err = pr.Issue.loadRepo(sess); err != nil { - return err - } - if err = pr.Issue.Repo.getOwner(sess); err != nil { - return err + if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil { + return false, err } - if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil { - return fmt.Errorf("Issue.changeStatus: %v", err) - } - if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { - return fmt.Errorf("update pull request: %v", err) + pr.Issue = nil + if err := pr.loadIssue(sess); err != nil { + return false, err } - if err = sess.Commit(); err != nil { - return fmt.Errorf("Commit: %v", err) + if tmpPr, err := getPullRequestByID(sess, pr.ID); err != nil { + return false, err + } else if tmpPr.HasMerged { + if pr.Issue.IsClosed { + return false, nil + } + return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID) + } else if pr.Issue.IsClosed { + return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index) } - return nil + + if err := pr.Issue.loadRepo(sess); err != nil { + return false, err + } + + if err := pr.Issue.Repo.getOwner(sess); err != nil { + return false, err + } + + if _, err := pr.Issue.changeStatus(sess, pr.Merger, true); err != nil { + return false, fmt.Errorf("Issue.changeStatus: %v", err) + } + + if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil { + return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err) + } + + if err := sess.Commit(); err != nil { + return false, fmt.Errorf("Commit: %v", err) + } + return true, nil } // NewPullRequest creates new pull request with labels for repository. @@ -707,6 +729,12 @@ func (pr *PullRequest) UpdateCols(cols ...string) error { return err } +// UpdateColsIfNotMerged updates specific fields of a pull request if it has not been merged +func (pr *PullRequest) UpdateColsIfNotMerged(cols ...string) error { + _, err := x.Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr) + return err +} + // IsWorkInProgress determine if the Pull Request is a Work In Progress by its title func (pr *PullRequest) IsWorkInProgress() bool { if err := pr.LoadIssue(); err != nil { diff --git a/services/pull/check.go b/services/pull/check.go index d64f49de3..17f9e047a 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -32,7 +32,7 @@ func AddToTaskQueue(pr *models.PullRequest) { go func() { err := prQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error { pr.Status = models.PullRequestStatusChecking - err := pr.UpdateCols("status") + err := pr.UpdateColsIfNotMerged("status") if err != nil { log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) } else { @@ -158,9 +158,11 @@ func manuallyMerged(pr *models.PullRequest) bool { pr.Merger = merger pr.MergerID = merger.ID - if err = pr.SetMerged(); err != nil { + if merged, err := pr.SetMerged(); err != nil { log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) return false + } else if !merged { + return false } notification.NotifyMergePullRequest(pr, merger) @@ -205,7 +207,7 @@ func handle(data ...queue.Data) { if err != nil { log.Error("GetPullRequestByID[%s]: %v", prID, err) continue - } else if pr.Status != models.PullRequestStatusChecking { + } else if pr.HasMerged { continue } else if manuallyMerged(pr) { continue diff --git a/services/pull/check_test.go b/services/pull/check_test.go index 4591edd7a..1bddc0b59 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -44,11 +44,11 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { prQueue = q.(queue.UniqueQueue) - pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest) AddToTaskQueue(pr) assert.Eventually(t, func() bool { - pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest) return pr.Status == models.PullRequestStatusChecking }, 1*time.Second, 100*time.Millisecond) @@ -73,7 +73,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { assert.False(t, has) assert.NoError(t, err) - pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest) assert.Equal(t, models.PullRequestStatusChecking, pr.Status) for _, callback := range queueShutdown { diff --git a/services/pull/merge.go b/services/pull/merge.go index 26c9ab3d1..8f3e18355 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -58,23 +58,30 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "") }() - if err := rawMerge(pr, doer, mergeStyle, message); err != nil { - return err - } - - pr.MergedCommitID, err = baseGitRepo.GetBranchCommitID(pr.BaseBranch) + pr.MergedCommitID, err = rawMerge(pr, doer, mergeStyle, message) if err != nil { - return fmt.Errorf("GetBranchCommit: %v", err) + return err } pr.MergedUnix = timeutil.TimeStampNow() pr.Merger = doer pr.MergerID = doer.ID - if err = pr.SetMerged(); err != nil { + if _, err = pr.SetMerged(); err != nil { log.Error("setMerged [%d]: %v", pr.ID, err) } + if err := pr.LoadIssue(); err != nil { + log.Error("loadIssue [%d]: %v", pr.ID, err) + } + + if err := pr.Issue.LoadRepo(); err != nil { + log.Error("loadRepo for issue [%d]: %v", pr.ID, err) + } + if err := pr.Issue.Repo.GetOwner(); err != nil { + log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err) + } + notification.NotifyMergePullRequest(pr, doer) // Reset cached commit count @@ -106,18 +113,18 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor } // rawMerge perform the merge operation without changing any pull information in database -func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.MergeStyle, message string) (err error) { +func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.MergeStyle, message string) (string, error) { binVersion, err := git.BinVersion() if err != nil { log.Error("git.BinVersion: %v", err) - return fmt.Errorf("Unable to get git version: %v", err) + return "", fmt.Errorf("Unable to get git version: %v", err) } // Clone base repo. tmpBasePath, err := createTemporaryRepo(pr) if err != nil { log.Error("CreateTemporaryPath: %v", err) - return err + return "", err } defer func() { if err := models.RemoveTemporaryPath(tmpBasePath); err != nil { @@ -135,19 +142,19 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge 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) + return "", fmt.Errorf("getDiffTree: %v", err) } infoPath := filepath.Join(tmpBasePath, ".git", "info") if err := os.MkdirAll(infoPath, 0700); err != nil { log.Error("Unable to create .git/info in %s: %v", tmpBasePath, err) - return fmt.Errorf("Unable to create .git/info in tmpBasePath: %v", 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 { 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) + return "", fmt.Errorf("Unable to write .git/info/sparse-checkout file in tmpBasePath: %v", err) } var gitConfigCommand func() *git.Command @@ -164,35 +171,35 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // Switch off LFS process (set required, clean and smudge here also) 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()) + 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("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()) + 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()) + 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()) + 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()) + return "", fmt.Errorf("git config [core.sparsecheckout -> true]: %v\n%s\n%s", err, outbuf.String(), errbuf.String()) } outbuf.Reset() errbuf.Reset() @@ -200,7 +207,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // Read base branch index 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()) + 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() @@ -235,12 +242,12 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge 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 + return "", err } if err := commitAndSignNoAuthor(pr, message, signArg, tmpBasePath, env); err != nil { log.Error("Unable to make final commit: %v", err) - return err + return "", err } case models.MergeStyleRebase: fallthrough @@ -248,7 +255,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // Checkout head branch 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()) + 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() @@ -262,10 +269,10 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge 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()) + 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{ + return "", models.ErrRebaseConflicts{ Style: mergeStyle, CommitSHA: strings.TrimSpace(string(commitShaBytes)), StdOut: outbuf.String(), @@ -274,7 +281,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge } } 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()) + 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() @@ -282,7 +289,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // Checkout base branch again 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()) + 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() @@ -298,12 +305,12 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // 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 + 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 + return "", err } } case models.MergeStyleSquash: @@ -311,35 +318,39 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge 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 + 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, &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 "", 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, &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 "", 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} + return "", models.ErrInvalidMergeStyle{ID: pr.BaseRepo.ID, Style: mergeStyle} } // OK we should cache our current head and origin/headbranch mergeHeadSHA, err := git.GetFullCommitID(tmpBasePath, "HEAD") if err != nil { - return fmt.Errorf("Failed to get full commit id for HEAD: %v", err) + return "", fmt.Errorf("Failed to get full commit id for HEAD: %v", err) } mergeBaseSHA, err := git.GetFullCommitID(tmpBasePath, "original_"+baseBranch) if err != nil { - return fmt.Errorf("Failed to get full commit id for origin/%s: %v", pr.BaseBranch, err) + return "", fmt.Errorf("Failed to get full commit id for origin/%s: %v", pr.BaseBranch, err) + } + mergeCommitID, err := git.GetFullCommitID(tmpBasePath, baseBranch) + if err != nil { + return "", fmt.Errorf("Failed to get full commit id for the new merge: %v", err) } // Now it's questionable about where this should go - either after or before the push @@ -347,7 +358,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // the merge as you can always remerge. if setting.LFS.StartServer { if err := LFSPush(tmpBasePath, mergeHeadSHA, mergeBaseSHA, pr); err != nil { - return err + return "", err } } @@ -356,7 +367,7 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge if err != nil { if !models.IsErrUserNotExist(err) { log.Error("Can't find user: %d for head repository - %v", pr.HeadRepo.OwnerID, err) - return err + return "", err } log.Error("Can't find user: %d for head repository - defaulting to doer: %s - %v", pr.HeadRepo.OwnerID, doer.Name, err) headUser = doer @@ -375,19 +386,19 @@ func rawMerge(pr *models.PullRequest, doer *models.User, mergeStyle models.Merge // Push back to upstream. 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{ + return "", models.ErrMergePushOutOfDate{ Style: mergeStyle, StdOut: outbuf.String(), StdErr: errbuf.String(), Err: err, } } - return fmt.Errorf("git push: %s", errbuf.String()) + return "", fmt.Errorf("git push: %s", errbuf.String()) } outbuf.Reset() errbuf.Reset() - return nil + return mergeCommitID, nil } func commitAndSignNoAuthor(pr *models.PullRequest, message, signArg, tmpBasePath string, env []string) error { diff --git a/services/pull/update.go b/services/pull/update.go index 5f055827e..66921abef 100644 --- a/services/pull/update.go +++ b/services/pull/update.go @@ -39,11 +39,13 @@ func Update(pull *models.PullRequest, doer *models.User, message string) error { return fmt.Errorf("HeadBranch of PR %d is up to date", pull.Index) } + _, err = rawMerge(pr, doer, models.MergeStyleMerge, message) + defer func() { go AddTestPullRequestTask(doer, pr.HeadRepo.ID, pr.HeadBranch, false, "", "") }() - return rawMerge(pr, doer, models.MergeStyleMerge, message) + return err } // IsUserAllowedToUpdate check if user is allowed to update PR with given permissions and branch protections