Skip to content

Commit 875c5e1

Browse files
zeripathlafriksguillep2k
authored
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 <[email protected]> * Apply suggestions from code review Co-Authored-By: guillep2k <[email protected]> Co-authored-by: Lauris BH <[email protected]> Co-authored-by: guillep2k <[email protected]>
1 parent 585316f commit 875c5e1

File tree

6 files changed

+108
-73
lines changed

6 files changed

+108
-73
lines changed

‎integrations/pull_merge_test.go

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -105,8 +105,6 @@ func TestPullRebase(t *testing.T) {
105105

106106
func TestPullRebaseMerge(t *testing.T) {
107107
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
108-
defer prepareTestEnv(t)()
109-
110108
hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number
111109
assert.NoError(t, err)
112110
hookTasksLenBefore := len(hookTasks)
@@ -129,8 +127,6 @@ func TestPullRebaseMerge(t *testing.T) {
129127

130128
func TestPullSquash(t *testing.T) {
131129
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
132-
defer prepareTestEnv(t)()
133-
134130
hookTasks, err := models.HookTasks(1, 1) //Retrieve previous hook number
135131
assert.NoError(t, err)
136132
hookTasksLenBefore := len(hookTasks)
@@ -154,10 +150,9 @@ func TestPullSquash(t *testing.T) {
154150

155151
func TestPullCleanUpAfterMerge(t *testing.T) {
156152
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
157-
defer prepareTestEnv(t)()
158153
session := loginUser(t, "user1")
159154
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
160-
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited)\n")
155+
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "feature/test", "README.md", "Hello, World (Edited - TestPullCleanUpAfterMerge)\n")
161156

162157
resp := testPullCreate(t, session, "user1", "repo1", "feature/test", "This is a pull title")
163158

@@ -190,7 +185,6 @@ func TestPullCleanUpAfterMerge(t *testing.T) {
190185

191186
func TestCantMergeWorkInProgress(t *testing.T) {
192187
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
193-
defer prepareTestEnv(t)()
194188
session := loginUser(t, "user1")
195189
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
196190
testEditFile(t, session, "user1", "repo1", "master", "README.md", "Hello, World (Edited)\n")
@@ -212,7 +206,6 @@ func TestCantMergeWorkInProgress(t *testing.T) {
212206

213207
func TestCantMergeConflict(t *testing.T) {
214208
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
215-
defer prepareTestEnv(t)()
216209
session := loginUser(t, "user1")
217210
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
218211
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "conflict", "README.md", "Hello, World (Edited Once)\n")
@@ -258,7 +251,6 @@ func TestCantMergeConflict(t *testing.T) {
258251

259252
func TestCantMergeUnrelated(t *testing.T) {
260253
onGiteaRun(t, func(t *testing.T, giteaURL *url.URL) {
261-
defer prepareTestEnv(t)()
262254
session := loginUser(t, "user1")
263255
testRepoFork(t, session, "user2", "repo1", "user1", "repo1")
264256
testEditFileToNewBranch(t, session, "user1", "repo1", "master", "base", "README.md", "Hello, World (Edited Twice)\n")

‎models/pull.go

Lines changed: 46 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -498,44 +498,66 @@ const (
498498
)
499499

500500
// SetMerged sets a pull request to merged and closes the corresponding issue
501-
func (pr *PullRequest) SetMerged() (err error) {
501+
func (pr *PullRequest) SetMerged() (bool, error) {
502502
if pr.HasMerged {
503-
return fmt.Errorf("PullRequest[%d] already merged", pr.Index)
503+
return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index)
504504
}
505505
if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil {
506-
return fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index)
506+
return false, fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index)
507507
}
508508

509509
pr.HasMerged = true
510510

511511
sess := x.NewSession()
512512
defer sess.Close()
513-
if err = sess.Begin(); err != nil {
514-
return err
513+
if err := sess.Begin(); err != nil {
514+
return false, err
515515
}
516516

517-
if err = pr.loadIssue(sess); err != nil {
518-
return err
517+
if _, err := sess.Exec("UPDATE `issue` SET `repo_id` = `repo_id` WHERE `id` = ?", pr.IssueID); err != nil {
518+
return false, err
519519
}
520520

521-
if err = pr.Issue.loadRepo(sess); err != nil {
522-
return err
521+
if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil {
522+
return false, err
523523
}
524-
if err = pr.Issue.Repo.getOwner(sess); err != nil {
525-
return err
524+
525+
pr.Issue = nil
526+
if err := pr.loadIssue(sess); err != nil {
527+
return false, err
528+
}
529+
530+
if tmpPr, err := getPullRequestByID(sess, pr.ID); err != nil {
531+
return false, err
532+
} else if tmpPr.HasMerged {
533+
if pr.Issue.IsClosed {
534+
return false, nil
535+
}
536+
return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID)
537+
} else if pr.Issue.IsClosed {
538+
return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index)
526539
}
527540

528-
if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
529-
return fmt.Errorf("Issue.changeStatus: %v", err)
541+
if err := pr.Issue.loadRepo(sess); err != nil {
542+
return false, err
530543
}
531-
if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
532-
return fmt.Errorf("update pull request: %v", err)
544+
545+
if err := pr.Issue.Repo.getOwner(sess); err != nil {
546+
return false, err
533547
}
534548

535-
if err = sess.Commit(); err != nil {
536-
return fmt.Errorf("Commit: %v", err)
549+
if _, err := pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
550+
return false, fmt.Errorf("Issue.changeStatus: %v", err)
537551
}
538-
return nil
552+
553+
if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
554+
return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err)
555+
}
556+
557+
if err := sess.Commit(); err != nil {
558+
return false, fmt.Errorf("Commit: %v", err)
559+
}
560+
return true, nil
539561
}
540562

541563
// NewPullRequest creates new pull request with labels for repository.
@@ -707,6 +729,12 @@ func (pr *PullRequest) UpdateCols(cols ...string) error {
707729
return err
708730
}
709731

732+
// UpdateColsIfNotMerged updates specific fields of a pull request if it has not been merged
733+
func (pr *PullRequest) UpdateColsIfNotMerged(cols ...string) error {
734+
_, err := x.Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr)
735+
return err
736+
}
737+
710738
// IsWorkInProgress determine if the Pull Request is a Work In Progress by its title
711739
func (pr *PullRequest) IsWorkInProgress() bool {
712740
if err := pr.LoadIssue(); err != nil {

‎services/pull/check.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ func AddToTaskQueue(pr *models.PullRequest) {
3232
go func() {
3333
err := prQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error {
3434
pr.Status = models.PullRequestStatusChecking
35-
err := pr.UpdateCols("status")
35+
err := pr.UpdateColsIfNotMerged("status")
3636
if err != nil {
3737
log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err)
3838
} else {
@@ -158,9 +158,11 @@ func manuallyMerged(pr *models.PullRequest) bool {
158158
pr.Merger = merger
159159
pr.MergerID = merger.ID
160160

161-
if err = pr.SetMerged(); err != nil {
161+
if merged, err := pr.SetMerged(); err != nil {
162162
log.Error("PullRequest[%d].setMerged : %v", pr.ID, err)
163163
return false
164+
} else if !merged {
165+
return false
164166
}
165167

166168
notification.NotifyMergePullRequest(pr, merger)
@@ -205,7 +207,7 @@ func handle(data ...queue.Data) {
205207
if err != nil {
206208
log.Error("GetPullRequestByID[%s]: %v", prID, err)
207209
continue
208-
} else if pr.Status != models.PullRequestStatusChecking {
210+
} else if pr.HasMerged {
209211
continue
210212
} else if manuallyMerged(pr) {
211213
continue

‎services/pull/check_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -44,11 +44,11 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) {
4444

4545
prQueue = q.(queue.UniqueQueue)
4646

47-
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
47+
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest)
4848
AddToTaskQueue(pr)
4949

5050
assert.Eventually(t, func() bool {
51-
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
51+
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest)
5252
return pr.Status == models.PullRequestStatusChecking
5353
}, 1*time.Second, 100*time.Millisecond)
5454

@@ -73,7 +73,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) {
7373
assert.False(t, has)
7474
assert.NoError(t, err)
7575

76-
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
76+
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest)
7777
assert.Equal(t, models.PullRequestStatusChecking, pr.Status)
7878

7979
for _, callback := range queueShutdown {

0 commit comments

Comments
 (0)