From c95d9603ea45ef985090014bde807fb47e3df931 Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 10 Feb 2020 14:09:08 +0100 Subject: [PATCH] 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 Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> * re order Co-authored-by: zeripath Co-authored-by: Lauris BH Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com> --- integrations/pull_merge_test.go | 10 +----- models/pull.go | 64 +++++++++++++++++++++++---------- services/pull/check.go | 8 +++-- services/pull/check_test.go | 4 +-- services/pull/merge.go | 17 +++++++-- 5 files changed, 68 insertions(+), 35 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 9df5b90fc..d165fde5f 100644 --- a/models/pull.go +++ b/models/pull.go @@ -649,44 +649,66 @@ func (pr *PullRequest) CheckUserAllowedToMerge(doer *User) (err error) { } // 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. @@ -845,6 +867,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 055403d0d..56561b7bb 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -31,7 +31,7 @@ var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLe func AddToTaskQueue(pr *models.PullRequest) { go pullRequestQueue.AddFunc(pr.ID, func() { pr.Status = models.PullRequestStatusChecking - if err := pr.UpdateCols("status"); err != nil { + if err := pr.UpdateColsIfNotMerged("status"); err != nil { log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) } }) @@ -142,9 +142,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 } baseGitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) @@ -194,7 +196,7 @@ func TestPullRequests(ctx context.Context) { 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 48a7774a6..043090249 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -18,7 +18,7 @@ import ( func TestPullRequest_AddToTaskQueue(t *testing.T) { assert.NoError(t, models.PrepareTestDatabase()) - pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest) AddToTaskQueue(pr) select { @@ -29,6 +29,6 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { } assert.True(t, pullRequestQueue.Exist(pr.ID)) - 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) } diff --git a/services/pull/merge.go b/services/pull/merge.go index 65a781e54..3958216b5 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -341,19 +341,30 @@ func Merge(pr *models.PullRequest, doer *models.User, baseGitRepo *git.Repositor outbuf.Reset() errbuf.Reset() - pr.MergedCommitID, err = baseGitRepo.GetBranchCommitID(pr.BaseBranch) + pr.MergedCommitID, err = git.GetFullCommitID(tmpBasePath, baseBranch) if err != nil { - return fmt.Errorf("GetBranchCommit: %v", err) + return fmt.Errorf("Failed to get full commit id for the new merge: %v", 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, baseGitRepo) // Reset cached commit count