Fix codeowner detected diff base branch to mergebase (#29783) (#29807)

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 <wxiaoguang@gmail.com>

models/issues/pull.go

@@ -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 {
+func (pr *PullRequest) IsWorkInProgress(ctx context.Context) bool {
-	if err := pr.LoadIssue(db.DefaultContext); err != nil {
+	if err := pr.LoadIssue(ctx); err != nil {
 		log.Error("LoadIssue: %v", err)
 		return false
 	}
@@ -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.
@@ -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

models/issues/pull_test.go

@@ -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) {

routers/web/repo/issue.go

@@ -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 {

routers/web/repo/pull.go

@@ -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)
 	}

services/convert/pull.go

@@ -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(),

services/issue/issue.go

@@ -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
 		}
 	}

services/issue/pull.go

@@ -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
+}

services/mailer/mail_issue.go

@@ -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)

services/mailer/notify.go

@@ -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)
 		}

services/pull/check.go

@@ -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
 		}
 

services/pull/pull.go

@@ -126,8 +126,8 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
 			return err
 		}
 
-		if !pr.IsWorkInProgress() {
+		if !pr.IsWorkInProgress(ctx) {
-			if err := issues_model.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
+			if err := issue_service.PullRequestCodeOwnersReview(ctx, issue, pr); err != nil {
 				return err
 			}
 		}

services/uinotification/notify.go

@@ -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,

templates/shared/issueicon.tmpl

@@ -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"}}

tests/integration/pull_create_test.go

@@ -4,6 +4,7 @@
 package integration
 
 import (
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -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)

tests/integration/pull_merge_test.go

@@ -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))
 	})
 }

tests/integration/pull_review_test.go

@@ -5,9 +5,19 @@ package integration
 
 import (
 	"net/http"
+	"net/url"
+	"strings"
 	"testing"
 
+	"code.gitea.io/gitea/models/db"
+	issues_model "code.gitea.io/gitea/models/issues"
+	"code.gitea.io/gitea/models/unittest"
+	user_model "code.gitea.io/gitea/models/user"
+	repo_service "code.gitea.io/gitea/services/repository"
+	files_service "code.gitea.io/gitea/services/repository/files"
 	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
 )
 
 func TestPullView_ReviewerMissed(t *testing.T) {
@@ -20,3 +30,115 @@ func TestPullView_ReviewerMissed(t *testing.T) {
 	req = NewRequest(t, "GET", "/user2/repo1/pulls/3")
 	session.MakeRequest(t, req, http.StatusOK)
 }
+
+func TestPullView_CodeOwner(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+
+		// Create the repo.
+		repo, err := repo_service.CreateRepositoryDirectly(db.DefaultContext, user2, user2, repo_service.CreateRepoOptions{
+			Name:          "test_codeowner",
+			Readme:        "Default",
+			AutoInit:      true,
+			DefaultBranch: "master",
+		})
+		assert.NoError(t, err)
+
+		// add CODEOWNERS to default branch
+		_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
+			OldBranch: repo.DefaultBranch,
+			Files: []*files_service.ChangeRepoFile{
+				{
+					Operation:     "create",
+					TreePath:      "CODEOWNERS",
+					ContentReader: strings.NewReader("README.md @user5\n"),
+				},
+			},
+		})
+		assert.NoError(t, err)
+
+		t.Run("First Pull Request", func(t *testing.T) {
+			// create a new branch to prepare for pull request
+			_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
+				NewBranch: "codeowner-basebranch",
+				Files: []*files_service.ChangeRepoFile{
+					{
+						Operation:     "update",
+						TreePath:      "README.md",
+						ContentReader: strings.NewReader("# This is a new project\n"),
+					},
+				},
+			})
+			assert.NoError(t, err)
+
+			// Create a pull request.
+			session := loginUser(t, "user2")
+			createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch", "Test Pull Request")
+
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch"})
+			unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 5})
+		})
+
+		// change the default branch CODEOWNERS file to change README.md's codeowner
+		_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
+			Files: []*files_service.ChangeRepoFile{
+				{
+					Operation:     "update",
+					TreePath:      "CODEOWNERS",
+					ContentReader: strings.NewReader("README.md @user8\n"),
+				},
+			},
+		})
+		assert.NoError(t, err)
+
+		t.Run("Second Pull Request", func(t *testing.T) {
+			// create a new branch to prepare for pull request
+			_, err = files_service.ChangeRepoFiles(db.DefaultContext, repo, user2, &files_service.ChangeRepoFilesOptions{
+				NewBranch: "codeowner-basebranch2",
+				Files: []*files_service.ChangeRepoFile{
+					{
+						Operation:     "update",
+						TreePath:      "README.md",
+						ContentReader: strings.NewReader("# This is a new project2\n"),
+					},
+				},
+			})
+			assert.NoError(t, err)
+
+			// Create a pull request.
+			session := loginUser(t, "user2")
+			createPullRequest(t, session, "user2", "test_codeowner", repo.DefaultBranch, "codeowner-basebranch2", "Test Pull Request2")
+
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch2"})
+			unittest.AssertExistsIf(t, true, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
+		})
+
+		t.Run("Forked Repo Pull Request", func(t *testing.T) {
+			user5 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 5})
+			forkedRepo, err := repo_service.ForkRepository(db.DefaultContext, user2, user5, repo_service.ForkRepoOptions{
+				BaseRepo: repo,
+				Name:     "test_codeowner",
+			})
+			assert.NoError(t, err)
+
+			// create a new branch to prepare for pull request
+			_, err = files_service.ChangeRepoFiles(db.DefaultContext, forkedRepo, user5, &files_service.ChangeRepoFilesOptions{
+				NewBranch: "codeowner-basebranch-forked",
+				Files: []*files_service.ChangeRepoFile{
+					{
+						Operation:     "update",
+						TreePath:      "README.md",
+						ContentReader: strings.NewReader("# This is a new forked project\n"),
+					},
+				},
+			})
+			assert.NoError(t, err)
+
+			session := loginUser(t, "user5")
+			createPullRequest(t, session, "user5", "test_codeowner", forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2")
+
+			pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: forkedRepo.ID, HeadBranch: "codeowner-basebranch-forked"})
+			unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
+		})
+	})
+}