From d8905cb623cf55194eec5f1ed97fdc7bb30ff1cb Mon Sep 17 00:00:00 2001 From: 6543 <6543@obermui.de> Date: Mon, 2 May 2022 01:54:44 +0200 Subject: [PATCH] Dont overwrite err with nil & rename PullCheckingFuncs to reflect there usage (#19572) - dont overwrite err with nil unintentionaly - rename CheckPRReadyToMerge to CheckPullBranchProtections - rename prQueue to prPatchCheckerQueue from #9307 Co-authored-by: delvh --- routers/private/hook_pre_receive.go | 2 +- services/pull/check.go | 28 ++++++++++++++-------------- services/pull/check_test.go | 10 +++++----- services/pull/merge.go | 4 ++-- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index 8824d9cc3..731b36fc1 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -343,7 +343,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID, refFullN } // Check all status checks and reviews are ok - if err := pull_service.CheckPRReadyToMerge(ctx, pr, true); err != nil { + if err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil { if models.IsErrDisallowedToMerge(err) { log.Warn("Forbidden: User %d is not allowed push to protected branch %s in %-v and pr #%d is not ready to be merged: %s", ctx.opts.UserID, branchName, repo, pr.Index, err.Error()) ctx.JSON(http.StatusForbidden, private.Response{ diff --git a/services/pull/check.go b/services/pull/check.go index f7747dfa2..eb3b17d55 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -28,12 +28,12 @@ import ( asymkey_service "code.gitea.io/gitea/services/asymkey" ) -// prQueue represents a queue to handle update pull request tests -var prQueue queue.UniqueQueue +// prPatchCheckerQueue represents a queue to handle update pull request tests +var prPatchCheckerQueue queue.UniqueQueue var ( - ErrIsClosed = errors.New("pull is cosed") - ErrUserNotAllowedToMerge = errors.New("user not allowed to merge") + ErrIsClosed = errors.New("pull is closed") + ErrUserNotAllowedToMerge = models.ErrDisallowedToMerge{} ErrHasMerged = errors.New("has already been merged") ErrIsWorkInProgress = errors.New("work in progress PRs cannot be merged") ErrIsChecking = errors.New("cannot merge while conflict checking is in progress") @@ -43,7 +43,7 @@ var ( // AddToTaskQueue adds itself to pull request test task queue. func AddToTaskQueue(pr *models.PullRequest) { - err := prQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error { + err := prPatchCheckerQueue.PushFunc(strconv.FormatInt(pr.ID, 10), func() error { pr.Status = models.PullRequestStatusChecking err := pr.UpdateColsIfNotMerged("status") if err != nil { @@ -93,13 +93,13 @@ func CheckPullMergable(ctx context.Context, doer *user_model.User, perm *models. return ErrIsChecking } - if err := CheckPRReadyToMerge(ctx, pr, false); err != nil { + if err := CheckPullBranchProtections(ctx, pr, false); err != nil { if models.IsErrDisallowedToMerge(err) { if force { - if isRepoAdmin, err := models.IsUserRepoAdmin(pr.BaseRepo, doer); err != nil { - return err + if isRepoAdmin, err2 := models.IsUserRepoAdmin(pr.BaseRepo, doer); err2 != nil { + return err2 } else if !isRepoAdmin { - return ErrUserNotAllowedToMerge + return err } } } else { @@ -144,7 +144,7 @@ func checkAndUpdateStatus(pr *models.PullRequest) { } // Make sure there is no waiting test to process before leaving the checking status. - has, err := prQueue.Has(strconv.FormatInt(pr.ID, 10)) + has, err := prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10)) if err != nil { log.Error("Unable to check if the queue is waiting to reprocess pr.ID %d. Error: %v", pr.ID, err) } @@ -293,7 +293,7 @@ func InitializePullRequests(ctx context.Context) { case <-ctx.Done(): return default: - if err := prQueue.PushFunc(strconv.FormatInt(prID, 10), func() error { + if err := prPatchCheckerQueue.PushFunc(strconv.FormatInt(prID, 10), func() error { log.Trace("Adding PR ID: %d to the pull requests patch checking queue", prID) return nil }); err != nil { @@ -358,13 +358,13 @@ func CheckPrsForBaseBranch(baseRepo *repo_model.Repository, baseBranchName strin // Init runs the task queue to test all the checking status pull requests func Init() error { - prQueue = queue.CreateUniqueQueue("pr_patch_checker", handle, "") + prPatchCheckerQueue = queue.CreateUniqueQueue("pr_patch_checker", handle, "") - if prQueue == nil { + if prPatchCheckerQueue == nil { return fmt.Errorf("Unable to create pr_patch_checker Queue") } - go graceful.GetManager().RunWithShutdownFns(prQueue.Run) + go graceful.GetManager().RunWithShutdownFns(prPatchCheckerQueue.Run) go graceful.GetManager().RunWithShutdownContext(InitializePullRequests) return nil } diff --git a/services/pull/check_test.go b/services/pull/check_test.go index 65bcb9c0e..bc4c45ffa 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -41,7 +41,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { queueShutdown := []func(){} queueTerminate := []func(){} - prQueue = q.(queue.UniqueQueue) + prPatchCheckerQueue = q.(queue.UniqueQueue) pr := unittest.AssertExistsAndLoadBean(t, &models.PullRequest{ID: 2}).(*models.PullRequest) AddToTaskQueue(pr) @@ -51,11 +51,11 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { return pr.Status == models.PullRequestStatusChecking }, 1*time.Second, 100*time.Millisecond) - has, err := prQueue.Has(strconv.FormatInt(pr.ID, 10)) + has, err := prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10)) assert.True(t, has) assert.NoError(t, err) - prQueue.Run(func(shutdown func()) { + prPatchCheckerQueue.Run(func(shutdown func()) { queueShutdown = append(queueShutdown, shutdown) }, func(terminate func()) { queueTerminate = append(queueTerminate, terminate) @@ -68,7 +68,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { assert.Fail(t, "Timeout: nothing was added to pullRequestQueue") } - has, err = prQueue.Has(strconv.FormatInt(pr.ID, 10)) + has, err = prPatchCheckerQueue.Has(strconv.FormatInt(pr.ID, 10)) assert.False(t, has) assert.NoError(t, err) @@ -82,5 +82,5 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { callback() } - prQueue = nil + prPatchCheckerQueue = nil } diff --git a/services/pull/merge.go b/services/pull/merge.go index 7967bce6a..330dffd5c 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -662,8 +662,8 @@ func IsUserAllowedToMerge(pr *models.PullRequest, p models.Permission, user *use return false, nil } -// CheckPRReadyToMerge checks whether the PR is ready to be merged (reviews and status checks) -func CheckPRReadyToMerge(ctx context.Context, pr *models.PullRequest, skipProtectedFilesCheck bool) (err error) { +// CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks) +func CheckPullBranchProtections(ctx context.Context, pr *models.PullRequest, skipProtectedFilesCheck bool) (err error) { if err = pr.LoadBaseRepoCtx(ctx); err != nil { return fmt.Errorf("LoadBaseRepo: %v", err) }