Skip to content

Commit 277f90d

Browse files
lunnywxiaoguang
lunny
and
wxiaoguang
authored
Fix codeowner detected diff base branch to mergebase (#29783)
Fix #29763 This PR fixes 2 problems with CodeOwner in the pull request. - Don't use the pull request base branch but merge-base as a diff base to detect the code owner. - CodeOwner detection in fork repositories will be disabled because almost all the fork repositories will not change CODEOWNERS files but it should not be used on fork repositories' pull requests. --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
1 parent 0827552 commit 277f90d

File tree

5 files changed

+248
-74
lines changed

5 files changed

+248
-74
lines changed

‎models/issues/pull.go

Lines changed: 0 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ import (
1919
repo_model "code.gitea.io/gitea/models/repo"
2020
user_model "code.gitea.io/gitea/models/user"
2121
"code.gitea.io/gitea/modules/git"
22-
"code.gitea.io/gitea/modules/gitrepo"
2322
"code.gitea.io/gitea/modules/log"
2423
"code.gitea.io/gitea/modules/setting"
2524
"code.gitea.io/gitea/modules/timeutil"
@@ -884,77 +883,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr *
884883
return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0
885884
}
886885

887-
func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullRequest) error {
888-
files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
889-
890-
if pr.IsWorkInProgress(ctx) {
891-
return nil
892-
}
893-
894-
if err := pr.LoadBaseRepo(ctx); err != nil {
895-
return err
896-
}
897-
898-
repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
899-
if err != nil {
900-
return err
901-
}
902-
defer repo.Close()
903-
904-
commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch)
905-
if err != nil {
906-
return err
907-
}
908-
909-
var data string
910-
for _, file := range files {
911-
if blob, err := commit.GetBlobByPath(file); err == nil {
912-
data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
913-
if err == nil {
914-
break
915-
}
916-
}
917-
}
918-
919-
rules, _ := GetCodeOwnersFromContent(ctx, data)
920-
changedFiles, err := repo.GetFilesChangedBetween(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
921-
if err != nil {
922-
return err
923-
}
924-
925-
uniqUsers := make(map[int64]*user_model.User)
926-
uniqTeams := make(map[string]*org_model.Team)
927-
for _, rule := range rules {
928-
for _, f := range changedFiles {
929-
if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) {
930-
for _, u := range rule.Users {
931-
uniqUsers[u.ID] = u
932-
}
933-
for _, t := range rule.Teams {
934-
uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t
935-
}
936-
}
937-
}
938-
}
939-
940-
for _, u := range uniqUsers {
941-
if u.ID != pull.Poster.ID {
942-
if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {
943-
log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
944-
return err
945-
}
946-
}
947-
}
948-
for _, t := range uniqTeams {
949-
if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {
950-
log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
951-
return err
952-
}
953-
}
954-
955-
return nil
956-
}
957-
958886
// GetCodeOwnersFromContent returns the code owners configuration
959887
// Return empty slice if files missing
960888
// Return warning messages on parsing errors

‎services/issue/issue.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,7 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode
9090
}
9191

9292
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) {
93-
if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil {
93+
if err := PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil {
9494
return err
9595
}
9696
}

‎services/issue/pull.go

Lines changed: 122 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,122 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package issue
5+
6+
import (
7+
"context"
8+
"fmt"
9+
"time"
10+
11+
issues_model "code.gitea.io/gitea/models/issues"
12+
org_model "code.gitea.io/gitea/models/organization"
13+
user_model "code.gitea.io/gitea/models/user"
14+
"code.gitea.io/gitea/modules/git"
15+
"code.gitea.io/gitea/modules/gitrepo"
16+
"code.gitea.io/gitea/modules/log"
17+
"code.gitea.io/gitea/modules/setting"
18+
)
19+
20+
func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) {
21+
// Add a temporary remote
22+
tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano())
23+
if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil {
24+
return "", fmt.Errorf("AddRemote: %w", err)
25+
}
26+
defer func() {
27+
if err := repo.RemoveRemote(tmpRemote); err != nil {
28+
log.Error("getMergeBase: RemoveRemote: %v", err)
29+
}
30+
}()
31+
32+
mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch)
33+
return mergeBase, err
34+
}
35+
36+
func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) error {
37+
files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}
38+
39+
if pr.IsWorkInProgress(ctx) {
40+
return nil
41+
}
42+
43+
if err := pr.LoadHeadRepo(ctx); err != nil {
44+
return err
45+
}
46+
47+
if pr.HeadRepo.IsFork {
48+
return nil
49+
}
50+
51+
if err := pr.LoadBaseRepo(ctx); err != nil {
52+
return err
53+
}
54+
55+
repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
56+
if err != nil {
57+
return err
58+
}
59+
defer repo.Close()
60+
61+
commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch)
62+
if err != nil {
63+
return err
64+
}
65+
66+
var data string
67+
for _, file := range files {
68+
if blob, err := commit.GetBlobByPath(file); err == nil {
69+
data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
70+
if err == nil {
71+
break
72+
}
73+
}
74+
}
75+
76+
rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data)
77+
78+
// get the mergebase
79+
mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
80+
if err != nil {
81+
return err
82+
}
83+
84+
// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
85+
// between the merge base and the head commit but not the base branch and the head commit
86+
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.HeadCommitID)
87+
if err != nil {
88+
return err
89+
}
90+
91+
uniqUsers := make(map[int64]*user_model.User)
92+
uniqTeams := make(map[string]*org_model.Team)
93+
for _, rule := range rules {
94+
for _, f := range changedFiles {
95+
if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) {
96+
for _, u := range rule.Users {
97+
uniqUsers[u.ID] = u
98+
}
99+
for _, t := range rule.Teams {
100+
uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t
101+
}
102+
}
103+
}
104+
}
105+
106+
for _, u := range uniqUsers {
107+
if u.ID != pull.Poster.ID {
108+
if _, err := issues_model.AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {
109+
log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
110+
return err
111+
}
112+
}
113+
}
114+
for _, t := range uniqTeams {
115+
if _, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {
116+
log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
117+
return err
118+
}
119+
}
120+
121+
return nil
122+
}

‎services/pull/pull.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
136136
}
137137

138138
if !pr.IsWorkInProgress(ctx) {
139-
if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
139+
if err := issue_service.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
140140
return err
141141
}
142142
}

‎tests/integration/pull_review_test.go

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,20 @@ package integration
55

66
import (
77
"net/http"
8+
"net/url"
9+
"strings"
810
"testing"
911

12+
"code.gitea.io/gitea/models/db"
13+
issues_model "code.gitea.io/gitea/models/issues"
14+
"code.gitea.io/gitea/models/unittest"
15+
user_model "code.gitea.io/gitea/models/user"
16+
"code.gitea.io/gitea/modules/git"
17+
repo_service "code.gitea.io/gitea/services/repository"
18+
files_service "code.gitea.io/gitea/services/repository/files"
1019
"code.gitea.io/gitea/tests"
20+
21+
"github.com/stretchr/testify/assert"
1122
)
1223

1324
func TestPullView_ReviewerMissed(t *testing.T) {
@@ -20,3 +31,116 @@ func TestPullView_ReviewerMissed(t *testing.T) {
2031
req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
2132
session.MakeRequest(t, req, http.StatusOK)
2233
}
34+
35+
func TestPullView_CodeOwner(t *testing.T) {
36+
onGiteaRun(t, func(t *testing.T, u *url.URL) {
37+
user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
38+
39+
// Create the repo.
40+
repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
41+
Name: "test_codeowner",
42+
Readme: "Default",
43+
AutoInit: true,
44+
ObjectFormatName: git.Sha1ObjectFormat.Name(),
45+
DefaultBranch: "master",
46+
})
47+
assert.NoError(t, err)
48+
49+
// add CODEOWNERS to default branch
50+
_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
51+
OldBranch: repo.DefaultBranch,
52+
Files: []*files_service.ChangeRepoFile{
53+
{
54+
Operation: "create",
55+
TreePath: "CODEOWNERS",
56+
ContentReader: strings.NewReader("README.md @user5\n"),
57+
},
58+
},
59+
})
60+
assert.NoError(t, err)
61+
62+
t.Run("First Pull Request", func(t *testing.T) {
63+
// create a new branch to prepare for pull request
64+
_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
65+
NewBranch: "codeowner-basebranch",
66+
Files: []*files_service.ChangeRepoFile{
67+
{
68+
Operation: "update",
69+
TreePath: "README.md",
70+
ContentReader: strings.NewReader("# This is a new project\n"),
71+
},
72+
},
73+
})
74+
assert.NoError(t, err)
75+
76+
// Create a pull request.
77+
session := loginUser(t, "user2")
78+
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request")
79+
80+
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"})
81+
unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
82+
})
83+
84+
// change the default branch CODEOWNERS file to change README.md's codeowner
85+
_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
86+
Files: []*files_service.ChangeRepoFile{
87+
{
88+
Operation: "update",
89+
TreePath: "CODEOWNERS",
90+
ContentReader: strings.NewReader("README.md @user8\n"),
91+
},
92+
},
93+
})
94+
assert.NoError(t, err)
95+
96+
t.Run("Second Pull Request", func(t *testing.T) {
97+
// create a new branch to prepare for pull request
98+
_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
99+
NewBranch: "codeowner-basebranch2",
100+
Files: []*files_service.ChangeRepoFile{
101+
{
102+
Operation: "update",
103+
TreePath: "README.md",
104+
ContentReader: strings.NewReader("# This is a new project2\n"),
105+
},
106+
},
107+
})
108+
assert.NoError(t, err)
109+
110+
// Create a pull request.
111+
session := loginUser(t, "user2")
112+
testPullCreate(t, session, "user2", "test_codeowner", false, repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2")
113+
114+
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"})
115+
unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
116+
})
117+
118+
t.Run("Forked Repo Pull Request", func(t *testing.T) {
119+
user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
120+
forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{
121+
BaseRepo: repo,
122+
Name: "test_codeowner_fork",
123+
})
124+
assert.NoError(t, err)
125+
126+
// create a new branch to prepare for pull request
127+
_, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{
128+
NewBranch: "codeowner-basebranch-forked",
129+
Files: []*files_service.ChangeRepoFile{
130+
{
131+
Operation: "update",
132+
TreePath: "README.md",
133+
ContentReader: strings.NewReader("# This is a new forked project\n"),
134+
},
135+
},
136+
})
137+
assert.NoError(t, err)
138+
139+
session := loginUser(t, "user5")
140+
testPullCreate(t, session, "user5", "test_codeowner_fork", false, forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2")
141+
142+
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"})
143+
unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
144+
})
145+
})
146+
}

0 commit comments

Comments
 (0)