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 <lauris@nix.lv>
Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>

4 files changed, 61 insertions, 46 deletions

diff --git a/services/pull/check.go b/services/pull/check.go
index d64f49de3b..17f9e047a5 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 4591edd7aa..1bddc0b592 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 26c9ab3d1c..8f3e183551 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 -> <false> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String())
-		return fmt.Errorf("git config [filter.lfs.required -> <false> ]: %v\n%s\n%s", err, outbuf.String(), errbuf.String())
+		return "", fmt.Errorf("git config [filter.lfs.required -> <false> ]: %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 5f055827e1..66921abefa 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