Option to delay conflict checking of old pull requests until page view (#27779)

`[repository.pull-request] DELAY_CHECK_FOR_INACTIVE_DAYS` is a new
setting to delay the mergeable check for pull requests that have been
inactive for the specified number of days.

This avoids potentially long delays for big repositories with many pull
requests. and reduces system load overall when there are many
repositories or pull requests.

When viewing the PR, checking will start immediately and the PR merge
box will automatically reload when complete. Accessing the PR through
the API will also start checking immediately.

The default value of `7` provides a balance between system load, and
keeping behavior similar to what it was before both for users and API
access. With `0` all conflict checking will be delayed, while `-1`
always checks immediately to restore the previous behavior.

---------

Co-authored-by: wxiaoguang <[email protected]>

Author: brechtvl

custom/conf/app.example.ini

@@ -1155,6 +1155,10 @@ 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 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
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;

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
 

modules/setting/repository.go

@@ -82,6 +82,7 @@ var (
 			AddCoCommitterTrailers                   bool
 			TestConflictingPatchesWithGitApply       bool
 			RetargetChildrenOnMerge                  bool
+			DelayCheckForInactiveDays                int
 		} `ini:"repository.pull-request"`
 
 		// Issue Setting
@@ -200,6 +201,7 @@ var (
 			AddCoCommitterTrailers                   bool
 			TestConflictingPatchesWithGitApply       bool
 			RetargetChildrenOnMerge                  bool
+			DelayCheckForInactiveDays                int
 		}{
 			WorkInProgressPrefixes: []string{"WIP:", "[WIP]"},
 			// Same as GitHub. See
@@ -215,6 +217,7 @@ var (
 			PopulateSquashCommentWithCommitMessages:  false,
 			AddCoCommitterTrailers:                   true,
 			RetargetChildrenOnMerge:                  true,
+			DelayCheckForInactiveDays:                7,
 		},
 
 		// Issue settings

options/locale/locale_en-US.ini

@@ -1880,7 +1880,7 @@ pulls.add_prefix = Add <strong>%s</strong> prefix
 pulls.remove_prefix = Remove <strong>%s</strong> prefix
 pulls.data_broken = This pull request is broken due to missing fork information.
 pulls.files_conflicted = This pull request has changes conflicting with the target branch.
-pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
+pulls.is_checking = Checking for merge conflicts ...
 pulls.is_ancestor = "This branch is already included in the target branch. There is nothing to merge."
 pulls.is_empty = "The changes on this branch are already on the target branch. This will be an empty commit."
 pulls.required_status_check_failed = Some required checks were not successful.

routers/api/v1/repo/pull.go

@@ -202,6 +202,10 @@ func GetPullRequest(ctx *context.APIContext) {
 		ctx.APIErrorInternal(err)
 		return
 	}
+
+	// Consider API access a view for delayed checking.
+	pull_service.StartPullRequestCheckOnView(ctx, pr)
+
 	ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer))
 }
 
@@ -287,6 +291,10 @@ func GetPullRequestByBaseHead(ctx *context.APIContext) {
 		ctx.APIErrorInternal(err)
 		return
 	}
+
+	// Consider API access a view for delayed checking.
+	pull_service.StartPullRequestCheckOnView(ctx, pr)
+
 	ctx.JSON(http.StatusOK, convert.ToAPIPullRequest(ctx, pr, ctx.Doer))
 }
 
@@ -921,15 +929,15 @@ 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, "")
 		} else if errors.Is(err, pull_service.ErrIsWorkInProgress) {
 			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)

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()),

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"

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 <refs/pulls/pr_index/head> in base repo is consistent with the head commit of head branch in the head repo

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 != nil && !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"
@@ -792,6 +822,8 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
 	allowMerge := false
 	canWriteToHeadRepo := false
 
+	pull_service.StartPullRequestCheckOnView(ctx, pull)
+
 	if ctx.IsSigned {
 		if err := pull.LoadHeadRepo(ctx); err != nil {
 			log.Error("LoadHeadRepo: %v", err)
@@ -838,6 +870,7 @@ func preparePullViewReviewAndMerge(ctx *context.Context, issue *issues_model.Iss
 		}
 	}
 
+	ctx.Data["PullMergeBoxReloadingInterval"] = util.Iif(pull != nil && pull.IsChecking(), 2000, 0)
 	ctx.Data["CanWriteToHeadRepo"] = canWriteToHeadRepo
 	ctx.Data["ShowMergeInstructions"] = canWriteToHeadRepo
 	ctx.Data["AllowMerge"] = allowMerge
@@ -958,5 +991,4 @@ func prepareIssueViewContent(ctx *context.Context, issue *issues_model.Issue) {
 		ctx.ServerError("roleDescriptor", err)
 		return
 	}
-	ctx.Data["Issue"] = issue
 }

routers/web/repo/pull.go

@@ -1052,15 +1052,15 @@ 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"))
 		case errors.Is(err, pull_service.ErrIsWorkInProgress):
 			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 ...