Skip to content

Commit

Permalink
Fix time to NotifyPullRequestSynchronized (#22650)
Browse files Browse the repository at this point in the history
Should call `PushToBaseRepo` before
`notification.NotifyPullRequestSynchronized`.

Or the notifier will get an old commit when reading branch
`pull/xxx/head`.

Found by ~#21937~ #22679.

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
  • Loading branch information
wolfogre and lunny committed Feb 5, 2023
1 parent df789d9 commit c18a622
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 19 deletions.
13 changes: 12 additions & 1 deletion models/issues/pull_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"

"xorm.io/xorm"
)
Expand Down Expand Up @@ -175,7 +176,17 @@ func (prs PullRequestList) loadAttributes(ctx context.Context) error {
}
for _, pr := range prs {
pr.Issue = set[pr.IssueID]
pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync
/*
Old code:
pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync
It's worth panic because it's almost impossible to happen under normal use.
But in integration testing, an asynchronous task could read a database that has been reset.
So returning an error would make more sense, let the caller has a choice to ignore it.
*/
if pr.Issue == nil {
return fmt.Errorf("issues and prs may be not in sync: cannot find issue %v for pr %v: %w", pr.IssueID, pr.ID, util.ErrNotExist)
}
}
return nil
}
Expand Down
36 changes: 18 additions & 18 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
return
}

for _, pr := range prs {
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
if pr.Flow == issues_model.PullRequestFlowGithub {
if err := PushToBaseRepo(ctx, pr); err != nil {
log.Error("PushToBaseRepo: %v", err)
continue
}
} else {
continue
}

AddToTaskQueue(pr)
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
if err == nil && comment != nil {
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
}
}

if isSync {
requests := issues_model.PullRequestList(prs)
if err = requests.LoadAttributes(); err != nil {
Expand Down Expand Up @@ -303,24 +321,6 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
}
}

for _, pr := range prs {
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
if pr.Flow == issues_model.PullRequestFlowGithub {
if err := PushToBaseRepo(ctx, pr); err != nil {
log.Error("PushToBaseRepo: %v", err)
continue
}
} else {
continue
}

AddToTaskQueue(pr)
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
if err == nil && comment != nil {
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
}
}

log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch)
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch)
if err != nil {
Expand Down

0 comments on commit c18a622

Please sign in to comment.