Skip to content

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

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

Merged
merged 14 commits into from
Apr 24, 2025
Merged
4 changes: 4 additions & 0 deletions custom/conf/app.example.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good default to me.

Would this be considered an API breaking change? I guess you already have to poll in case it happens to be checking, and an API access will trigger the check to happen so it should eventually return an actual status.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe we could mention it in the changelog, and collect user feedbacks.


;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
Expand Down
22 changes: 0 additions & 22 deletions models/issues/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"fmt"
"io"
"regexp"
"strconv"
"strings"

"code.gitea.io/gitea/models/db"
Expand Down Expand Up @@ -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

Expand Down
3 changes: 3 additions & 0 deletions modules/setting/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ var (
AddCoCommitterTrailers bool
TestConflictingPatchesWithGitApply bool
RetargetChildrenOnMerge bool
DelayCheckForInactiveDays int
} `ini:"repository.pull-request"`

// Issue Setting
Expand Down Expand Up @@ -200,6 +201,7 @@ var (
AddCoCommitterTrailers bool
TestConflictingPatchesWithGitApply bool
RetargetChildrenOnMerge bool
DelayCheckForInactiveDays int
}{
WorkInProgressPrefixes: []string{"WIP:", "[WIP]"},
// Same as GitHub. See
Expand All @@ -215,6 +217,7 @@ var (
PopulateSquashCommentWithCommitMessages: false,
AddCoCommitterTrailers: true,
RetargetChildrenOnMerge: true,
DelayCheckForInactiveDays: 7,
},

// Issue settings
Expand Down
12 changes: 10 additions & 2 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}

Expand Down Expand Up @@ -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))
}

Expand Down Expand Up @@ -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)
Expand Down
3 changes: 2 additions & 1 deletion routers/private/hook_pre_receive.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package private

import (
"errors"
"fmt"
"net/http"
"os"
Expand Down Expand Up @@ -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()),
Expand Down
3 changes: 2 additions & 1 deletion routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
69 changes: 51 additions & 18 deletions routers/web/repo/issue_view.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -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.
Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 ...
Expand Down
1 change: 1 addition & 0 deletions routers/web/web.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion services/agit/agit.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion services/automerge/automerge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion services/migrations/gitea_uploader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
Loading