From dbd6fe172a651416fd8e7b4a8617124f922f0b6d Mon Sep 17 00:00:00 2001 From: Brecht Van Lommel Date: Mon, 22 May 2023 00:22:01 +0200 Subject: [PATCH 01/12] Option to delay conflict checking of old pull requests until page view [repository.pull-request] CHECK_ONLY_LAST_UPDATED_DAYS specifies the number of days in which the PR must have been last updated for conflict checking to happen immediately when the base branch is updated. The default value of -1 behaves exactly as before with no delay. With 0 all conflict checking will be delayed until page view. We have found that 1 day works well in practice. This mostly eliminates the "Merge conflict checking is in progress. Try again in few moments." message when conflict checking is slow for big repositories and many pull requests. PRs that are actively being worked on will be checked immediately, while for others are likely to actually get checked in the next few seconds. Even better would be to auto-update the page as GitHub does, but this relatively simple change effectively solved the problem for us. --- custom/conf/app.example.ini | 5 +++ modules/setting/repository.go | 3 ++ routers/api/v1/repo/pull.go | 8 ++++ routers/web/repo/issue.go | 2 + services/pull/check.go | 81 +++++++++++++++++++++++++++++++---- services/pull/pull.go | 2 +- 6 files changed, 91 insertions(+), 10 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index a2dd92b1051ab..4c53c44031b3d 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1087,6 +1087,11 @@ LEVEL = Info ;; ;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. ;RETARGET_CHILDREN_ON_MERGE = true +;; +;; Delay conflict checking until page view or API access, for pull requests that have not been updated in the specified number of days. +;; The default `-1` means never delay. With `1` day, pull requests under active development will be checked quickly without undue server +;; load for old pull requests. +;CHECK_ONLY_LAST_UPDATED_DAYS = -1 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/modules/setting/repository.go b/modules/setting/repository.go index 8656ebc7ecfd0..ce9545b619d68 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -86,6 +86,7 @@ var ( AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool + CheckOnlyLastUpdatedDays int } `ini:"repository.pull-request"` // Issue Setting @@ -211,6 +212,7 @@ var ( AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool + CheckOnlyLastUpdatedDays int }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See @@ -226,6 +228,7 @@ var ( PopulateSquashCommentWithCommitMessages: false, AddCoCommitterTrailers: true, RetargetChildrenOnMerge: true, + CheckOnlyLastUpdatedDays: -1, }, // Issue settings diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 7eb4a8b8a2d4d..84cd6b0363f13 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -205,6 +205,10 @@ func GetPullRequest(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } + + // Consider API access a view for delayed checking. + pull_service.AddToTaskQueueOnView(ctx, pr) + ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer)) } @@ -290,6 +294,10 @@ func GetPullRequestByBaseHead(ctx *context.APIContext) { ctx.Error(http.StatusInternalServerError, "LoadHeadRepo", err) return } + + // Consider API access a view for delayed checking. + pull_service.AddToTaskQueueOnView(ctx, pr) + ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer)) } diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index 596abb4b9ca5b..dd71741d5e7ce 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -1824,6 +1824,8 @@ func ViewIssue(ctx *context.Context) { allowMerge := false canWriteToHeadRepo := false + pull_service.AddToTaskQueueOnView(ctx, pull) + if ctx.IsSigned { if err := pull.LoadHeadRepo(ctx); err != nil { log.Error("LoadHeadRepo: %v", err) diff --git a/services/pull/check.go b/services/pull/check.go index ce212f7d83bed..b38d1a28260d1 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -10,6 +10,7 @@ import ( "fmt" "strconv" "strings" + "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/models/db" @@ -26,6 +27,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/process" "code.gitea.io/gitea/modules/queue" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/timeutil" asymkey_service "code.gitea.io/gitea/services/asymkey" notify_service "code.gitea.io/gitea/services/notify" @@ -44,21 +46,85 @@ var ( ErrDependenciesLeft = errors.New("is blocked by an open dependency") ) -// AddToTaskQueue adds itself to pull request test task queue. -func AddToTaskQueue(ctx context.Context, pr *issues_model.PullRequest) { +// Change the pull request status to checking. +func setStatusChecking(ctx context.Context, pr *issues_model.PullRequest) bool { pr.Status = issues_model.PullRequestStatusChecking err := pr.UpdateColsIfNotMerged(ctx, "status") if err != nil { - log.Error("AddToTaskQueue(%-v).UpdateCols.(add to queue): %v", pr, err) - return + log.Error("setStatusChecking(%-v).UpdateCols.(add to queue): %v", pr, err) + return false } + return true +} + +// Add pull request in the actual queue. +func addToTaskQueue(pr *issues_model.PullRequest) { log.Trace("Adding %-v to the test pull requests queue", pr) - err = prPatchCheckerQueue.Push(strconv.FormatInt(pr.ID, 10)) + err := prPatchCheckerQueue.Push(strconv.FormatInt(pr.ID, 10)) if err != nil && err != queue.ErrAlreadyInQueue { log.Error("Error adding %-v to the test pull requests queue: %v", pr, err) } } +// Test if check should be delayed. +func shouldDelayCheck(ctx context.Context, pr *issues_model.PullRequest) bool { + if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 { + if err := pr.LoadIssue(ctx); err != nil { + return true + } + delay := time.Hour * 24 * time.Duration(setting.Repository.PullRequest.CheckOnlyLastUpdatedDays) + if pr.Issue.UpdatedUnix.AddDuration(delay) < timeutil.TimeStampNow() { + log.Trace("Delaying %-v patch checking because it was not updated recently", pr) + return true + } + } + return false +} + +// When base branch is updated. +func AddToTaskQueueOnBaseUpdate(ctx context.Context, pr *issues_model.PullRequest) { + if !setStatusChecking(ctx, pr) { + return + } + if shouldDelayCheck(ctx, pr) { + return + } + addToTaskQueue(pr) +} + +// When pull request is viewed by a user or through the API. +func AddToTaskQueueOnView(ctx context.Context, pr *issues_model.PullRequest) { + if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 && + pr.Status == issues_model.PullRequestStatusChecking { + addToTaskQueue(pr) + } +} + +// When Gitea restarts. +func AddToTaskQueueOnInit(ctx context.Context, prID int64) { + if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 { + pr, err := issues_model.GetPullRequestByID(ctx, prID) + if err != nil { + return + } + if shouldDelayCheck(ctx, pr) { + return + } + } + + log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID) + if err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)); err != nil { + log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err) + } +} + +// When pull request must be checked immediately without delay. +func AddToTaskQueue(ctx context.Context, pr *issues_model.PullRequest) { + if setStatusChecking(ctx, pr) { + addToTaskQueue(pr) + } +} + type MergeCheckType int const ( @@ -317,10 +383,7 @@ func InitializePullRequests(ctx context.Context) { case <-ctx.Done(): return default: - log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID) - if err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)); err != nil { - log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err) - } + AddToTaskQueueOnInit(ctx, prID) } } } diff --git a/services/pull/pull.go b/services/pull/pull.go index 154ff6c5c6851..6e1787a92eced 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -431,7 +431,7 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, log.Error("UpdateCommitDivergence: %v", err) } } - AddToTaskQueue(ctx, pr) + AddToTaskQueueOnBaseUpdate(ctx, pr) } }) } From 1c00cc3dfb142558da7f3003f2b53a9941186c94 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 24 Apr 2025 20:15:43 +0800 Subject: [PATCH 02/12] temp --- custom/conf/app.example.ini | 7 +- models/issues/pull.go | 22 --- modules/setting/repository.go | 6 +- routers/api/v1/repo/pull.go | 8 +- routers/private/hook_pre_receive.go | 3 +- routers/web/repo/issue_comment.go | 2 +- routers/web/repo/issue_view.go | 2 +- routers/web/repo/pull.go | 4 +- services/agit/agit.go | 2 +- services/automerge/automerge.go | 2 +- services/migrations/gitea_uploader.go | 2 +- services/pull/check.go | 146 +++++++++----------- services/pull/merge.go | 45 +----- services/pull/merge_prepare.go | 6 +- services/pull/patch.go | 13 +- services/pull/pull.go | 10 +- services/pull/temp_repo.go | 8 +- templates/repo/issue/view_content/pull.tmpl | 5 +- 18 files changed, 111 insertions(+), 182 deletions(-) diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index 4341e743f84a0..42d181a00af21 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -1156,10 +1156,9 @@ LEVEL = Info ;; Retarget child pull requests to the parent pull request branch target on merge of parent pull request. It only works on merged PRs where the head and base branch target the same repo. ;RETARGET_CHILDREN_ON_MERGE = true ;; -;; Delay conflict checking until page view or API access, for pull requests that have not been updated in the specified number of days. -;; The default `-1` means never delay. With `1` day, pull requests under active development will be checked quickly without undue server -;; load for old pull requests. -;CHECK_ONLY_LAST_UPDATED_DAYS = -1 +;; Delay mergeable check until page view or API access, for pull requests that have not been updated in the specified days when their base branches get updated. +;; Use "-1" to always check all pull requests (old behavior). Use "0" to always delay the checks. +;DELAY_CHECK_FOR_INACTIVE_DAYS = 7 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/models/issues/pull.go b/models/issues/pull.go index 016db9f75cb7e..e65b214dabfb3 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -10,7 +10,6 @@ import ( "fmt" "io" "regexp" - "strconv" "strings" "code.gitea.io/gitea/models/db" @@ -104,27 +103,6 @@ const ( PullRequestStatusAncestor ) -func (status PullRequestStatus) String() string { - switch status { - case PullRequestStatusConflict: - return "CONFLICT" - case PullRequestStatusChecking: - return "CHECKING" - case PullRequestStatusMergeable: - return "MERGEABLE" - case PullRequestStatusManuallyMerged: - return "MANUALLY_MERGED" - case PullRequestStatusError: - return "ERROR" - case PullRequestStatusEmpty: - return "EMPTY" - case PullRequestStatusAncestor: - return "ANCESTOR" - default: - return strconv.Itoa(int(status)) - } -} - // PullRequestFlow the flow of pull request type PullRequestFlow int diff --git a/modules/setting/repository.go b/modules/setting/repository.go index 62f5a8facff6e..c6bdc65b3218e 100644 --- a/modules/setting/repository.go +++ b/modules/setting/repository.go @@ -82,7 +82,7 @@ var ( AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool - CheckOnlyLastUpdatedDays int + DelayCheckForInactiveDays int } `ini:"repository.pull-request"` // Issue Setting @@ -201,7 +201,7 @@ var ( AddCoCommitterTrailers bool TestConflictingPatchesWithGitApply bool RetargetChildrenOnMerge bool - CheckOnlyLastUpdatedDays int + DelayCheckForInactiveDays int }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, // Same as GitHub. See @@ -217,7 +217,7 @@ var ( PopulateSquashCommentWithCommitMessages: false, AddCoCommitterTrailers: true, RetargetChildrenOnMerge: true, - CheckOnlyLastUpdatedDays: -1, + DelayCheckForInactiveDays: 7, }, // Issue settings diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index 06e68818f4928..04d9b1078729d 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -204,7 +204,7 @@ func GetPullRequest(ctx *context.APIContext) { } // Consider API access a view for delayed checking. - pull_service.AddToTaskQueueOnView(ctx, pr) + pull_service.StartPullRequestCheckOnView(ctx, pr) ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer)) } @@ -293,7 +293,7 @@ func GetPullRequestByBaseHead(ctx *context.APIContext) { } // Consider API access a view for delayed checking. - pull_service.AddToTaskQueueOnView(ctx, pr) + pull_service.StartPullRequestCheckOnView(ctx, pr) ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer)) } @@ -929,7 +929,7 @@ func MergePullRequest(ctx *context.APIContext) { if err := pull_service.CheckPullMergeable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil { if errors.Is(err, pull_service.ErrIsClosed) { ctx.APIErrorNotFound() - } else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { + } else if errors.Is(err, pull_service.ErrNoPermissionToMerge) { ctx.APIError(http.StatusMethodNotAllowed, "User not allowed to merge PR") } else if errors.Is(err, pull_service.ErrHasMerged) { ctx.APIError(http.StatusMethodNotAllowed, "") @@ -937,7 +937,7 @@ func MergePullRequest(ctx *context.APIContext) { ctx.APIError(http.StatusMethodNotAllowed, "Work in progress PRs cannot be merged") } else if errors.Is(err, pull_service.ErrNotMergeableState) { ctx.APIError(http.StatusMethodNotAllowed, "Please try again later") - } else if pull_service.IsErrDisallowedToMerge(err) { + } else if errors.Is(err, pull_service.ErrNotReadyToMerge) { ctx.APIError(http.StatusMethodNotAllowed, err) } else if asymkey_service.IsErrWontSign(err) { ctx.APIError(http.StatusMethodNotAllowed, err) diff --git a/routers/private/hook_pre_receive.go b/routers/private/hook_pre_receive.go index be9923c98f13d..dd9d0bc15e9b5 100644 --- a/routers/private/hook_pre_receive.go +++ b/routers/private/hook_pre_receive.go @@ -4,6 +4,7 @@ package private import ( + "errors" "fmt" "net/http" "os" @@ -374,7 +375,7 @@ func preReceiveBranch(ctx *preReceiveContext, oldCommitID, newCommitID string, r // Check all status checks and reviews are ok if err := pull_service.CheckPullBranchProtections(ctx, pr, true); err != nil { - if pull_service.IsErrDisallowedToMerge(err) { + if errors.Is(err, pull_service.ErrNotReadyToMerge) { 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{ UserMsg: fmt.Sprintf("Not allowed to push to protected branch %s and pr #%d is not ready to be merged: %s", branchName, ctx.opts.PullRequestID, err.Error()), diff --git a/routers/web/repo/issue_comment.go b/routers/web/repo/issue_comment.go index 45463200f6c29..8adce26ccc2aa 100644 --- a/routers/web/repo/issue_comment.go +++ b/routers/web/repo/issue_comment.go @@ -96,7 +96,7 @@ func NewComment(ctx *context.Context) { // Regenerate patch and test conflict. if pr == nil { issue.PullRequest.HeadCommitID = "" - pull_service.AddToTaskQueue(ctx, issue.PullRequest) + pull_service.StartPullRequestCheckImmediately(ctx, issue.PullRequest) } // check whether the ref of PR in base repo is consistent with the head commit of head branch in the head repo diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index 88236765c3b1a..e5811d1bb59e1 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -792,7 +792,7 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss allowMerge := false canWriteToHeadRepo := false - pull_service.AddToTaskQueueOnView(ctx, pull) + pull_service.StartPullRequestCheckOnView(ctx, pull) if ctx.IsSigned { if err := pull.LoadHeadRepo(ctx); err != nil { diff --git a/routers/web/repo/pull.go b/routers/web/repo/pull.go index a33542fd37e6f..15c9658fa82f0 100644 --- a/routers/web/repo/pull.go +++ b/routers/web/repo/pull.go @@ -1052,7 +1052,7 @@ func MergePullRequest(ctx *context.Context) { } else { ctx.JSONError(ctx.Tr("repo.issues.closed_title")) } - case errors.Is(err, pull_service.ErrUserNotAllowedToMerge): + case errors.Is(err, pull_service.ErrNoPermissionToMerge): ctx.JSONError(ctx.Tr("repo.pulls.update_not_allowed")) case errors.Is(err, pull_service.ErrHasMerged): ctx.JSONError(ctx.Tr("repo.pulls.has_merged")) @@ -1060,7 +1060,7 @@ func MergePullRequest(ctx *context.Context) { ctx.JSONError(ctx.Tr("repo.pulls.no_merge_wip")) case errors.Is(err, pull_service.ErrNotMergeableState): ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready")) - case pull_service.IsErrDisallowedToMerge(err): + case errors.Is(err, pull_service.ErrNotReadyToMerge): ctx.JSONError(ctx.Tr("repo.pulls.no_merge_not_ready")) case asymkey_service.IsErrWontSign(err): ctx.JSONError(err.Error()) // has no translation ... diff --git a/services/agit/agit.go b/services/agit/agit.go index 1e6ce933123f0..0fe28c5d66639 100644 --- a/services/agit/agit.go +++ b/services/agit/agit.go @@ -204,7 +204,7 @@ func ProcReceive(ctx context.Context, repo *repo_model.Repository, gitRepo *git. return nil, fmt.Errorf("failed to update pull ref. Error: %w", err) } - pull_service.AddToTaskQueue(ctx, pr) + pull_service.StartPullRequestCheckImmediately(ctx, pr) err = pr.LoadIssue(ctx) if err != nil { return nil, fmt.Errorf("failed to load pull issue. Error: %w", err) diff --git a/services/automerge/automerge.go b/services/automerge/automerge.go index 9d2f7f4857f55..0520a097d30ee 100644 --- a/services/automerge/automerge.go +++ b/services/automerge/automerge.go @@ -289,7 +289,7 @@ func handlePullRequestAutoMerge(pullID int64, sha string) { } if err := pull_service.CheckPullMergeable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false); err != nil { - if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) { + if errors.Is(err, pull_service.ErrNotReadyToMerge) { log.Info("%-v was scheduled to automerge by an unauthorized user", pr) return } diff --git a/services/migrations/gitea_uploader.go b/services/migrations/gitea_uploader.go index 82d756dc56fda..b6caa494c6520 100644 --- a/services/migrations/gitea_uploader.go +++ b/services/migrations/gitea_uploader.go @@ -556,7 +556,7 @@ func (g *GiteaLocalUploader) CreatePullRequests(ctx context.Context, prs ...*bas } for _, pr := range gprs { g.issues[pr.Issue.Index] = pr.Issue - pull.AddToTaskQueue(ctx, pr) + pull.StartPullRequestCheckImmediately(ctx, pr) } return nil } diff --git a/services/pull/check.go b/services/pull/check.go index 39e30119b8360..4f13cd2b0f8f4 100644 --- a/services/pull/check.go +++ b/services/pull/check.go @@ -36,91 +36,74 @@ import ( var prPatchCheckerQueue *queue.WorkerPoolQueue[string] var ( - ErrIsClosed = errors.New("pull is closed") - ErrUserNotAllowedToMerge = 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") - ErrNotMergeableState = errors.New("not in mergeable state") - ErrDependenciesLeft = errors.New("is blocked by an open dependency") + ErrIsClosed = errors.New("pull is closed") + ErrNoPermissionToMerge = errors.New("no permission to merge") + ErrNotReadyToMerge = errors.New("not ready to merge") + 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") + ErrNotMergeableState = errors.New("not in mergeable state") + ErrDependenciesLeft = errors.New("is blocked by an open dependency") ) -// Change the pull request status to checking. -func setStatusChecking(ctx context.Context, pr *issues_model.PullRequest) bool { +func markPullRequestStatusAsChecking(ctx context.Context, pr *issues_model.PullRequest) bool { pr.Status = issues_model.PullRequestStatusChecking err := pr.UpdateColsIfNotMerged(ctx, "status") if err != nil { - log.Error("setStatusChecking(%-v).UpdateCols.(add to queue): %v", pr, err) + log.Error("UpdateColsIfNotMerged failed, pr: %-v, err: %v", pr, err) return false } - return true -} - -// Add pull request in the actual queue. -func addToTaskQueue(pr *issues_model.PullRequest) { - log.Trace("Adding %-v to the test pull requests queue", pr) - err := prPatchCheckerQueue.Push(strconv.FormatInt(pr.ID, 10)) - if err != nil && err != queue.ErrAlreadyInQueue { - log.Error("Error adding %-v to the test pull requests queue: %v", pr, err) + pr, err = issues_model.GetPullRequestByID(ctx, pr.ID) + if err != nil { + log.Error("GetPullRequestByID failed, pr: %-v, err: %v", pr, err) + return false } + return pr.Status == issues_model.PullRequestStatusChecking } -// Test if check should be delayed. -func shouldDelayCheck(ctx context.Context, pr *issues_model.PullRequest) bool { - if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 { - if err := pr.LoadIssue(ctx); err != nil { - return true - } - delay := time.Hour * 24 * time.Duration(setting.Repository.PullRequest.CheckOnlyLastUpdatedDays) - if pr.Issue.UpdatedUnix.AddDuration(delay) < timeutil.TimeStampNow() { - log.Trace("Delaying %-v patch checking because it was not updated recently", pr) - return true - } +func addPullRequestToCheckQueue(prID int64) { + err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)) + if err != nil && !errors.Is(err, queue.ErrAlreadyInQueue) { + log.Error("Error adding %v to the pull requests check queue: %v", prID, err) } - return false } -// When base branch is updated. -func AddToTaskQueueOnBaseUpdate(ctx context.Context, pr *issues_model.PullRequest) { - if !setStatusChecking(ctx, pr) { - return - } - if shouldDelayCheck(ctx, pr) { +func StartPullRequestCheckImmediately(ctx context.Context, pr *issues_model.PullRequest) { + if !markPullRequestStatusAsChecking(ctx, pr) { return } - addToTaskQueue(pr) + addPullRequestToCheckQueue(pr.ID) } -// When pull request is viewed by a user or through the API. -func AddToTaskQueueOnView(ctx context.Context, pr *issues_model.PullRequest) { - if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 && - pr.Status == issues_model.PullRequestStatusChecking { - addToTaskQueue(pr) +// StartPullRequestCheckDelayable will delay the check if the pull request is not update recently. +// The case is when the "base" branch gets updated, all PRs targeting that "base" branch needs to re-check whether they are mergeable. +// When there are too many stale PRs, each "base" branch update will consume a lot of system resources. +// So we could delay the checks for PRs that are not updated recently, only mark their status as "checking", +// then next time when these PRs are update or viewed, the real checks will run. +func StartPullRequestCheckDelayable(ctx context.Context, pr *issues_model.PullRequest) { + if !markPullRequestStatusAsChecking(ctx, pr) { + return } -} -// When Gitea restarts. -func AddToTaskQueueOnInit(ctx context.Context, prID int64) { - if setting.Repository.PullRequest.CheckOnlyLastUpdatedDays >= 0 { - pr, err := issues_model.GetPullRequestByID(ctx, prID) - if err != nil { + if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 { + if err := pr.LoadIssue(ctx); err != nil { return } - if shouldDelayCheck(ctx, pr) { + duration := 24 * time.Hour * time.Duration(setting.Repository.PullRequest.DelayCheckForInactiveDays) + if pr.Issue.UpdatedUnix.AddDuration(duration) <= timeutil.TimeStampNow() { return } } - log.Trace("Adding PR[%d] to the pull requests patch checking queue", prID) - if err := prPatchCheckerQueue.Push(strconv.FormatInt(prID, 10)); err != nil { - log.Error("Error adding PR[%d] to the pull requests patch checking queue %v", prID, err) - } + addPullRequestToCheckQueue(pr.ID) } -// When pull request must be checked immediately without delay. -func AddToTaskQueue(ctx context.Context, pr *issues_model.PullRequest) { - if setStatusChecking(ctx, pr) { - addToTaskQueue(pr) +func StartPullRequestCheckOnView(_ context.Context, pr *issues_model.PullRequest) { + // TODO: its correctness totally depends on the "unique queue" feature. + // So duplicate "start" requests will be ignored if there is already a task in the queue + // Ideally in the future we should decouple the "unique queue" feature from the "start" request + if pr.Status == issues_model.PullRequestStatusChecking { + addPullRequestToCheckQueue(pr.ID) } } @@ -150,7 +133,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc log.Error("Error whilst checking if %-v is allowed to merge %-v: %v", doer, pr, err) return err } else if !allowedMerge { - return ErrUserNotAllowedToMerge + return ErrNoPermissionToMerge } if mergeCheckType == MergeCheckTypeManually { @@ -171,7 +154,7 @@ func CheckPullMergeable(stdCtx context.Context, doer *user_model.User, perm *acc } if err := CheckPullBranchProtections(ctx, pr, false); err != nil { - if !IsErrDisallowedToMerge(err) { + if !errors.Is(err, ErrNotReadyToMerge) { log.Error("Error whilst checking pull branch protection for %-v: %v", pr, err) return err } @@ -238,10 +221,10 @@ func isSignedIfRequired(ctx context.Context, pr *issues_model.PullRequest, doer return sign, err } -// checkAndUpdateStatus checks if pull request is possible to leaving checking status, +// markPullRequestAsMergeable checks if pull request is possible to leaving checking status, // and set to be either conflict or mergeable. -func checkAndUpdateStatus(ctx context.Context, pr *issues_model.PullRequest) { - // If status has not been changed to conflict by testPatch then we are mergeable +func markPullRequestAsMergeable(ctx context.Context, pr *issues_model.PullRequest) { + // If status has not been changed to conflict by testPullRequestTmpRepoBranchMergeable then we are mergeable if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -376,6 +359,10 @@ func manuallyMerged(ctx context.Context, pr *issues_model.PullRequest) bool { // InitializePullRequests checks and tests untested patches of pull requests. func InitializePullRequests(ctx context.Context) { + // if we prefer to delay the checks, then no need to do any check during startup, there should be no much difference + if setting.Repository.PullRequest.DelayCheckForInactiveDays >= 0 { + return + } prs, err := issues_model.GetPullRequestIDsByCheckStatus(ctx, issues_model.PullRequestStatusChecking) if err != nil { log.Error("Find Checking PRs: %v", err) @@ -386,21 +373,12 @@ func InitializePullRequests(ctx context.Context) { case <-ctx.Done(): return default: - AddToTaskQueueOnInit(ctx, prID) + addPullRequestToCheckQueue(prID) } } } -// handle passed PR IDs and test the PRs -func handler(items ...string) []string { - for _, s := range items { - id, _ := strconv.ParseInt(s, 10, 64) - testPR(id) - } - return nil -} - -func testPR(id int64) { +func checkPullRequestMergeable(id int64) { ctx := graceful.GetManager().HammerContext() releaser, err := globallock.Lock(ctx, getPullWorkingLockKey(id)) if err != nil { @@ -414,7 +392,7 @@ func testPR(id int64) { pr, err := issues_model.GetPullRequestByID(ctx, id) if err != nil { - log.Error("Unable to GetPullRequestByID[%d] for testPR: %v", id, err) + log.Error("Unable to GetPullRequestByID[%d] for checkPullRequestMergeable: %v", id, err) return } @@ -433,15 +411,15 @@ func testPR(id int64) { return } - if err := TestPatch(pr); err != nil { - log.Error("testPatch[%-v]: %v", pr, err) + if err := testPullRequestBranchMergeable(pr); err != nil { + log.Error("testPullRequestTmpRepoBranchMergeable[%-v]: %v", pr, err) pr.Status = issues_model.PullRequestStatusError if err := pr.UpdateCols(ctx, "status"); err != nil { log.Error("update pr [%-v] status to PullRequestStatusError failed: %v", pr, err) } return } - checkAndUpdateStatus(ctx, pr) + markPullRequestAsMergeable(ctx, pr) } // CheckPRsForBaseBranch check all pulls with baseBrannch @@ -450,17 +428,21 @@ func CheckPRsForBaseBranch(ctx context.Context, baseRepo *repo_model.Repository, if err != nil { return err } - for _, pr := range prs { - AddToTaskQueue(ctx, pr) + StartPullRequestCheckImmediately(ctx, pr) } - return nil } // Init runs the task queue to test all the checking status pull requests func Init() error { - prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", handler) + prPatchCheckerQueue = queue.CreateUniqueQueue(graceful.GetManager().ShutdownContext(), "pr_patch_checker", func(items ...string) []string { + for _, s := range items { + id, _ := strconv.ParseInt(s, 10, 64) + checkPullRequestMergeable(id) + } + return nil + }) if prPatchCheckerQueue == nil { return errors.New("unable to create pr_patch_checker queue") diff --git a/services/pull/merge.go b/services/pull/merge.go index 9804d8aac1ed6..256db847ef39b 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -518,25 +518,6 @@ func IsUserAllowedToMerge(ctx context.Context, pr *issues_model.PullRequest, p a return false, nil } -// ErrDisallowedToMerge represents an error that a branch is protected and the current user is not allowed to modify it. -type ErrDisallowedToMerge struct { - Reason string -} - -// IsErrDisallowedToMerge checks if an error is an ErrDisallowedToMerge. -func IsErrDisallowedToMerge(err error) bool { - _, ok := err.(ErrDisallowedToMerge) - return ok -} - -func (err ErrDisallowedToMerge) Error() string { - return fmt.Sprintf("not allowed to merge [reason: %s]", err.Reason) -} - -func (err ErrDisallowedToMerge) Unwrap() error { - return util.ErrPermissionDenied -} - // CheckPullBranchProtections checks whether the PR is ready to be merged (reviews and status checks) func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullRequest, skipProtectedFilesCheck bool) (err error) { if err = pr.LoadBaseRepo(ctx); err != nil { @@ -556,31 +537,21 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques return err } if !isPass { - return ErrDisallowedToMerge{ - Reason: "Not all required status checks successful", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Not all required status checks successful") } if !issues_model.HasEnoughApprovals(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "Does not have enough approvals", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Does not have enough approvals") } if issues_model.MergeBlockedByRejectedReview(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "There are requested changes", - } + return util.ErrorWrap(ErrNotReadyToMerge, "There are requested changes") } if issues_model.MergeBlockedByOfficialReviewRequests(ctx, pb, pr) { - return ErrDisallowedToMerge{ - Reason: "There are official review requests", - } + return util.ErrorWrap(ErrNotReadyToMerge, "There are official review requests") } if issues_model.MergeBlockedByOutdatedBranch(pb, pr) { - return ErrDisallowedToMerge{ - Reason: "The head branch is behind the base branch", - } + return util.ErrorWrap(ErrNotReadyToMerge, "The head branch is behind the base branch") } if skipProtectedFilesCheck { @@ -588,9 +559,7 @@ func CheckPullBranchProtections(ctx context.Context, pr *issues_model.PullReques } if pb.MergeBlockedByProtectedFiles(pr.ChangedProtectedFiles) { - return ErrDisallowedToMerge{ - Reason: "Changed protected files", - } + return util.ErrorWrap(ErrNotReadyToMerge, "Changed protected files") } return nil @@ -709,7 +678,7 @@ func SetMerged(ctx context.Context, pr *issues_model.PullRequest, mergedCommitID return false, fmt.Errorf("ChangeIssueStatus: %w", err) } - // We need to save all of the data used to compute this merge as it may have already been changed by TestPatch. FIXME: need to set some state to prevent TestPatch from running whilst we are merging. + // We need to save all of the data used to compute this merge as it may have already been changed by testPullRequestBranchMergeable. FIXME: need to set some state to prevent testPullRequestBranchMergeable from running whilst we are merging. if cnt, err := db.GetEngine(ctx).Where("id = ?", pr.ID). And("has_merged = ?", false). Cols("has_merged, status, merge_base, merged_commit_id, merger_id, merged_unix, conflicted_files"). diff --git a/services/pull/merge_prepare.go b/services/pull/merge_prepare.go index 593cba550a667..719cc6b965605 100644 --- a/services/pull/merge_prepare.go +++ b/services/pull/merge_prepare.go @@ -23,7 +23,7 @@ import ( ) type mergeContext struct { - *prContext + *prTmpRepoContext doer *user_model.User sig *git.Signature committer *git.Signature @@ -68,8 +68,8 @@ func createTemporaryRepoForMerge(ctx context.Context, pr *issues_model.PullReque } mergeCtx = &mergeContext{ - prContext: prCtx, - doer: doer, + prTmpRepoContext: prCtx, + doer: doer, } if expectedHeadCommitID != "" { diff --git a/services/pull/patch.go b/services/pull/patch.go index 7a242377249d3..153e0baf87ab4 100644 --- a/services/pull/patch.go +++ b/services/pull/patch.go @@ -67,9 +67,8 @@ var patchErrorSuffices = []string{ ": does not exist in index", } -// TestPatch will test whether a simple patch will apply -func TestPatch(pr *issues_model.PullRequest) error { - ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("TestPatch: %s", pr)) +func testPullRequestBranchMergeable(pr *issues_model.PullRequest) error { + ctx, _, finished := process.GetManager().AddContext(graceful.GetManager().HammerContext(), fmt.Sprintf("testPullRequestBranchMergeable: %s", pr)) defer finished() prCtx, cancel, err := createTemporaryRepoForPR(ctx, pr) @@ -81,10 +80,10 @@ func TestPatch(pr *issues_model.PullRequest) error { } defer cancel() - return testPatch(ctx, prCtx, pr) + return testPullRequestTmpRepoBranchMergeable(ctx, prCtx, pr) } -func testPatch(ctx context.Context, prCtx *prContext, pr *issues_model.PullRequest) error { +func testPullRequestTmpRepoBranchMergeable(ctx context.Context, prCtx *prTmpRepoContext, pr *issues_model.PullRequest) error { gitRepo, err := git.OpenRepository(ctx, prCtx.tmpBasePath) if err != nil { return fmt.Errorf("OpenRepository: %w", err) @@ -380,7 +379,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * return false, nil } - log.Trace("PullRequest[%d].testPatch (patchPath): %s", pr.ID, patchPath) + log.Trace("PullRequest[%d].testPullRequestTmpRepoBranchMergeable (patchPath): %s", pr.ID, patchPath) // 4. Read the base branch in to the index of the temporary repository _, _, err = git.NewCommand("read-tree", "base").RunStdString(gitRepo.Ctx, &git.RunOpts{Dir: tmpBasePath}) @@ -450,7 +449,7 @@ func checkConflicts(ctx context.Context, pr *issues_model.PullRequest, gitRepo * scanner := bufio.NewScanner(stderrReader) for scanner.Scan() { line := scanner.Text() - log.Trace("PullRequest[%d].testPatch: stderr: %s", pr.ID, line) + log.Trace("PullRequest[%d].testPullRequestTmpRepoBranchMergeable: stderr: %s", pr.ID, line) if strings.HasPrefix(line, prefix) { conflict = true filepath := strings.TrimSpace(strings.Split(line[len(prefix):], ":")[0]) diff --git a/services/pull/pull.go b/services/pull/pull.go index b5c1757b2b82a..e3053409b2530 100644 --- a/services/pull/pull.go +++ b/services/pull/pull.go @@ -96,7 +96,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error { } defer cancel() - if err := testPatch(ctx, prCtx, pr); err != nil { + if err := testPullRequestTmpRepoBranchMergeable(ctx, prCtx, pr); err != nil { return err } @@ -314,12 +314,12 @@ func ChangeTargetBranch(ctx context.Context, pr *issues_model.PullRequest, doer pr.BaseBranch = targetBranch // Refresh patch - if err := TestPatch(pr); err != nil { + if err := testPullRequestBranchMergeable(pr); err != nil { return err } // Update target branch, PR diff and status - // This is the same as checkAndUpdateStatus in check service, but also updates base_branch + // This is the same as markPullRequestAsMergeable in check service, but also updates base_branch if pr.Status == issues_model.PullRequestStatusChecking { pr.Status = issues_model.PullRequestStatusMergeable } @@ -409,7 +409,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { continue } - AddToTaskQueue(ctx, pr) + StartPullRequestCheckImmediately(ctx, pr) comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID) if err == nil && comment != nil { notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment) @@ -502,7 +502,7 @@ func AddTestPullRequestTask(opts TestPullRequestOptions) { log.Error("UpdateCommitDivergence: %v", err) } } - AddToTaskQueueOnBaseUpdate(ctx, pr) + StartPullRequestCheckDelayable(ctx, pr) } }) } diff --git a/services/pull/temp_repo.go b/services/pull/temp_repo.go index d543e3d4a3681..72406482e0821 100644 --- a/services/pull/temp_repo.go +++ b/services/pull/temp_repo.go @@ -28,7 +28,7 @@ const ( stagingBranch = "staging" // this is used for a working branch ) -type prContext struct { +type prTmpRepoContext struct { context.Context tmpBasePath string pr *issues_model.PullRequest @@ -36,7 +36,7 @@ type prContext struct { errbuf *strings.Builder // any use should be preceded by a Reset and preferably after use } -func (ctx *prContext) RunOpts() *git.RunOpts { +func (ctx *prTmpRepoContext) RunOpts() *git.RunOpts { ctx.outbuf.Reset() ctx.errbuf.Reset() return &git.RunOpts{ @@ -48,7 +48,7 @@ func (ctx *prContext) RunOpts() *git.RunOpts { // createTemporaryRepoForPR creates a temporary repo with "base" for pr.BaseBranch and "tracking" for pr.HeadBranch // it also create a second base branch called "original_base" -func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) (prCtx *prContext, cancel context.CancelFunc, err error) { +func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) (prCtx *prTmpRepoContext, cancel context.CancelFunc, err error) { if err := pr.LoadHeadRepo(ctx); err != nil { log.Error("%-v LoadHeadRepo: %v", pr, err) return nil, nil, fmt.Errorf("%v LoadHeadRepo: %w", pr, err) @@ -81,7 +81,7 @@ func createTemporaryRepoForPR(ctx context.Context, pr *issues_model.PullRequest) } cancel = cleanup - prCtx = &prContext{ + prCtx = &prTmpRepoContext{ Context: ctx, tmpBasePath: tmpBasePath, pr: pr, diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 064b62e1289e9..eb4aeb15636dd 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -191,10 +191,11 @@ {{end}} {{end}} + {{template "repo/issue/view_content/update_branch_by_merge" $}} + {{if .Issue.PullRequest.IsEmpty}}
-
{{svg "octicon-alert"}} {{ctx.Locale.Tr "repo.pulls.is_empty"}} @@ -318,7 +319,7 @@ {{if .IsBlockedByApprovals}}
{{svg "octicon-x"}} - {{ctx.Locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}} + {{ctx.Locale.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .ProtectedBranch.RequiredApprovals}}
{{else if .IsBlockedByRejection}}
From 7062121555a18823700c2fad7466a7f67273de87 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 24 Apr 2025 21:38:26 +0800 Subject: [PATCH 03/12] fix test and add ui --- routers/web/repo/issue.go | 3 +- routers/web/repo/issue_view.go | 67 ++++++--- routers/web/web.go | 1 + services/pull/check_test.go | 2 +- templates/repo/issue/view_content.tmpl | 2 +- .../{pull.tmpl => pull_merge_box.tmpl} | 12 +- .../view_content/update_branch_by_merge.tmpl | 8 +- web_src/css/repo.css | 8 -- web_src/js/features/repo-issue-pr-form.ts | 10 -- web_src/js/features/repo-issue-pr-status.ts | 10 -- web_src/js/features/repo-issue-pull.ts | 132 ++++++++++++++++++ web_src/js/features/repo-issue.ts | 48 ------- web_src/js/features/repo-legacy.ts | 8 +- 13 files changed, 201 insertions(+), 110 deletions(-) rename templates/repo/issue/view_content/{pull.tmpl => pull_merge_box.tmpl} (97%) delete mode 100644 web_src/js/features/repo-issue-pr-form.ts delete mode 100644 web_src/js/features/repo-issue-pr-status.ts create mode 100644 web_src/js/features/repo-issue-pull.ts diff --git a/routers/web/repo/issue.go b/routers/web/repo/issue.go index dbbe29a3c3f4f..86ee56b467506 100644 --- a/routers/web/repo/issue.go +++ b/routers/web/repo/issue.go @@ -43,7 +43,8 @@ const ( tplIssueChoose templates.TplName = "repo/issue/choose" tplIssueView templates.TplName = "repo/issue/view" - tplReactions templates.TplName = "repo/issue/view_content/reactions" + tplPullMergeBox templates.TplName = "repo/issue/view_content/pull_merge_box" + tplReactions templates.TplName = "repo/issue/view_content/reactions" issueTemplateKey = "IssueTemplate" issueTemplateTitleKey = "IssueTemplateTitle" diff --git a/routers/web/repo/issue_view.go b/routers/web/repo/issue_view.go index e5811d1bb59e1..84852cee13fe5 100644 --- a/routers/web/repo/issue_view.go +++ b/routers/web/repo/issue_view.go @@ -31,6 +31,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" "code.gitea.io/gitea/modules/templates/vars" + "code.gitea.io/gitea/modules/util" asymkey_service "code.gitea.io/gitea/services/asymkey" "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context/upload" @@ -271,8 +272,23 @@ func combineLabelComments(issue *issues_model.Issue) { } } -// ViewIssue render issue view page -func ViewIssue(ctx *context.Context) { +func prepareIssueViewLoad(ctx *context.Context) *issues_model.Issue { + issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index")) + if err != nil { + ctx.NotFoundOrServerError("GetIssueByIndex", issues_model.IsErrIssueNotExist, err) + return nil + } + issue.Repo = ctx.Repo.Repository + ctx.Data["Issue"] = issue + + if err = issue.LoadPullRequest(ctx); err != nil { + ctx.ServerError("LoadPullRequest", err) + return nil + } + return issue +} + +func handleViewIssueRedirectExternal(ctx *context.Context) { if ctx.PathParam("type") == "issues" { // If issue was requested we check if repo has external tracker and redirect extIssueUnit, err := ctx.Repo.Repository.GetUnit(ctx, unit.TypeExternalTracker) @@ -294,18 +310,18 @@ func ViewIssue(ctx *context.Context) { return } } +} - issue, err := issues_model.GetIssueByIndex(ctx, ctx.Repo.Repository.ID, ctx.PathParamInt64("index")) - if err != nil { - if issues_model.IsErrIssueNotExist(err) { - ctx.NotFound(err) - } else { - ctx.ServerError("GetIssueByIndex", err) - } +// ViewIssue render issue view page +func ViewIssue(ctx *context.Context) { + handleViewIssueRedirectExternal(ctx) + if ctx.Written() { return } - if issue.Repo == nil { - issue.Repo = ctx.Repo.Repository + + issue := prepareIssueViewLoad(ctx) + if ctx.Written() { + return } // Make sure type and URL matches. @@ -337,12 +353,12 @@ func ViewIssue(ctx *context.Context) { ctx.Data["IsAttachmentEnabled"] = setting.Attachment.Enabled upload.AddUploadContext(ctx, "comment") - if err = issue.LoadAttributes(ctx); err != nil { + if err := issue.LoadAttributes(ctx); err != nil { ctx.ServerError("LoadAttributes", err) return } - if err = filterXRefComments(ctx, issue); err != nil { + if err := filterXRefComments(ctx, issue); err != nil { ctx.ServerError("filterXRefComments", err) return } @@ -351,7 +367,7 @@ func ViewIssue(ctx *context.Context) { if ctx.IsSigned { // Update issue-user. - if err = activities_model.SetIssueReadBy(ctx, issue.ID, ctx.Doer.ID); err != nil { + if err := activities_model.SetIssueReadBy(ctx, issue.ID, ctx.Doer.ID); err != nil { ctx.ServerError("ReadBy", err) return } @@ -365,15 +381,13 @@ func ViewIssue(ctx *context.Context) { prepareFuncs := []func(*context.Context, *issues_model.Issue){ prepareIssueViewContent, - func(ctx *context.Context, issue *issues_model.Issue) { - preparePullViewPullInfo(ctx, issue) - }, prepareIssueViewCommentsAndSidebarParticipants, - preparePullViewReviewAndMerge, prepareIssueViewSidebarWatch, prepareIssueViewSidebarTimeTracker, prepareIssueViewSidebarDependency, prepareIssueViewSidebarPin, + func(ctx *context.Context, issue *issues_model.Issue) { preparePullViewPullInfo(ctx, issue) }, + preparePullViewReviewAndMerge, } for _, prepareFunc := range prepareFuncs { @@ -412,9 +426,25 @@ func ViewIssue(ctx *context.Context) { return user_service.CanBlockUser(ctx, ctx.Doer, blocker, blockee) } + if !issue.PullRequest.IsChecking() && !setting.IsProd { + ctx.Data["PullMergeBoxReloadingInterval"] = 1 // in dev env, force using the reloading logic to make sure it won't break + } + ctx.HTML(http.StatusOK, tplIssueView) } +func ViewPullMergeBox(ctx *context.Context) { + issue := prepareIssueViewLoad(ctx) + if !issue.IsPull { + ctx.NotFound(nil) + return + } + preparePullViewPullInfo(ctx, issue) + preparePullViewReviewAndMerge(ctx, issue) + ctx.Data["PullMergeBoxReloading"] = issue.PullRequest.IsChecking() + ctx.HTML(http.StatusOK, tplPullMergeBox) +} + func prepareIssueViewSidebarDependency(ctx *context.Context, issue *issues_model.Issue) { if issue.IsPull && !ctx.Repo.CanRead(unit.TypeIssues) { ctx.Data["IssueDependencySearchType"] = "pulls" @@ -840,6 +870,7 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss } } + ctx.Data["PullMergeBoxReloadingInterval"] = util.Iif(issue.PullRequest.IsChecking(), 2000, 0) ctx.Data["CanWriteToHeadRepo"] = canWriteToHeadRepo ctx.Data["ShowMergeInstructions"] = canWriteToHeadRepo ctx.Data["AllowMerge"] = allowMerge diff --git a/routers/web/web.go b/routers/web/web.go index f28dc6baa4d62..bd850baec0f46 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -1505,6 +1505,7 @@ func registerWebRoutes(m *web.Router) { m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewIssue) m.Get(".diff", repo.DownloadPullDiff) m.Get(".patch", repo.DownloadPullPatch) + m.Get("/merge_box", repo.ViewPullMergeBox) m.Group("/commits", func() { m.Get("", repo.SetWhitespaceBehavior, repo.GetPullDiffStats, repo.ViewPullCommits) m.Get("/list", repo.GetPullCommits) diff --git a/services/pull/check_test.go b/services/pull/check_test.go index 6d85ac158e88a..fa3a676ef1c98 100644 --- a/services/pull/check_test.go +++ b/services/pull/check_test.go @@ -36,7 +36,7 @@ func TestPullRequest_AddToTaskQueue(t *testing.T) { assert.NoError(t, err) pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) - AddToTaskQueue(db.DefaultContext, pr) + StartPullRequestCheckImmediately(db.DefaultContext, pr) assert.Eventually(t, func() bool { pr = unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2}) diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index 4671cfd6db59f..dae3c4ee6a909 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -68,7 +68,7 @@ {{template "repo/issue/view_content/comments" .}} {{if and .Issue.IsPull (not $.Repository.IsArchived)}} - {{template "repo/issue/view_content/pull".}} + {{template "repo/issue/view_content/pull_merge_box".}} {{end}} {{if .IsSigned}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull_merge_box.tmpl similarity index 97% rename from templates/repo/issue/view_content/pull.tmpl rename to templates/repo/issue/view_content/pull_merge_box.tmpl index eb4aeb15636dd..40f19324b0494 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull_merge_box.tmpl @@ -1,7 +1,13 @@ {{if and .Issue.PullRequest.HasMerged (not .IsPullBranchDeletable)}} {{/* Then the merge box will not be displayed because this page already contains enough information */}} {{else}} -
+
-
{{/* another similar form is in PullRequestMergeForm.vue*/}} + {{/* another similar form is in PullRequestMergeForm.vue*/}} {{.CsrfTokenHtml}}
diff --git a/templates/repo/issue/view_content/update_branch_by_merge.tmpl b/templates/repo/issue/view_content/update_branch_by_merge.tmpl index e0ed262f173fe..5d959bf0b3c1f 100644 --- a/templates/repo/issue/view_content/update_branch_by_merge.tmpl +++ b/templates/repo/issue/view_content/update_branch_by_merge.tmpl @@ -9,7 +9,7 @@ {{if and $.UpdateAllowed $.UpdateByRebaseAllowed}}
{{end}} {{if and $.UpdateAllowed (not $.UpdateByRebaseAllowed)}} - + {{$.CsrfTokenHtml}}