Skip to content

Commit 1e72480

Browse files
lunnywxiaoguang
lunny
and
wxiaoguang
authored
Pull request updates will also trigger code owners review requests (go-gitea#33744)
Fix go-gitea#33490 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 de2d472 commit 1e72480

File tree

8 files changed

+157
-37
lines changed

8 files changed

+157
-37
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
@@ -211,7 +211,15 @@ func Merge(ctx context.Context, pr *issues_model.PullRequest, doer *user_model.U
211211
}
212212
defer releaser()
213213
defer func() {
214-
go AddTestPullRequestTask(doer, pr.BaseRepo.ID, pr.BaseBranch, false, "", "")
214+
go AddTestPullRequestTask(TestPullRequestOptions{
215+
RepoID: pr.BaseRepo.ID,
216+
Doer: doer,
217+
Branch: pr.BaseBranch,
218+
IsSync: false,
219+
IsForcePush: false,
220+
OldCommitID: "",
221+
NewCommitID: "",
222+
})
215223
}()
216224

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

‎services/pull/pull.go‎

Lines changed: 42 additions & 17 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
}
@@ -372,19 +372,29 @@ func checkForInvalidation(ctx context.Context, requests issues_model.PullRequest
372372
return nil
373373
}
374374

375+
type TestPullRequestOptions struct {
376+
RepoID int64
377+
Doer *user_model.User
378+
Branch string
379+
IsSync bool // True means it's a pull request synchronization, false means it's triggered for pull request merging or updating
380+
IsForcePush bool
381+
OldCommitID string
382+
NewCommitID string
383+
}
384+
375385
// AddTestPullRequestTask adds new test tasks by given head/base repository and head/base branch,
376386
// and generate new patch for testing as needed.
377-
func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string, isSync bool, oldCommitID, newCommitID string) {
378-
log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", repoID, branch)
387+
func AddTestPullRequestTask(opts TestPullRequestOptions) {
388+
log.Trace("AddTestPullRequestTask [head_repo_id: %d, head_branch: %s]: finding pull requests", opts.RepoID, opts.Branch)
379389
graceful.GetManager().RunWithShutdownContext(func(ctx context.Context) {
380390
// There is no sensible way to shut this down ":-("
381391
// If you don't let it run all the way then you will lose data
382392
// TODO: graceful: AddTestPullRequestTask needs to become a queue!
383393

384394
// GetUnmergedPullRequestsByHeadInfo() only return open and unmerged PR.
385-
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, repoID, branch)
395+
prs, err := issues_model.GetUnmergedPullRequestsByHeadInfo(ctx, opts.RepoID, opts.Branch)
386396
if err != nil {
387-
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", repoID, branch, err)
397+
log.Error("Find pull requests [head_repo_id: %d, head_branch: %s]: %v", opts.RepoID, opts.Branch, err)
388398
return
389399
}
390400

@@ -400,24 +410,24 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
400410
}
401411

402412
AddToTaskQueue(ctx, pr)
403-
comment, err := CreatePushPullComment(ctx, doer, pr, oldCommitID, newCommitID)
413+
comment, err := CreatePushPullComment(ctx, opts.Doer, pr, opts.OldCommitID, opts.NewCommitID)
404414
if err == nil && comment != nil {
405-
notify_service.PullRequestPushCommits(ctx, doer, pr, comment)
415+
notify_service.PullRequestPushCommits(ctx, opts.Doer, pr, comment)
406416
}
407417
}
408418

409-
if isSync {
419+
if opts.IsSync {
410420
if err = prs.LoadAttributes(ctx); err != nil {
411421
log.Error("PullRequestList.LoadAttributes: %v", err)
412422
}
413-
if invalidationErr := checkForInvalidation(ctx, prs, repoID, doer, branch); invalidationErr != nil {
423+
if invalidationErr := checkForInvalidation(ctx, prs, opts.RepoID, opts.Doer, opts.Branch); invalidationErr != nil {
414424
log.Error("checkForInvalidation: %v", invalidationErr)
415425
}
416426
if err == nil {
417427
for _, pr := range prs {
418428
objectFormat := git.ObjectFormatFromName(pr.BaseRepo.ObjectFormatName)
419-
if newCommitID != "" && newCommitID != objectFormat.EmptyObjectID().String() {
420-
changed, err := checkIfPRContentChanged(ctx, pr, oldCommitID, newCommitID)
429+
if opts.NewCommitID != "" && opts.NewCommitID != objectFormat.EmptyObjectID().String() {
430+
changed, err := checkIfPRContentChanged(ctx, pr, opts.OldCommitID, opts.NewCommitID)
421431
if err != nil {
422432
log.Error("checkIfPRContentChanged: %v", err)
423433
}
@@ -433,12 +443,12 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
433443
log.Error("GetFirstMatchProtectedBranchRule: %v", err)
434444
}
435445
if pb != nil && pb.DismissStaleApprovals {
436-
if err := DismissApprovalReviews(ctx, doer, pr); err != nil {
446+
if err := DismissApprovalReviews(ctx, opts.Doer, pr); err != nil {
437447
log.Error("DismissApprovalReviews: %v", err)
438448
}
439449
}
440450
}
441-
if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, newCommitID); err != nil {
451+
if err := issues_model.MarkReviewsAsNotStale(ctx, pr.IssueID, opts.NewCommitID); err != nil {
442452
log.Error("MarkReviewsAsNotStale: %v", err)
443453
}
444454
divergence, err := GetDiverging(ctx, pr)
@@ -452,15 +462,30 @@ func AddTestPullRequestTask(doer *user_model.User, repoID int64, branch string,
452462
}
453463
}
454464

455-
notify_service.PullRequestSynchronized(ctx, doer, pr)
465+
if !pr.IsWorkInProgress(ctx) {
466+
var reviewNotifiers []*issue_service.ReviewRequestNotifier
467+
if opts.IsForcePush {
468+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReview(ctx, pr)
469+
} else {
470+
reviewNotifiers, err = issue_service.PullRequestCodeOwnersReviewSpecialCommits(ctx, pr, opts.OldCommitID, opts.NewCommitID)
471+
}
472+
if err != nil {
473+
log.Error("PullRequestCodeOwnersReview: %v", err)
474+
}
475+
if len(reviewNotifiers) > 0 {
476+
issue_service.ReviewRequestNotify(ctx, pr.Issue, opts.Doer, reviewNotifiers)
477+
}
478+
}
479+
480+
notify_service.PullRequestSynchronized(ctx, opts.Doer, pr)
456481
}
457482
}
458483
}
459484

460-
log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", repoID, branch)
461-
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, repoID, branch)
485+
log.Trace("AddTestPullRequestTask [base_repo_id: %d, base_branch: %s]: finding pull requests", opts.RepoID, opts.Branch)
486+
prs, err = issues_model.GetUnmergedPullRequestsByBaseInfo(ctx, opts.RepoID, opts.Branch)
462487
if err != nil {
463-
log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", repoID, branch, err)
488+
log.Error("Find pull requests [base_repo_id: %d, base_branch: %s]: %v", opts.RepoID, opts.Branch, err)
464489
return
465490
}
466491
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
@@ -166,7 +166,6 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
166166
branch := opts.RefFullName.BranchName()
167167
if !opts.IsDelRef() {
168168
log.Trace("TriggerTask '%s/%s' by %s", repo.Name, branch, pusher.Name)
169-
go pull_service.AddTestPullRequestTask(pusher, repo.ID, branch, true, opts.OldCommitID, opts.NewCommitID)
170169

171170
newCommit, err := gitRepo.GetCommit(opts.NewCommitID)
172171
if err != nil {
@@ -208,6 +207,17 @@ func pushUpdates(optsList []*repo_module.PushUpdateOptions) error {
208207
log.Error("IsForcePush %s:%s failed: %v", repo.FullName(), branch, err)
209208
}
210209

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

‎tests/integration/pull_review_test.go‎

Lines changed: 39 additions & 3 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.AssertExistsAndLoadBean(t, &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))
@@ -140,6 +171,11 @@ func TestPullView_CodeOwner(t *testing.T) {
140171

141172
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"})
142173
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)