Skip to content

Commit c18a622

Browse files
wolfogrelunny
andauthored
Fix time to NotifyPullRequestSynchronized (#22650)
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 <[email protected]>
1 parent df789d9 commit c18a622

File tree

2 files changed

+30
-19
lines changed

2 files changed

+30
-19
lines changed

Diff for: models/issues/pull_list.go

+12-1
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
user_model "code.gitea.io/gitea/models/user"
1414
"code.gitea.io/gitea/modules/base"
1515
"code.gitea.io/gitea/modules/log"
16+
"code.gitea.io/gitea/modules/util"
1617

1718
"xorm.io/xorm"
1819
)
@@ -175,7 +176,17 @@ func (prs PullRequestList) loadAttributes(ctx context.Context) error {
175176
}
176177
for _, pr := range prs {
177178
pr.Issue = set[pr.IssueID]
178-
pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync
179+
/*
180+
Old code:
181+
pr.Issue.PullRequest = pr // panic here means issueIDs and prs are not in sync
182+
183+
It's worth panic because it's almost impossible to happen under normal use.
184+
But in integration testing, an asynchronous task could read a database that has been reset.
185+
So returning an error would make more sense, let the caller has a choice to ignore it.
186+
*/
187+
if pr.Issue == nil {
188+
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)
189+
}
179190
}
180191
return nil
181192
}

Diff for: services/pull/pull.go

+18-18
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
263263
return
264264
}
265265

266+
for _, pr := range prs {
267+
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
268+
if pr.Flow == issues_model.PullRequestFlowGithub {
269+
if err := PushToBaseRepo(ctx, pr); err != nil {
270+
log.Error("PushToBaseRepo: %v", err)
271+
continue
272+
}
273+
} else {
274+
continue
275+
}
276+
277+
AddToTaskQueue(pr)
278+
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
279+
if err == nil && comment != nil {
280+
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
281+
}
282+
}
283+
266284
if isSync {
267285
requests := issues_model.PullRequestList(prs)
268286
if err = requests.LoadAttributes(); err != nil {
@@ -303,24 +321,6 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
303321
}
304322
}
305323

306-
for _, pr := range prs {
307-
log.Trace("Updating PR[%d]: composing new test task", pr.ID)
308-
if pr.Flow == issues_model.PullRequestFlowGithub {
309-
if err := PushToBaseRepo(ctx, pr); err != nil {
310-
log.Error("PushToBaseRepo: %v", err)
311-
continue
312-
}
313-
} else {
314-
continue
315-
}
316-
317-
AddToTaskQueue(pr)
318-
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
319-
if err == nil && comment != nil {
320-
notification.NotifyPullRequestPushCommits(ctx, doer, pr, comment)
321-
}
322-
}
323-
324324
log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch)
325325
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(repoID, branch)
326326
if err != nil {

0 commit comments

Comments
 (0)