Skip to content

Commit

Permalink
Fix codeowner detected diff base branch to mergebase (#29783) (#29807)
Browse files Browse the repository at this point in the history
Fix #29763
Backport #29783 

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 <[email protected]>
  • Loading branch information
lunny and wxiaoguang authored Mar 17, 2024
1 parent 8242c3c commit 85f31eb
Show file tree
Hide file tree
Showing 16 changed files with 281 additions and 96 deletions.
84 changes: 4 additions & 80 deletions models/issues/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -706,8 +706,8 @@ func (pr *PullRequest) UpdateColsIfNotMerged(ctx context.Context, cols ...string

// IsWorkInProgress determine if the Pull Request is a Work In Progress by its title
// Issue must be set before this method can be called.
func (pr *PullRequest) IsWorkInProgress() bool {
if err := pr.LoadIssue(db.DefaultContext); err != nil {
func (pr *PullRequest) IsWorkInProgress(ctx context.Context) bool {
if err := pr.LoadIssue(ctx); err != nil {
log.Error("LoadIssue: %v", err)
return false
}
Expand Down Expand Up @@ -810,14 +810,14 @@ func UpdateAllowEdits(ctx context.Context, pr *PullRequest) error {
}

// Mergeable returns if the pullrequest is mergeable.
func (pr *PullRequest) Mergeable() bool {
func (pr *PullRequest) Mergeable(ctx context.Context) bool {
// If a pull request isn't mergable if it's:
// - Being conflict checked.
// - Has a conflict.
// - Received a error while being conflict checked.
// - Is a work-in-progress pull request.
return pr.Status != PullRequestStatusChecking && pr.Status != PullRequestStatusConflict &&
pr.Status != PullRequestStatusError && !pr.IsWorkInProgress()
pr.Status != PullRequestStatusError && !pr.IsWorkInProgress(ctx)
}

// HasEnoughApprovals returns true if pr has enough granted approvals.
Expand Down Expand Up @@ -887,82 +887,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr *
return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0
}

func PullRequestCodeOwnersReview(ctx context.Context, pull *Issue, pr *PullRequest) error {
files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}

if pr.IsWorkInProgress() {
return nil
}

if err := pr.LoadBaseRepo(ctx); err != nil {
return err
}

repo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
if err != nil {
return err
}
defer repo.Close()

branch, err := repo.GetDefaultBranch()
if err != nil {
return err
}

commit, err := repo.GetBranchCommit(branch)
if err != nil {
return err
}

var data string
for _, file := range files {
if blob, err := commit.GetBlobByPath(file); err == nil {
data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
if err == nil {
break
}
}
}

rules, _ := GetCodeOwnersFromContent(ctx, data)
changedFiles, err := repo.GetFilesChangedBetween(git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
if err != nil {
return err
}

uniqUsers := make(map[int64]*user_model.User)
uniqTeams := make(map[string]*org_model.Team)
for _, rule := range rules {
for _, f := range changedFiles {
if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) {
for _, u := range rule.Users {
uniqUsers[u.ID] = u
}
for _, t := range rule.Teams {
uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t
}
}
}
}

for _, u := range uniqUsers {
if u.ID != pull.Poster.ID {
if _, err := AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {
log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
return err
}
}
}
for _, t := range uniqTeams {
if _, err := AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {
log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
return err
}
}

return nil
}

// GetCodeOwnersFromContent returns the code owners configuration
// Return empty slice if files missing
// Return warning messages on parsing errors
Expand Down
6 changes: 3 additions & 3 deletions models/issues/pull_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,13 +260,13 @@ func TestPullRequest_IsWorkInProgress(t *testing.T) {
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{ID: 2})
pr.LoadIssue(db.DefaultContext)

assert.False(t, pr.IsWorkInProgress())
assert.False(t, pr.IsWorkInProgress(db.DefaultContext))

pr.Issue.Title = "WIP: " + pr.Issue.Title
assert.True(t, pr.IsWorkInProgress())
assert.True(t, pr.IsWorkInProgress(db.DefaultContext))

pr.Issue.Title = "[wip]: " + pr.Issue.Title
assert.True(t, pr.IsWorkInProgress())
assert.True(t, pr.IsWorkInProgress(db.DefaultContext))
}

func TestPullRequest_GetWorkInProgressPrefixWorkInProgress(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -1910,7 +1910,7 @@ func ViewIssue(ctx *context.Context) {
if pull.HasMerged || issue.IsClosed || !ctx.IsSigned {
return false
}
if pull.CanAutoMerge() || pull.IsWorkInProgress() || pull.IsChecking() {
if pull.CanAutoMerge() || pull.IsWorkInProgress(ctx) || pull.IsChecking() {
return false
}
if (ctx.Doer.IsAdmin || ctx.Repo.IsAdmin()) && prConfig.AllowManualMerge {
Expand Down
2 changes: 1 addition & 1 deletion routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -706,7 +706,7 @@ func PrepareViewPullInfo(ctx *context.Context, issue *issues_model.Issue) *git.C
ctx.Data["IsNothingToCompare"] = true
}

if pull.IsWorkInProgress() {
if pull.IsWorkInProgress(ctx) {
ctx.Data["IsPullWorkInProgress"] = true
ctx.Data["WorkInProgressPrefix"] = pull.GetWorkInProgressPrefix(ctx)
}
Expand Down
2 changes: 1 addition & 1 deletion services/convert/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func ToAPIPullRequest(ctx context.Context, pr *issues_model.PullRequest, doer *u
PatchURL: pr.Issue.PatchURL(),
HasMerged: pr.HasMerged,
MergeBase: pr.MergeBase,
Mergeable: pr.Mergeable(),
Mergeable: pr.Mergeable(ctx),
Deadline: apiIssue.Deadline,
Created: pr.Issue.CreatedUnix.AsTimePtr(),
Updated: pr.Issue.UpdatedUnix.AsTimePtr(),
Expand Down
2 changes: 1 addition & 1 deletion services/issue/issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ func ChangeTitle(ctx context.Context, issue *issues_model.Issue, doer *user_mode
}

if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issues_model.HasWorkInProgressPrefix(title) {
if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil {
if err := PullRequestCodeOwnersReview(ctx, issue, issue.PullRequest); err != nil {
return err
}
}
Expand Down
121 changes: 121 additions & 0 deletions services/issue/pull.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
// Copyright 2024 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT

package issue

import (
"context"
"fmt"
"time"

issues_model "code.gitea.io/gitea/models/issues"
org_model "code.gitea.io/gitea/models/organization"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
)

func getMergeBase(repo *git.Repository, pr *issues_model.PullRequest, baseBranch, headBranch string) (string, error) {
// Add a temporary remote
tmpRemote := fmt.Sprintf("mergebase-%d-%d", pr.ID, time.Now().UnixNano())
if err := repo.AddRemote(tmpRemote, repo.Path, false); err != nil {
return "", fmt.Errorf("AddRemote: %w", err)
}
defer func() {
if err := repo.RemoveRemote(tmpRemote); err != nil {
log.Error("getMergeBase: RemoveRemote: %v", err)
}
}()

mergeBase, _, err := repo.GetMergeBase(tmpRemote, baseBranch, headBranch)
return mergeBase, err
}

func PullRequestCodeOwnersReview(ctx context.Context, pull *issues_model.Issue, pr *issues_model.PullRequest) error {
files := []string{"CODEOWNERS", "docs/CODEOWNERS", ".gitea/CODEOWNERS"}

if pr.IsWorkInProgress(ctx) {
return nil
}

if err := pr.LoadHeadRepo(ctx); err != nil {
return err
}

if pr.HeadRepo.IsFork {
return nil
}

if err := pr.LoadBaseRepo(ctx); err != nil {
return err
}

repo, err := git.OpenRepository(ctx, pr.BaseRepo.RepoPath())
if err != nil {
return err
}
defer repo.Close()

commit, err := repo.GetBranchCommit(pr.BaseRepo.DefaultBranch)
if err != nil {
return err
}

var data string
for _, file := range files {
if blob, err := commit.GetBlobByPath(file); err == nil {
data, err = blob.GetBlobContent(setting.UI.MaxDisplayFileSize)
if err == nil {
break
}
}
}

rules, _ := issues_model.GetCodeOwnersFromContent(ctx, data)

// get the mergebase
mergeBase, err := getMergeBase(repo, pr, git.BranchPrefix+pr.BaseBranch, pr.GetGitRefName())
if err != nil {
return err
}

// https://github.com/go-gitea/gitea/issues/29763, we need to get the files changed
// between the merge base and the head commit but not the base branch and the head commit
changedFiles, err := repo.GetFilesChangedBetween(mergeBase, pr.HeadCommitID)
if err != nil {
return err
}

uniqUsers := make(map[int64]*user_model.User)
uniqTeams := make(map[string]*org_model.Team)
for _, rule := range rules {
for _, f := range changedFiles {
if (rule.Rule.MatchString(f) && !rule.Negative) || (!rule.Rule.MatchString(f) && rule.Negative) {
for _, u := range rule.Users {
uniqUsers[u.ID] = u
}
for _, t := range rule.Teams {
uniqTeams[fmt.Sprintf("%d/%d", t.OrgID, t.ID)] = t
}
}
}
}

for _, u := range uniqUsers {
if u.ID != pull.Poster.ID {
if _, err := issues_model.AddReviewRequest(ctx, pull, u, pull.Poster); err != nil {
log.Warn("Failed add assignee user: %s to PR review: %s#%d, error: %s", u.Name, pr.BaseRepo.Name, pr.ID, err)
return err
}
}
}
for _, t := range uniqTeams {
if _, err := issues_model.AddTeamReviewRequest(ctx, pull, t, pull.Poster); err != nil {
log.Warn("Failed add assignee team: %s to PR review: %s#%d, error: %s", t.Name, pr.BaseRepo.Name, pr.ID, err)
return err
}
}

return nil
}
2 changes: 1 addition & 1 deletion services/mailer/mail_issue.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*user_mo

// =========== Repo watchers ===========
// Make repo watchers last, since it's likely the list with the most users
if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress() && ctx.ActionType != activities_model.ActionCreatePullRequest) {
if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress(ctx) && ctx.ActionType != activities_model.ActionCreatePullRequest) {
ids, err = repo_model.GetRepoWatchersIDs(ctx, ctx.Issue.RepoID)
if err != nil {
return fmt.Errorf("GetRepoWatchersIDs(%d): %w", ctx.Issue.RepoID, err)
Expand Down
2 changes: 1 addition & 1 deletion services/mailer/notify.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func (m *mailNotifier) IssueChangeTitle(ctx context.Context, doer *user_model.Us
log.Error("issue.LoadPullRequest: %v", err)
return
}
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() {
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress(ctx) {
if err := MailParticipants(ctx, issue, doer, activities_model.ActionPullRequestReadyForReview, nil); err != nil {
log.Error("MailParticipants: %v", err)
}
Expand Down
2 changes: 1 addition & 1 deletion services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
return nil
}

if pr.IsWorkInProgress() {
if pr.IsWorkInProgress(ctx) {
return ErrIsWorkInProgress
}

Expand Down
4 changes: 2 additions & 2 deletions services/pull/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,8 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
return err
}

if !pr.IsWorkInProgress() {
if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
if !pr.IsWorkInProgress(ctx) {
if err := issue_service.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
return err
}
}
Expand Down
2 changes: 1 addition & 1 deletion services/uinotification/notify.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ func (ns *notificationService) IssueChangeTitle(ctx context.Context, doer *user_
log.Error("issue.LoadPullRequest: %v", err)
return
}
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() {
if issue.IsPull && issues_model.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress(ctx) {
_ = ns.issueQueue.Push(issueNotificationOpts{
IssueID: issue.ID,
NotificationAuthorID: doer.ID,
Expand Down
4 changes: 2 additions & 2 deletions templates/shared/issueicon.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
{{if .IsClosed}}
{{svg "octicon-git-pull-request" 16 "text red"}}
{{else}}
{{if and .PullRequest .PullRequest.IsWorkInProgress}}
{{if and .PullRequest .PullRequest.IsWorkInProgress ctx}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else if and .GetPullRequest .GetPullRequest.IsWorkInProgress}}
{{else if and .GetPullRequest .GetPullRequest.IsWorkInProgress ctx}}
{{svg "octicon-git-pull-request-draft" 16 "text grey"}}
{{else}}
{{svg "octicon-git-pull-request" 16 "text green"}}
Expand Down
18 changes: 18 additions & 0 deletions tests/integration/pull_create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package integration

import (
"fmt"
"net/http"
"net/http/httptest"
"net/url"
Expand All @@ -17,6 +18,23 @@ import (
"github.com/stretchr/testify/assert"
)

func createPullRequest(t *testing.T, session *TestSession, user, repo, baseBranch, headBranch, title string) *httptest.ResponseRecorder {
link := fmt.Sprintf("/%s/%s/compare/%s...%s", user, repo, baseBranch, headBranch)
req := NewRequest(t, "GET", link)
resp := session.MakeRequest(t, req, http.StatusOK)

// Submit the form for creating the pull
htmlDoc := NewHTMLParser(t, resp.Body)
link, exists := htmlDoc.doc.Find("form.ui.form").Attr("action")
assert.True(t, exists, "The template has changed")
req = NewRequestWithValues(t, "POST", link, map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"title": title,
})
resp = session.MakeRequest(t, req, http.StatusOK)
return resp
}

func testPullCreate(t *testing.T, session *TestSession, user, repo, branch, title string) *httptest.ResponseRecorder {
req := NewRequest(t, "GET", path.Join(user, repo))
resp := session.MakeRequest(t, req, http.StatusOK)
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/pull_merge_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,6 @@ func TestConflictChecking(t *testing.T) {
// Check if status is correct.
assert.Equal(t, issues_model.PullRequestStatusConflict, conflictingPR.Status)
// Ensure that mergeable returns false
assert.False(t, conflictingPR.Mergeable())
assert.False(t, conflictingPR.Mergeable(db.DefaultContext))
})
}
Loading

0 comments on commit 85f31eb

Please sign in to comment.