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>

1 files changed, 46 insertions, 18 deletions

diff --git a/models/pull.go b/models/pull.go
index 8289c53cfe..46c50986b9 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 := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil {
+		return false, err
 	}
-	if err = pr.Issue.Repo.getOwner(sess); err != nil {
-		return err
+
+	pr.Issue = nil
+	if err := pr.loadIssue(sess); err != nil {
+		return false, 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)
 	}
 
-	if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
-		return fmt.Errorf("Issue.changeStatus: %v", err)
+	if err := pr.Issue.loadRepo(sess); err != nil {
+		return false, 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)
+
+	if err := pr.Issue.Repo.getOwner(sess); err != nil {
+		return false, err
 	}
 
-	if err = sess.Commit(); err != nil {
-		return fmt.Errorf("Commit: %v", err)
+	if _, err := pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
+		return false, fmt.Errorf("Issue.changeStatus: %v", err)
 	}
-	return nil
+
+	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 {