Skip to content

Commit c95d960

Browse files
6543zeripathlafriksguillep2k
authored
Only check for conflicts/merging if the PR has not been merged in the interim (#10132) (#10206)
* 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 * 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 move check pr after merge * Remove unnecessary prepareTestEnv - onGiteaRun does this for us * Add information about when merging occuring * More logging Co-authored-by: Lauris BH <[email protected]> Co-authored-by: guillep2k <[email protected]> * re order Co-authored-by: zeripath <[email protected]> Co-authored-by: Lauris BH <[email protected]> Co-authored-by: guillep2k <[email protected]>
1 parent 9169b39 commit c95d960

File tree

5 files changed

+68
-35
lines changed

5 files changed

+68
-35
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
@@ -649,44 +649,66 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) {
649649
}
650650

651651
// SetMerged sets a pull request to merged and closes the corresponding issue
652-
func (pr *PullRequest) SetMerged() (err error) {
652+
func (pr *PullRequest) SetMerged() (bool, error) {
653653
if pr.HasMerged {
654-
return fmt.Errorf("PullRequest[%d] already merged", pr.Index)
654+
return false, fmt.Errorf("PullRequest[%d] already merged", pr.Index)
655655
}
656656
if pr.MergedCommitID == "" || pr.MergedUnix == 0 || pr.Merger == nil {
657-
return fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index)
657+
return false, fmt.Errorf("Unable to merge PullRequest[%d], some required fields are empty", pr.Index)
658658
}
659659

660660
pr.HasMerged = true
661661

662662
sess := x.NewSession()
663663
defer sess.Close()
664-
if err = sess.Begin(); err != nil {
665-
return err
664+
if err := sess.Begin(); err != nil {
665+
return false, err
666666
}
667667

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

672-
if err = pr.Issue.loadRepo(sess); err != nil {
673-
return err
672+
if _, err := sess.Exec("UPDATE `pull_request` SET `issue_id` = `issue_id` WHERE `id` = ?", pr.ID); err != nil {
673+
return false, err
674674
}
675-
if err = pr.Issue.Repo.getOwner(sess); err != nil {
676-
return err
675+
676+
pr.Issue = nil
677+
if err := pr.loadIssue(sess); err != nil {
678+
return false, err
679+
}
680+
681+
if tmpPr, err := getPullRequestByID(sess, pr.ID); err != nil {
682+
return false, err
683+
} else if tmpPr.HasMerged {
684+
if pr.Issue.IsClosed {
685+
return false, nil
686+
}
687+
return false, fmt.Errorf("PullRequest[%d] already merged but it's associated issue [%d] is not closed", pr.Index, pr.IssueID)
688+
} else if pr.Issue.IsClosed {
689+
return false, fmt.Errorf("PullRequest[%d] already closed", pr.Index)
677690
}
678691

679-
if _, err = pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
680-
return fmt.Errorf("Issue.changeStatus: %v", err)
692+
if err := pr.Issue.loadRepo(sess); err != nil {
693+
return false, err
681694
}
682-
if _, err = sess.ID(pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
683-
return fmt.Errorf("update pull request: %v", err)
695+
696+
if err := pr.Issue.Repo.getOwner(sess); err != nil {
697+
return false, err
684698
}
685699

686-
if err = sess.Commit(); err != nil {
687-
return fmt.Errorf("Commit: %v", err)
700+
if _, err := pr.Issue.changeStatus(sess, pr.Merger, true); err != nil {
701+
return false, fmt.Errorf("Issue.changeStatus: %v", err)
688702
}
689-
return nil
703+
704+
if _, err := sess.Where("id = ?", pr.ID).Cols("has_merged, status, merged_commit_id, merger_id, merged_unix").Update(pr); err != nil {
705+
return false, fmt.Errorf("Failed to update pr[%d]: %v", pr.ID, err)
706+
}
707+
708+
if err := sess.Commit(); err != nil {
709+
return false, fmt.Errorf("Commit: %v", err)
710+
}
711+
return true, nil
690712
}
691713

692714
// NewPullRequest creates new pull request with labels for repository.
@@ -845,6 +867,12 @@ func (pr *PullRequest) UpdateCols(cols ...string) error {
845867
return err
846868
}
847869

870+
// UpdateColsIfNotMerged updates specific fields of a pull request if it has not been merged
871+
func (pr *PullRequest) UpdateColsIfNotMerged(cols ...string) error {
872+
_, err := x.Where("id = ? AND has_merged = ?", pr.ID, false).Cols(cols...).Update(pr)
873+
return err
874+
}
875+
848876
// IsWorkInProgress determine if the Pull Request is a Work In Progress by its title
849877
func (pr *PullRequest) IsWorkInProgress() bool {
850878
if err := pr.LoadIssue(); err != nil {

‎services/pull/check.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLe
3131
func AddToTaskQueue(pr *models.PullRequest) {
3232
go pullRequestQueue.AddFunc(pr.ID, func() {
3333
pr.Status = models.PullRequestStatusChecking
34-
if err := pr.UpdateCols("status"); err != nil {
34+
if err := pr.UpdateColsIfNotMerged("status"); err != nil {
3535
log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err)
3636
}
3737
})
@@ -142,9 +142,11 @@ func manuallyMerged(pr *models.PullRequest) bool {
142142
pr.Merger = merger
143143
pr.MergerID = merger.ID
144144

145-
if err = pr.SetMerged(); err != nil {
145+
if merged, err := pr.SetMerged(); err != nil {
146146
log.Error("PullRequest[%d].setMerged : %v", pr.ID, err)
147147
return false
148+
} else if !merged {
149+
return false
148150
}
149151

150152
baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath())
@@ -194,7 +196,7 @@ func TestPullRequests(ctx context.Context) {
194196
if err != nil {
195197
log.Error("GetPullRequestByID[%s]: %v", prID, err)
196198
continue
197-
} else if pr.Status != models.PullRequestStatusChecking {
199+
} else if pr.HasMerged {
198200
continue
199201
} else if manuallyMerged(pr) {
200202
continue

‎services/pull/check_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import (
1818
func TestPullRequest_AddToTaskQueue(t *testing.T) {
1919
assert.NoError(t, models.PrepareTestDatabase())
2020

21-
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
21+
pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest)
2222
AddToTaskQueue(pr)
2323

2424
select {
@@ -29,6 +29,6 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) {
2929
}
3030

3131
assert.True(t, pullRequestQueue.Exist(pr.ID))
32-
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest)
32+
pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest)
3333
assert.Equal(t, models.PullRequestStatusChecking, pr.Status)
3434
}

‎services/pull/merge.go

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -341,19 +341,30 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor
341341
outbuf.Reset()
342342
errbuf.Reset()
343343

344-
pr.MergedCommitID, err = baseGitRepo.GetBranchCommitID(pr.BaseBranch)
344+
pr.MergedCommitID, err = git.GetFullCommitID(tmpBasePath, baseBranch)
345345
if err != nil {
346-
return fmt.Errorf("GetBranchCommit: %v", err)
346+
return fmt.Errorf("Failed to get full commit id for the new merge: %v", err)
347347
}
348348

349349
pr.MergedUnix = timeutil.TimeStampNow()
350350
pr.Merger = doer
351351
pr.MergerID = doer.ID
352352

353-
if err = pr.SetMerged(); err != nil {
353+
if _, err = pr.SetMerged(); err != nil {
354354
log.Error("setMerged [%d]: %v", pr.ID, err)
355355
}
356356

357+
if err := pr.LoadIssue(); err != nil {
358+
log.Error("loadIssue [%d]: %v", pr.ID, err)
359+
}
360+
361+
if err := pr.Issue.LoadRepo(); err != nil {
362+
log.Error("loadRepo for issue [%d]: %v", pr.ID, err)
363+
}
364+
if err := pr.Issue.Repo.GetOwner(); err != nil {
365+
log.Error("GetOwner for issue repo [%d]: %v", pr.ID, err)
366+
}
367+
357368
notification.NotifyMergePullRequest(pr, doer, baseGitRepo)
358369

359370
// Reset cached commit count

0 commit comments

Comments
 (0)