From 82e0383d2104f454af5b3e0e768f0497113f3b13 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Sat, 7 Dec 2019 10:44:10 +0800 Subject: [PATCH] Move some pull request functions from models to services (#9266) * Move some pull request functions from models to services * Fix test --- models/pull.go | 131 +--------------------- models/pull_list.go | 71 ++---------- models/pull_test.go | 20 ---- routers/init.go | 3 +- routers/repo/issue.go | 3 +- services/pull/check.go | 209 ++++++++++++++++++++++++++++++++++++ services/pull/check_test.go | 34 ++++++ services/pull/main_test.go | 17 +++ services/pull/pull.go | 4 +- 9 files changed, 280 insertions(+), 212 deletions(-) create mode 100644 services/pull/check.go create mode 100644 services/pull/check_test.go create mode 100644 services/pull/main_test.go diff --git a/models/pull.go b/models/pull.go index 388ee16b46..2bd79202f0 100644 --- a/models/pull.go +++ b/models/pull.go @@ -8,7 +8,6 @@ package models import ( "bufio" "fmt" - "io/ioutil" "os" "path" "path/filepath" @@ -20,14 +19,11 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" - "code.gitea.io/gitea/modules/sync" "code.gitea.io/gitea/modules/timeutil" "github.com/unknwon/com" ) -var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLength) - // PullRequestType defines pull request type type PullRequestType int @@ -485,102 +481,6 @@ func (pr *PullRequest) SetMerged() (err error) { return nil } -// manuallyMerged checks if a pull request got manually merged -// When a pull request got manually merged mark the pull request as merged -func (pr *PullRequest) manuallyMerged() bool { - commit, err := pr.getMergeCommit() - if err != nil { - log.Error("PullRequest[%d].getMergeCommit: %v", pr.ID, err) - return false - } - if commit != nil { - pr.MergedCommitID = commit.ID.String() - pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) - pr.Status = PullRequestStatusManuallyMerged - merger, _ := GetUserByEmail(commit.Author.Email) - - // When the commit author is unknown set the BaseRepo owner as merger - if merger == nil { - if pr.BaseRepo.Owner == nil { - if err = pr.BaseRepo.getOwner(x); err != nil { - log.Error("BaseRepo.getOwner[%d]: %v", pr.ID, err) - return false - } - } - merger = pr.BaseRepo.Owner - } - pr.Merger = merger - pr.MergerID = merger.ID - - if err = pr.SetMerged(); err != nil { - log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) - return false - } - log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) - return true - } - return false -} - -// getMergeCommit checks if a pull request got merged -// Returns the git.Commit of the pull request if merged -func (pr *PullRequest) getMergeCommit() (*git.Commit, error) { - if pr.BaseRepo == nil { - var err error - pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID) - if err != nil { - return nil, fmt.Errorf("GetRepositoryByID: %v", err) - } - } - - indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())) - defer os.Remove(indexTmpPath) - - headFile := pr.GetGitRefName() - - // Check if a pull request is merged into BaseBranch - _, err := git.NewCommand("merge-base", "--is-ancestor", headFile, pr.BaseBranch).RunInDirWithEnv(pr.BaseRepo.RepoPath(), []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) - if err != nil { - // Errors are signaled by a non-zero status that is not 1 - if strings.Contains(err.Error(), "exit status 1") { - return nil, nil - } - return nil, fmt.Errorf("git merge-base --is-ancestor: %v", err) - } - - commitIDBytes, err := ioutil.ReadFile(pr.BaseRepo.RepoPath() + "/" + headFile) - if err != nil { - return nil, fmt.Errorf("ReadFile(%s): %v", headFile, err) - } - commitID := string(commitIDBytes) - if len(commitID) < 40 { - return nil, fmt.Errorf(`ReadFile(%s): invalid commit-ID "%s"`, headFile, commitID) - } - cmd := commitID[:40] + ".." + pr.BaseBranch - - // Get the commit from BaseBranch where the pull request got merged - mergeCommit, err := git.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse", cmd).RunInDirWithEnv("", []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) - if err != nil { - return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v", err) - } else if len(mergeCommit) < 40 { - // PR was fast-forwarded, so just use last commit of PR - mergeCommit = commitID[:40] - } - - gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) - if err != nil { - return nil, fmt.Errorf("OpenRepository: %v", err) - } - defer gitRepo.Close() - - commit, err := gitRepo.GetCommit(mergeCommit[:40]) - if err != nil { - return nil, fmt.Errorf("GetCommit: %v", err) - } - - return commit, nil -} - // patchConflicts is a list of conflict description from Git. var patchConflicts = []string{ "patch does not apply", @@ -589,6 +489,11 @@ var patchConflicts = []string{ "error:", } +// TestPatch checks if patch can be merged to base repository without conflict. +func (pr *PullRequest) TestPatch() error { + return pr.testPatch(x) +} + // testPatch checks if patch can be merged to base repository without conflict. func (pr *PullRequest) testPatch(e Engine) (err error) { if pr.BaseRepo == nil { @@ -949,32 +854,6 @@ func (pr *PullRequest) PushToBaseRepo() (err error) { return nil } -// AddToTaskQueue adds itself to pull request test task queue. -func (pr *PullRequest) AddToTaskQueue() { - go pullRequestQueue.AddFunc(pr.ID, func() { - pr.Status = PullRequestStatusChecking - if err := pr.UpdateCols("status"); err != nil { - log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) - } - }) -} - -// checkAndUpdateStatus checks if pull request is possible to leaving checking status, -// and set to be either conflict or mergeable. -func (pr *PullRequest) checkAndUpdateStatus() { - // Status is not changed to conflict means mergeable. - if pr.Status == PullRequestStatusChecking { - pr.Status = PullRequestStatusMergeable - } - - // Make sure there is no waiting test to process before leaving the checking status. - if !pullRequestQueue.Exist(pr.ID) { - if err := pr.UpdateCols("status, conflicted_files"); err != nil { - log.Error("Update[%d]: %v", pr.ID, 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/models/pull_list.go b/models/pull_list.go index 2c2f53f4a1..49d04ba0b8 100644 --- a/models/pull_list.go +++ b/models/pull_list.go @@ -10,7 +10,7 @@ import ( "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/log" - "github.com/unknwon/com" + "xorm.io/xorm" ) @@ -68,6 +68,14 @@ func GetUnmergedPullRequestsByBaseInfo(repoID int64, branch string) ([]*PullRequ Find(&prs) } +// GetPullRequestsByCheckStatus returns all pull requests according the special checking status. +func GetPullRequestsByCheckStatus(status PullRequestStatus) ([]*PullRequest, error) { + prs := make([]*PullRequest, 0, 10) + return prs, x. + Where("status=?", status). + Find(&prs) +} + // PullRequests returns all pull requests for a base Repo by the given conditions func PullRequests(baseRepoID int64, opts *PullRequestsOptions) ([]*PullRequest, int64, error) { if opts.Page <= 0 { @@ -161,64 +169,3 @@ func (prs PullRequestList) invalidateCodeComments(e Engine, doer *User, repo *gi func (prs PullRequestList) InvalidateCodeComments(doer *User, repo *git.Repository, branch string) error { return prs.invalidateCodeComments(x, doer, repo, branch) } - -// TestPullRequests checks and tests untested patches of pull requests. -// TODO: test more pull requests at same time. -func TestPullRequests() { - prs := make([]*PullRequest, 0, 10) - - err := x.Where("status = ?", PullRequestStatusChecking).Find(&prs) - if err != nil { - log.Error("Find Checking PRs: %v", err) - return - } - - var checkedPRs = make(map[int64]struct{}) - - // Update pull request status. - for _, pr := range prs { - checkedPRs[pr.ID] = struct{}{} - if err := pr.GetBaseRepo(); err != nil { - log.Error("GetBaseRepo: %v", err) - continue - } - if pr.manuallyMerged() { - continue - } - if err := pr.testPatch(x); err != nil { - log.Error("testPatch: %v", err) - continue - } - - pr.checkAndUpdateStatus() - } - - // Start listening on new test requests. - for prID := range pullRequestQueue.Queue() { - log.Trace("TestPullRequests[%v]: processing test task", prID) - pullRequestQueue.Remove(prID) - - id := com.StrTo(prID).MustInt64() - if _, ok := checkedPRs[id]; ok { - continue - } - - pr, err := GetPullRequestByID(id) - if err != nil { - log.Error("GetPullRequestByID[%s]: %v", prID, err) - continue - } else if pr.manuallyMerged() { - continue - } else if err = pr.testPatch(x); err != nil { - log.Error("testPatch[%d]: %v", pr.ID, err) - continue - } - - pr.checkAndUpdateStatus() - } -} - -// InitTestPullRequests runs the task to test all the checking status pull requests -func InitTestPullRequests() { - go TestPullRequests() -} diff --git a/models/pull_test.go b/models/pull_test.go index 8e2436b1a2..4971ff2e52 100644 --- a/models/pull_test.go +++ b/models/pull_test.go @@ -5,9 +5,7 @@ package models import ( - "strconv" "testing" - "time" "github.com/stretchr/testify/assert" ) @@ -196,24 +194,6 @@ func TestPullRequest_UpdateCols(t *testing.T) { // TODO TestPullRequest_PushToBaseRepo -func TestPullRequest_AddToTaskQueue(t *testing.T) { - assert.NoError(t, PrepareTestDatabase()) - - pr := AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) - pr.AddToTaskQueue() - - select { - case id := <-pullRequestQueue.Queue(): - assert.EqualValues(t, strconv.FormatInt(pr.ID, 10), id) - case <-time.After(time.Second): - assert.Fail(t, "Timeout: nothing was added to pullRequestQueue") - } - - assert.True(t, pullRequestQueue.Exist(pr.ID)) - pr = AssertExistsAndLoadBean(t, &PullRequest{ID: 1}).(*PullRequest) - assert.Equal(t, PullRequestStatusChecking, pr.Status) -} - func TestPullRequestList_LoadAttributes(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) diff --git a/routers/init.go b/routers/init.go index e161143f47..447c168852 100644 --- a/routers/init.go +++ b/routers/init.go @@ -26,6 +26,7 @@ import ( "code.gitea.io/gitea/modules/webhook" "code.gitea.io/gitea/services/mailer" mirror_service "code.gitea.io/gitea/services/mirror" + pull_service "code.gitea.io/gitea/services/pull" "gitea.com/macaron/macaron" ) @@ -104,7 +105,7 @@ func GlobalInit() { models.InitRepoIndexer() mirror_service.InitSyncMirrors() webhook.InitDeliverHooks() - models.InitTestPullRequests() + pull_service.Init() if err := task.Init(); err != nil { log.Fatal("Failed to initialize task scheduler: %v", err) } diff --git a/routers/repo/issue.go b/routers/repo/issue.go index cabb9e63c1..66e87e714f 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -28,6 +28,7 @@ import ( "code.gitea.io/gitea/modules/util" comment_service "code.gitea.io/gitea/services/comments" issue_service "code.gitea.io/gitea/services/issue" + pull_service "code.gitea.io/gitea/services/pull" "github.com/unknwon/com" ) @@ -1272,7 +1273,7 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { return } - issue.PullRequest.AddToTaskQueue() + pull_service.AddToTaskQueue(issue.PullRequest) } } diff --git a/services/pull/check.go b/services/pull/check.go new file mode 100644 index 0000000000..0fd3e2a76f --- /dev/null +++ b/services/pull/check.go @@ -0,0 +1,209 @@ +// Copyright 2019 The Gitea Authors. +// All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package pull + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strconv" + "strings" + "time" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/git" + "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/sync" + "code.gitea.io/gitea/modules/timeutil" + + "github.com/unknwon/com" +) + +// pullRequestQueue represents a queue to handle update pull request tests +var pullRequestQueue = sync.NewUniqueQueue(setting.Repository.PullRequestQueueLength) + +// AddToTaskQueue adds itself to pull request test task queue. +func AddToTaskQueue(pr *models.PullRequest) { + go pullRequestQueue.AddFunc(pr.ID, func() { + pr.Status = models.PullRequestStatusChecking + if err := pr.UpdateCols("status"); err != nil { + log.Error("AddToTaskQueue.UpdateCols[%d].(add to queue): %v", pr.ID, err) + } + }) +} + +// checkAndUpdateStatus checks if pull request is possible to leaving checking status, +// and set to be either conflict or mergeable. +func checkAndUpdateStatus(pr *models.PullRequest) { + // Status is not changed to conflict means mergeable. + if pr.Status == models.PullRequestStatusChecking { + pr.Status = models.PullRequestStatusMergeable + } + + // Make sure there is no waiting test to process before leaving the checking status. + if !pullRequestQueue.Exist(pr.ID) { + if err := pr.UpdateCols("status, conflicted_files"); err != nil { + log.Error("Update[%d]: %v", pr.ID, err) + } + } +} + +// getMergeCommit checks if a pull request got merged +// Returns the git.Commit of the pull request if merged +func getMergeCommit(pr *models.PullRequest) (*git.Commit, error) { + if pr.BaseRepo == nil { + var err error + pr.BaseRepo, err = models.GetRepositoryByID(pr.BaseRepoID) + if err != nil { + return nil, fmt.Errorf("GetRepositoryByID: %v", err) + } + } + + indexTmpPath := filepath.Join(os.TempDir(), "gitea-"+pr.BaseRepo.Name+"-"+strconv.Itoa(time.Now().Nanosecond())) + defer os.Remove(indexTmpPath) + + headFile := pr.GetGitRefName() + + // Check if a pull request is merged into BaseBranch + _, err := git.NewCommand("merge-base", "--is-ancestor", headFile, pr.BaseBranch).RunInDirWithEnv(pr.BaseRepo.RepoPath(), []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) + if err != nil { + // Errors are signaled by a non-zero status that is not 1 + if strings.Contains(err.Error(), "exit status 1") { + return nil, nil + } + return nil, fmt.Errorf("git merge-base --is-ancestor: %v", err) + } + + commitIDBytes, err := ioutil.ReadFile(pr.BaseRepo.RepoPath() + "/" + headFile) + if err != nil { + return nil, fmt.Errorf("ReadFile(%s): %v", headFile, err) + } + commitID := string(commitIDBytes) + if len(commitID) < 40 { + return nil, fmt.Errorf(`ReadFile(%s): invalid commit-ID "%s"`, headFile, commitID) + } + cmd := commitID[:40] + ".." + pr.BaseBranch + + // Get the commit from BaseBranch where the pull request got merged + mergeCommit, err := git.NewCommand("rev-list", "--ancestry-path", "--merges", "--reverse", cmd).RunInDirWithEnv("", []string{"GIT_INDEX_FILE=" + indexTmpPath, "GIT_DIR=" + pr.BaseRepo.RepoPath()}) + if err != nil { + return nil, fmt.Errorf("git rev-list --ancestry-path --merges --reverse: %v", err) + } else if len(mergeCommit) < 40 { + // PR was fast-forwarded, so just use last commit of PR + mergeCommit = commitID[:40] + } + + gitRepo, err := git.OpenRepository(pr.BaseRepo.RepoPath()) + if err != nil { + return nil, fmt.Errorf("OpenRepository: %v", err) + } + defer gitRepo.Close() + + commit, err := gitRepo.GetCommit(mergeCommit[:40]) + if err != nil { + return nil, fmt.Errorf("GetCommit: %v", err) + } + + return commit, nil +} + +// manuallyMerged checks if a pull request got manually merged +// When a pull request got manually merged mark the pull request as merged +func manuallyMerged(pr *models.PullRequest) bool { + commit, err := getMergeCommit(pr) + if err != nil { + log.Error("PullRequest[%d].getMergeCommit: %v", pr.ID, err) + return false + } + if commit != nil { + pr.MergedCommitID = commit.ID.String() + pr.MergedUnix = timeutil.TimeStamp(commit.Author.When.Unix()) + pr.Status = models.PullRequestStatusManuallyMerged + merger, _ := models.GetUserByEmail(commit.Author.Email) + + // When the commit author is unknown set the BaseRepo owner as merger + if merger == nil { + if pr.BaseRepo.Owner == nil { + if err = pr.BaseRepo.GetOwner(); err != nil { + log.Error("BaseRepo.GetOwner[%d]: %v", pr.ID, err) + return false + } + } + merger = pr.BaseRepo.Owner + } + pr.Merger = merger + pr.MergerID = merger.ID + + if err = pr.SetMerged(); err != nil { + log.Error("PullRequest[%d].setMerged : %v", pr.ID, err) + return false + } + log.Info("manuallyMerged[%d]: Marked as manually merged into %s/%s by commit id: %s", pr.ID, pr.BaseRepo.Name, pr.BaseBranch, commit.ID.String()) + return true + } + return false +} + +// TestPullRequests checks and tests untested patches of pull requests. +// TODO: test more pull requests at same time. +func TestPullRequests() { + prs, err := models.GetPullRequestsByCheckStatus(models.PullRequestStatusChecking) + if err != nil { + log.Error("Find Checking PRs: %v", err) + return + } + + var checkedPRs = make(map[int64]struct{}) + + // Update pull request status. + for _, pr := range prs { + checkedPRs[pr.ID] = struct{}{} + if err := pr.GetBaseRepo(); err != nil { + log.Error("GetBaseRepo: %v", err) + continue + } + if manuallyMerged(pr) { + continue + } + if err := pr.TestPatch(); err != nil { + log.Error("testPatch: %v", err) + continue + } + + checkAndUpdateStatus(pr) + } + + // Start listening on new test requests. + for prID := range pullRequestQueue.Queue() { + log.Trace("TestPullRequests[%v]: processing test task", prID) + pullRequestQueue.Remove(prID) + + id := com.StrTo(prID).MustInt64() + if _, ok := checkedPRs[id]; ok { + continue + } + + pr, err := models.GetPullRequestByID(id) + if err != nil { + log.Error("GetPullRequestByID[%s]: %v", prID, err) + continue + } else if manuallyMerged(pr) { + continue + } else if err = pr.TestPatch(); err != nil { + log.Error("testPatch[%d]: %v", pr.ID, err) + continue + } + + checkAndUpdateStatus(pr) + } +} + +// Init runs the task queue to test all the checking status pull requests +func Init() { + go TestPullRequests() +} diff --git a/services/pull/check_test.go b/services/pull/check_test.go new file mode 100644 index 0000000000..48a7774a61 --- /dev/null +++ b/services/pull/check_test.go @@ -0,0 +1,34 @@ +// Copyright 2019 The Gitea Authors. +// All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package pull + +import ( + "strconv" + "testing" + "time" + + "code.gitea.io/gitea/models" + + "github.com/stretchr/testify/assert" +) + +func TestPullRequest_AddToTaskQueue(t *testing.T) { + assert.NoError(t, models.PrepareTestDatabase()) + + pr := models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + AddToTaskQueue(pr) + + select { + case id := <-pullRequestQueue.Queue(): + assert.EqualValues(t, strconv.FormatInt(pr.ID, 10), id) + case <-time.After(time.Second): + assert.Fail(t, "Timeout: nothing was added to pullRequestQueue") + } + + assert.True(t, pullRequestQueue.Exist(pr.ID)) + pr = models.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 1}).(*models.PullRequest) + assert.Equal(t, models.PullRequestStatusChecking, pr.Status) +} diff --git a/services/pull/main_test.go b/services/pull/main_test.go new file mode 100644 index 0000000000..6c49e8fbf2 --- /dev/null +++ b/services/pull/main_test.go @@ -0,0 +1,17 @@ +// Copyright 2019 The Gitea Authors. +// All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package pull + +import ( + "path/filepath" + "testing" + + "code.gitea.io/gitea/models" +) + +func TestMain(m *testing.M) { + models.MainTest(m, filepath.Join("..", "..")) +} diff --git a/services/pull/pull.go b/services/pull/pull.go index 7a9c2ef9ad..2650dacc11 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -64,7 +64,7 @@ func addHeadRepoTasks(prs []*models.PullRequest) { continue } - pr.AddToTaskQueue() + AddToTaskQueue(pr) } } @@ -103,6 +103,6 @@ func AddTestPullRequestTask(doer *models.User, repoID int64, branch string, isSy return } for _, pr := range prs { - pr.AddToTaskQueue() + AddToTaskQueue(pr) } }