]> source.dussan.org Git - gitea.git/commitdiff
Fix codeowner detected diff base branch to mergebase (#29783)
authorLunny Xiao <xiaolunwen@gmail.com>
Fri, 15 Mar 2024 05:13:09 +0000 (13:13 +0800)
committerGitHub <noreply@github.com>
Fri, 15 Mar 2024 05:13:09 +0000 (05:13 +0000)
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>
models/issues/pull.go
services/issue/issue.go
services/issue/pull.go [new file with mode: 0644]
services/pull/pull.go
tests/integration/pull_review_test.go

index 80b149da5c79079047d9ecae3fd0210624ab68dc..dc1b1b956aab887b6cb67fd53c76f498b80145fe 100644 (file)
@@ -19,7 +19,6 @@ import (
        repo_model "code.gitea.io/gitea/models/repo"
        user_model "code.gitea.io/gitea/models/user"
        "code.gitea.io/gitea/modules/git"
-       "code.gitea.io/gitea/modules/gitrepo"
        "code.gitea.io/gitea/modules/log"
        "code.gitea.io/gitea/modules/setting"
        "code.gitea.io/gitea/modules/timeutil"
@@ -884,77 +883,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(ctx) {
-               return nil
-       }
-
-       if err := pr.LoadBaseRepo(ctx); err != nil {
-               return err
-       }
-
-       repo, err := gitrepo.OpenRepository(ctx, pr.BaseRepo)
-       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, _ := 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
index 0753813b646f95677ae0b4629a150759564015ad..7662c9d9e8c87c3c932ba654d3ab39311f177036 100644 (file)
@@ -90,7 +90,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
                }
        }
diff --git a/services/issue/pull.go b/services/issue/pull.go
new file mode 100644 (file)
index 0000000..698e262
--- /dev/null
@@ -0,0 +1,122 @@
+// 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/gitrepo"
+       "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 := gitrepo.OpenRepository(ctx, pr.BaseRepo)
+       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
+}
index 9133a72acf748911439989588a6053bba91c8eca..57873b63ba6bf8df7780cb741335a2ad0e6803f8 100644 (file)
@@ -136,7 +136,7 @@ func NewPullRequest(ctx context.Context, repo *repo_model.Repository, issue *iss
                }
 
                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
                        }
                }
index 68d80a102157d9118813ebddd8d240598078cbd9..7166a740ab148fc3e689bb11b2b6792ada75124c 100644 (file)
@@ -5,9 +5,20 @@ 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"
+       "code.gitea.io/gitea/modules/git"
+       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 +31,116 @@ 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,
+                       ObjectFormatName: git.Sha1ObjectFormat.Name(),
+                       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")
+                       testPullCreate(t, session, "user2", "test_codeowner", false, 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")
+                       testPullCreate(t, session, "user2", "test_codeowner", false, 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_fork",
+                       })
+                       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")
+                       testPullCreate(t, session, "user5", "test_codeowner_fork", false, forkedRepo.DefaultBranch, "codeowner-basebranch-forked", "Test Pull Request2")
+
+                       pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{BaseRepoID: repo.ID, HeadBranch: "codeowner-basebranch-forked"})
+                       unittest.AssertExistsIf(t, false, &issues_model.Review{IssueID: pr.IssueID, Type: issues_model.ReviewTypeRequest, ReviewerID: 8})
+               })
+       })
+}