Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Only check for conflicts/merging if the PR has not been merged in the interim (#10132) #10206

Merged
merged 2 commits into from Feb 10, 2020
Merged

Only check for conflicts/merging if the PR has not been merged in the interim (#10132) #10206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d907f51
Only check for conflicts/merging if the PR has not been merged in the…
zeripath Feb 9, 2020
68af585
re order
6543 Feb 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 1 addition & 9 deletions integrations/pull_merge_test.go
View file
Open in desktop
Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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")

Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand Down
64 changes: 46 additions & 18 deletions models/pull.go
View file
Open in desktop
Expand Up @@ -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.
Expand Down Expand Up @@ -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 {
Expand Down
8 changes: 5 additions & 3 deletions services/pull/check.go
View file
Open in desktop
Expand Up @@ -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)
}
})
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions services/pull/check_test.go
View file
Open in desktop
Expand Up @@ -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 {
Expand All @@ -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)
}
17 changes: 14 additions & 3 deletions services/pull/merge.go
View file
Open in desktop
Expand Up @@ -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
Expand Down