Skip to content

Commit 7f962a1

Browse files
lunnywxiaoguang
lunny
and
wxiaoguang
authored
Pull request updates will also trigger code owners review requests (#33744) (#34045)
Fix #33490 Backport #33744 It will only read the changed file on the pushed commits but not all the files of this PR. Co-authored-by: wxiaoguang <[email protected]>
1 parent 5d81f6d commit 7f962a1

File tree

8 files changed

+159
-40
lines changed

8 files changed

+159
-40
lines changed

‎routers/web/repo/view_file.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ import (
99
"image"
1010
"io"
1111
"path"
12-
"slices"
1312
"strings"
1413

1514
git_model "code.gitea.io/gitea/models/git"
@@ -79,7 +78,7 @@ func prepareToRenderFile(ctx *context.Context, entry *git.TreeEntry) {
7978
if workFlowErr != nil {
8079
ctx.Data["FileError"] = ctx.Locale.Tr("actions.runs.invalid_workflow_helper", workFlowErr.Error())
8180
}
82-
} else if slices.Contains([]string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}, ctx.Repo.TreePath) {
81+
} else if issue_service.IsCodeOwnerFile(ctx.Repo.TreePath) {
8382
if data, err := blob.GetBlobContent(setting.UI.MaxDisplayFileSize); err == nil {
8483
_, warnings := issue_model.GetCodeOwnersFromContent(ctx, data)
8584
if len(warnings) > 0 {

‎services/issue/issue.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -92,8 +92,12 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode
9292

9393
var reviewNotifiers []*ReviewRequestNotifier
9494
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) {
95+
if err := issue.LoadPullRequest(ctx); err != nil {
96+
return err
97+
}
98+
9599
var err error
96-
reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest)
100+
reviewNotifiers, err = PullRequestCodeOwnersReview(ctx, issue.PullRequest)
97101
if err != nil {
98102
log.Error("PullRequestCodeOwnersReview: %v", err)
99103
}

‎services/issue/pull.go‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package issue
66
import (
77
"context"
88
"fmt"
9+
"slices"
910
"time"
1011

1112
issues_model "code.gitea.io/gitea/models/issues"
@@ -40,20 +41,31 @@ type ReviewRequestNotifier struct {
4041
ReviewTeam *org_model.Team
4142
}
4243

43-
func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) {
44-
files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
44+
var codeOwnerFiles = []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
4545

46+
func IsCodeOwnerFile(f string) bool {
47+
return slices.Contains(codeOwnerFiles, f)
48+
}
49+
50+
func PullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullRequest) ([]*ReviewRequestNotifier, error) {
51+
return PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, "", "") // no commit is provided, then it uses PR's base&head branch
52+
}
53+
54+
func PullRequestCodeOwnersReviewSpecialCommits(ctx context.Context, pr *issues_model.PullRequest, startCommitID, endCommitID string) ([]*ReviewRequestNotifier, error) {
55+
if err := pr.LoadIssue(ctx); err != nil {
56+
return nil, err
57+
}
58+
issue := pr.Issue
4659
if pr.IsWorkInProgress(ctx) {
4760
return nil, nil
4861
}
49-
5062
if err := pr.LoadHeadRepo(ctx); err != nil {
5163
return nil, err
5264
}
53-
5465
if err := pr.LoadBaseRepo(ctx); err != nil {
5566
return nil, err
5667
}
68+
pr.Issue.Repo = pr.BaseRepo
5769

5870
if pr.BaseRepo.IsFork {
5971
return nil, nil
@@ -71,26 +83,36 @@ func PullRequestCodeOwnersReview(ctx context.Context, issue *issues_model.Issue,
7183
}
7284

7385
var data string
74-
for _, file := range files {
86+
for _, file := range codeOwnerFiles {
7587
if blob, err := commit.GetBlobByPath(file); err == nil {
7688
data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
7789
if err == nil {
7890
break
7991
}
8092
}
8193
}
94+
if data == "" {
95+
return nil, nil
96+
}
8297

8398
rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data)
99+
if len(rules) == 0 {
100+
return nil, nil
101+
}
84102

85-
// get the mergebase
86-
mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
87-
if err != nil {
88-
return nil, err
103+
if startCommitID == "" && endCommitID == "" {
104+
// get the mergebase
105+
mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
106+
if err != nil {
107+
return nil, err
108+
}
109+
startCommitID = mergeBase
110+
endCommitID = pr.GetGitRefName()
89111
}
90112

91113
// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
92114
// between the merge base and the head commit but not the base branch and the head commit
93-
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.GetGitRefName())
115+
changedFiles, err := repo.GetFilesChangedBetween(startCommitID, endCommitID)
94116
if err != nil {
95117
return nil, err
96118
}

‎services/pull/merge.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
189189
}
190190
defer releaser()
191191
defer func() {
192-
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
192+
go AddTestPullRequestTask(TestPullRequestOptions{
193+
RepoID: pr.BaseRepo.ID,
194+
Doer: doer,
195+
Branch: pr.BaseBranch,
196+
IsSync: false,
197+
IsForcePush: false,
198+
OldCommitID: "",
199+
NewCommitID: "",
200+
})
193201
}()
194202

195203
_, err = doMergeAndPush(ctx, pr, doer, mergeStyle, expectedHeadCommitID, message, repo_module.PushTriggerPRMergeToBase)

‎services/pull/pull.go‎

Lines changed: 43 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func NewPullRequest(ctx context.Context, opts *NewPullRequestOptions) error {
176176
}
177177

178178
if !pr.IsWorkInProgress(ctx) {
179-
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, issue, pr)
179+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr)
180180
if err != nil {
181181
return err
182182
}
@@ -349,19 +349,29 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest
349349
return nil
350350
}
351351

352+
type TestPullRequestOptions struct {
353+
RepoID int64
354+
Doer *user_model.User
355+
Branch string
356+
IsSync bool // True means it's a pull request synchronization, false means it's triggered for pull request merging or updating
357+
IsForcePush bool
358+
OldCommitID string
359+
NewCommitID string
360+
}
361+
352362
// AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch,
353363
// and generate new patch for testing as needed.
354-
func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) {
355-
log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch)
364+
func AddTestPullRequestTask(opts TestPullRequestOptions) {
365+
log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch)
356366
graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) {
357367
// There is no sensible way to shut this down ":-("
358368
// If you don't let it run all the way then you will lose data
359369
// TODO: graceful: AddTestPullRequestTask needs to become a queue!
360370

361371
// GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR.
362-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch)
372+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch)
363373
if err != nil {
364-
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err)
374+
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", opts.RepoID, opts.Branch, err)
365375
return
366376
}
367377

@@ -377,25 +387,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
377387
}
378388

379389
AddToTaskQueue(ctx, pr)
380-
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
390+
comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID)
381391
if err == nil && comment != nil {
382-
notify_service.PullRequestPushCommits(ctx, doer, pr, comment)
392+
notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment)
383393
}
384394
}
385395

386-
if isSync {
387-
requests := issues_model.PullRequestList(prs)
388-
if err = requests.LoadAttributes(ctx); err != nil {
396+
if opts.IsSync {
397+
if err = issues_model.PullRequestList(prs).LoadAttributes(ctx); err != nil {
389398
log.Error("PullRequestList.LoadAttributes: %v", err)
390399
}
391-
if invalidationErr := checkForInvalidation(ctx, requests, repoID, doer, branch); invalidationErr != nil {
400+
if invalidationErr := checkForInvalidation(ctx, prs, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil {
392401
log.Error("checkForInvalidation: %v", invalidationErr)
393402
}
394403
if err == nil {
395404
for _, pr := range prs {
396405
objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
397-
if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() {
398-
changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID)
406+
if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() {
407+
changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID)
399408
if err != nil {
400409
log.Error("checkIfPRContentChanged: %v", err)
401410
}
@@ -411,12 +420,12 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
411420
log.Error("GetFirstMatchProtectedBranchRule: %v", err)
412421
}
413422
if pb != nil && pb.DismissStaleApprovals {
414-
if err := DismissApprovalReviews(ctx, doer, pr); err != nil {
423+
if err := DismissApprovalReviews(ctx, opts.Doer, pr); err != nil {
415424
log.Error("DismissApprovalReviews: %v", err)
416425
}
417426
}
418427
}
419-
if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil {
428+
if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil {
420429
log.Error("MarkReviewsAsNotStale: %v", err)
421430
}
422431
divergence, err := GetDiverging(ctx, pr)
@@ -430,15 +439,30 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
430439
}
431440
}
432441

433-
notify_service.PullRequestSynchronized(ctx, doer, pr)
442+
if !pr.IsWorkInProgress(ctx) {
443+
var reviewNotifiers []*issue_service.ReviewRequestNotifier
444+
if opts.IsForcePush {
445+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr)
446+
} else {
447+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, opts.OldCommitID, opts.NewCommitID)
448+
}
449+
if err != nil {
450+
log.Error("PullRequestCodeOwnersReview: %v", err)
451+
}
452+
if len(reviewNotifiers) > 0 {
453+
issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers)
454+
}
455+
}
456+
457+
notify_service.PullRequestSynchronized(ctx, opts.Doer, pr)
434458
}
435459
}
436460
}
437461

438-
log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch)
439-
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch)
462+
log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch)
463+
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch)
440464
if err != nil {
441-
log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repoID, branch, err)
465+
log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err)
442466
return
443467
}
444468
for _, pr := range prs {

‎services/pull/update.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,15 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
4242

4343
if rebase {
4444
defer func() {
45-
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
45+
go AddTestPullRequestTask(TestPullRequestOptions{
46+
RepoID: pr.BaseRepo.ID,
47+
Doer: doer,
48+
Branch: pr.BaseBranch,
49+
IsSync: false,
50+
IsForcePush: false,
51+
OldCommitID: "",
52+
NewCommitID: "",
53+
})
4654
}()
4755

4856
return updateHeadByRebaseOnToBase(ctx, pr, doer)
@@ -83,7 +91,15 @@ func Update(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.
8391
_, err = doMergeAndPush(ctx, reversePR, doer, repo_model.MergeStyleMerge, "", message, repository.PushTriggerPRUpdateWithBase)
8492

8593
defer func() {
86-
go AddTestPullRequestTask(doer, reversePR.HeadRepo.ID, reversePR.HeadBranch, false, "", "")
94+
go AddTestPullRequestTask(TestPullRequestOptions{
95+
RepoID: reversePR.HeadRepo.ID,
96+
Doer: doer,
97+
Branch: reversePR.HeadBranch,
98+
IsSync: false,
99+
IsForcePush: false,
100+
OldCommitID: "",
101+
NewCommitID: "",
102+
})
87103
}()
88104

89105
return err

‎services/repository/push.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
170170
branch := opts.RefFullName.BranchName()
171171
if !opts.IsDelRef() {
172172
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
173-
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID)
174173

175174
newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
176175
if err != nil {
@@ -212,6 +211,17 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
212211
log.Error("IsForcePush %s:%s failed: %v", repo.FullName(), branch, err)
213212
}
214213

214+
// only update branch can trigger pull request task because the pull request hasn't been created yet when creaing a branch
215+
go pull_service.AddTestPullRequestTask(pull_service.TestPullRequestOptions{
216+
RepoID: repo.ID,
217+
Doer: pusher,
218+
Branch: branch,
219+
IsSync: true,
220+
IsForcePush: isForcePush,
221+
OldCommitID: opts.OldCommitID,
222+
NewCommitID: opts.NewCommitID,
223+
})
224+
215225
if isForcePush {
216226
log.Trace("Push %s is a force push", opts.NewCommitID)
217227

‎tests/integration/pull_review_test.go‎

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"net/http/httptest"
99
"net/url"
1010
"path"
11+
"sort"
1112
"strings"
1213
"testing"
1314

@@ -67,15 +68,15 @@ func TestPullView_CodeOwner(t *testing.T) {
6768
{
6869
Operation: "create",
6970
TreePath: "CODEOWNERS",
70-
ContentReader: strings.NewReader("README.md @user5\n"),
71+
ContentReader: strings.NewReader("README.md @user5\nuser8-file.md @user8\n"),
7172
},
7273
},
7374
})
7475
assert.NoError(t, err)
7576

7677
t.Run("First Pull Request", func(t *testing.T) {
7778
// create a new branch to prepare for pull request
78-
_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
79+
resp1, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
7980
NewBranch: "codeowner-basebranch",
8081
Files: []*files_service.ChangeRepoFile{
8182
{
@@ -95,7 +96,37 @@ func TestPullView_CodeOwner(t *testing.T) {
9596
unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
9697
assert.NoError(t, pr.LoadIssue(db.DefaultContext))
9798

98-
err := issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request")
99+
reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr)
100+
assert.NoError(t, err)
101+
assert.Len(t, reviewNotifiers, 1)
102+
assert.EqualValues(t, 5, reviewNotifiers[0].Reviewer.ID)
103+
104+
// update the file on the pr branch
105+
resp2, err := files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
106+
OldBranch: "codeowner-basebranch",
107+
Files: []*files_service.ChangeRepoFile{
108+
{
109+
Operation: "create",
110+
TreePath: "user8-file.md",
111+
ContentReader: strings.NewReader("# This is a new project2\n"),
112+
},
113+
},
114+
})
115+
assert.NoError(t, err)
116+
117+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr)
118+
assert.NoError(t, err)
119+
assert.Len(t, reviewNotifiers, 2)
120+
reviewerIDs := []int64{reviewNotifiers[0].Reviewer.ID, reviewNotifiers[1].Reviewer.ID}
121+
sort.Slice(reviewerIDs, func(i, j int) bool { return reviewerIDs[i] < reviewerIDs[j] })
122+
assert.EqualValues(t, []int64{5, 8}, reviewerIDs)
123+
124+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(db.DefaultContext, pr, resp1.Commit.SHA, resp2.Commit.SHA)
125+
assert.NoError(t, err)
126+
assert.Len(t, reviewNotifiers, 1)
127+
assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID)
128+
129+
err = issue_service.ChangeTitle(db.DefaultContext, pr.Issue, user2, "[WIP] Test Pull Request")
99130
assert.NoError(t, err)
100131
prUpdated1 := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: pr.ID})
101132
assert.NoError(t, prUpdated1.LoadIssue(db.DefaultContext))
@@ -139,7 +170,12 @@ func TestPullView_CodeOwner(t *testing.T) {
139170
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2")
140171

141172
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"})
142-
unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
173+
unittest.AssertExistsAndLoadBean(t, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
174+
175+
reviewNotifiers, err := issue_service.PullRequestCodeOwnersReview(db.DefaultContext, pr)
176+
assert.NoError(t, err)
177+
assert.Len(t, reviewNotifiers, 1)
178+
assert.EqualValues(t, 8, reviewNotifiers[0].Reviewer.ID)
143179
})
144180

145181
t.Run("Forked Repo Pull Request", func(t *testing.T) {

0 commit comments

Comments
 (0)