enhance(codeowners): support section / glob / file view display / skip review request (#40243)

Document: https://gitea.com/gitea/docs/pulls/634 ([doc PR
preview](955ae7d75c/docs/usage/repository/code-owners.md))

1. If the pattern starts with `*` or contains `/*`, use our glob package
to parse it as glob to regexp
* Make it easier to use. Otherwise it's really ugly to keep writing a
lot of backslash escapes
2. Add GitLab-style "section" support
   * Fix #28926

```codeowners
[Section] @user
*.js
```

3. Add our annotation support, at the moment only need to support
review-request=true/false
    * Fix #40241

```codeowners
## review-request=false # this is global

*.js $user ## review-request=true # this is line-level

[Section] @user ## review-request=true # this is section-level
*.md
```

4. Add GitHub/GitLab-style "show codeowner when viewing a file"
    * Fix #28137
This commit is contained in:
wxiaoguang
2026-11-06 07:19:51 +00:00
committed by GitHub
parent 0d59b737aa
commit fd98ab5472
10 changed files with 600 additions and 208 deletions

256
models/issues/code_owner.go Normal file
View File

@@ -0,0 +1,256 @@
// Copyright 2026 The Gitea Authors. All rights reserved.
// SPDX-License-Identifier: MIT
package issues
import (
"context"
"fmt"
"strings"
"time"
org_model "gitea.dev/models/organization"
user_model "gitea.dev/models/user"
"gitea.dev/modules/glob"
"gitea.dev/modules/optional"
"github.com/dlclark/regexp2/v2"
)
// GetCodeOwnersFromContent returns the code owners configuration
// Return empty slice if files missing
// Return warning messages on parsing errors
// We're trying to do the best we can when parsing a file.
// Invalid lines are skipped. Non-existent users and teams too.
func GetCodeOwnersFromContent(ctx context.Context, data string) (rules []*CodeOwnerRule, warnings []string) {
globalReview := optional.None[bool]()
sectionReview := optional.None[bool]()
section := ""
var owners []string
lineNum := 0
for line := range strings.SplitSeq(data, "\n") {
lineNum++
line, reviewRequest, annotationWarnings := parseCodeOwnerAnnotation(line)
for _, warning := range annotationWarnings {
warnings = append(warnings, fmt.Sprintf("Line: %d: %s", lineNum, warning))
}
line = strings.TrimSpace(line)
if line == "" {
if reviewRequest.Has() {
globalReview = reviewRequest
}
continue
}
// To support section headers, there could be an edge case for end users:
// `[a-z] @user` now becomes a section header instead of a rule, but it is really rare to happen.
// A workaround is to use `/[a-z] @user` to make it a rule.
if heading := line; strings.HasPrefix(heading, "[") {
if end := strings.IndexByte(heading, ']'); end > 1 && (end == len(heading)-1 || heading[end+1] == ' ' || heading[end+1] == '\t') {
section = heading[1:end]
owners = TokenizeCodeOwnersLine(heading[end+1:])
sectionReview = reviewRequest
continue
}
}
tokens := TokenizeCodeOwnersLine(line)
if len(tokens) == 0 {
continue
}
if len(tokens) == 1 && len(owners) > 0 {
tokens = append(tokens, owners...)
}
if len(tokens) < 2 {
warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", lineNum))
continue
}
rule, wr := ParseCodeOwnersLine(ctx, tokens)
for _, w := range wr {
warnings = append(warnings, fmt.Sprintf("Line: %d: %s", lineNum, w))
}
if rule == nil {
continue
}
rule.LineNum = lineNum
rule.Section = section
rule.ReviewRequest = reviewRequest
if !rule.ReviewRequest.Has() {
rule.ReviewRequest = sectionReview
}
if !rule.ReviewRequest.Has() {
rule.ReviewRequest = globalReview
}
rules = append(rules, rule)
}
return rules, warnings
}
// codeOwnerMatchTimeout bounds a single pattern match so a crafted pattern
// cannot stall via catastrophic backtracking. See also the aggregate budget
// enforced by the caller across the whole rules×files match loop.
const codeOwnerMatchTimeout = 150 * time.Millisecond
type CodeOwnerRule struct {
LineNum int
Section string
ReviewRequest optional.Option[bool]
Rule *regexp2.Regexp // it supports negative lookahead, does better for end users
Negative bool
// TODO: these objects should not be loaded here, but rather lazily when needed.
// The parsed rule should only contain the user/team names.
Users []*user_model.User
Teams []*org_model.Team
}
func ParseCodeOwnersLine(ctx context.Context, tokens []string) (rule *CodeOwnerRule, warnings []string) {
rule = &CodeOwnerRule{Negative: strings.HasPrefix(tokens[0], "!")}
// Strip leading "!" for negative rules, then strip leading "/" since
// git returns relative paths (e.g. "docs/foo.md" not "/docs/foo.md")
// and the regex is already anchored with ^...$, so the "/" is redundant.
pattern := strings.TrimPrefix(tokens[0], "!")
pattern = strings.TrimPrefix(pattern, "/")
expr := fmt.Sprintf("^%s$", pattern)
// To make life easier, we support glob expressions if the pattern contains a "*" or "/*" (e.g. "*.md" or "docs/*.md")
// However, "src/test\\..*\\.js" is still the only way because "src/test.*.js" can't be used as a glob at the moment.
// TODO: In the future, maybe it's better to introduce a new annotation to specify the pattern type, e.g.: "pattern-syntax=glob" or "pattern-syntax=regexp"
if strings.HasPrefix(pattern, "*") || strings.Contains(pattern, "/*") {
compiled, compileErr := glob.CompileToRegexp(pattern)
if compileErr != nil {
return nil, []string{fmt.Sprintf("incorrect codeowner glob: %s", compileErr)}
}
expr = compiled.String()
}
var err error
rule.Rule, err = regexp2.Compile(expr, regexp2.None)
if err != nil {
warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err))
return nil, warnings
}
// Bound matching time so user-supplied patterns cannot stall PR creation via catastrophic backtracking.
rule.Rule.MatchTimeout = codeOwnerMatchTimeout
for _, user := range tokens[1:] {
user = strings.TrimPrefix(user, "@")
// Only @org/team can contain slashes
if strings.Contains(user, "/") {
s := strings.Split(user, "/")
if len(s) != 2 {
warnings = append(warnings, "incorrect codeowner group: "+user)
continue
}
orgName := s[0]
teamName := s[1]
org, err := org_model.GetOrgByName(ctx, orgName)
if err != nil {
warnings = append(warnings, "incorrect codeowner organization: "+user)
continue
}
teams, err := org.LoadTeams(ctx)
if err != nil {
warnings = append(warnings, "incorrect codeowner team: "+user)
continue
}
for _, team := range teams {
if team.Name == teamName {
rule.Teams = append(rule.Teams, team)
}
}
} else {
u, err := user_model.GetUserByName(ctx, user)
if err != nil {
warnings = append(warnings, "incorrect codeowner user: "+user)
continue
}
rule.Users = append(rule.Users, u)
}
}
if len(rule.Users) == 0 && len(rule.Teams) == 0 {
warnings = append(warnings, "no users/groups matched")
return nil, warnings
}
return rule, warnings
}
func TokenizeCodeOwnersLine(line string) (tokens []string) {
line = strings.TrimSpace(line)
line = strings.ReplaceAll(line, "\t", " ")
if line == "" {
return nil
}
escape := false
var token strings.Builder
for _, char := range line {
if escape {
token.WriteRune(char)
escape = false
} else if char == '\\' {
escape = true
} else if char == '#' {
break
} else if char == ' ' {
if token.Len() > 0 {
tokens = append(tokens, token.String())
token.Reset()
}
} else {
token.WriteRune(char)
}
}
if token.Len() > 0 {
tokens = append(tokens, token.String())
}
return tokens
}
func parseCodeOwnerAnnotation(line string) (string, optional.Option[bool], []string) {
escape := false
for i := 0; i < len(line); i++ {
if escape {
escape = false
continue
}
if line[i] == '\\' {
escape = true
continue
}
if line[i] != '#' {
continue
}
review := optional.None[bool]()
var warnings []string
if strings.HasPrefix(line[i:], "##") {
annotation, _, _ := strings.Cut(line[i+2:], "#")
for field := range strings.FieldsSeq(annotation) {
key, value, ok := strings.Cut(field, "=")
if key != "review-request" {
warnings = append(warnings, "unknown codeowner annotation: "+key)
continue
}
if !ok || (value != "true" && value != "false") {
warnings = append(warnings, "incorrect review-request annotation: "+field)
continue
}
review = optional.Some(value == "true")
}
}
return line[:i], review, warnings
}
return line, optional.None[bool](), nil
}

View File

@@ -10,7 +10,6 @@ import (
"fmt" "fmt"
"io" "io"
"strings" "strings"
"time"
"gitea.dev/models/db" "gitea.dev/models/db"
git_model "gitea.dev/models/git" git_model "gitea.dev/models/git"
@@ -24,7 +23,6 @@ import (
"gitea.dev/modules/timeutil" "gitea.dev/modules/timeutil"
"gitea.dev/modules/util" "gitea.dev/modules/util"
"github.com/dlclark/regexp2/v2"
"xorm.io/builder" "xorm.io/builder"
) )
@@ -825,162 +823,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr *
return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0 return protectBranch.BlockOnOutdatedBranch && pr.CommitsBehind > 0
} }
// GetCodeOwnersFromContent returns the code owners configuration
// Return empty slice if files missing
// Return warning messages on parsing errors
// We're trying to do the best we can when parsing a file.
// Invalid lines are skipped. Non-existent users and teams too.
func GetCodeOwnersFromContent(ctx context.Context, data string) ([]*CodeOwnerRule, []string) {
if len(data) == 0 {
return nil, nil
}
rules := make([]*CodeOwnerRule, 0)
lines := strings.Split(data, "\n")
warnings := make([]string, 0)
for i, line := range lines {
tokens := TokenizeCodeOwnersLine(line)
if len(tokens) == 0 {
continue
} else if len(tokens) < 2 {
warnings = append(warnings, fmt.Sprintf("Line: %d: incorrect format", i+1))
continue
}
rule, wr := ParseCodeOwnersLine(ctx, tokens)
for _, w := range wr {
warnings = append(warnings, fmt.Sprintf("Line: %d: %s", i+1, w))
}
if rule == nil {
continue
}
rules = append(rules, rule)
}
return rules, warnings
}
// codeOwnerMatchTimeout bounds a single pattern match so a crafted pattern
// cannot stall via catastrophic backtracking. See also the aggregate budget
// enforced by the caller across the whole rules×files match loop.
const codeOwnerMatchTimeout = 150 * time.Millisecond
type CodeOwnerRule struct {
Rule *regexp2.Regexp // it supports negative lookahead, does better for end users
Negative bool
Users []*user_model.User
Teams []*org_model.Team
}
func ParseCodeOwnersLine(ctx context.Context, tokens []string) (*CodeOwnerRule, []string) {
var err error
rule := &CodeOwnerRule{
Users: make([]*user_model.User, 0),
Teams: make([]*org_model.Team, 0),
Negative: strings.HasPrefix(tokens[0], "!"),
}
warnings := make([]string, 0)
// Strip leading "!" for negative rules, then strip leading "/" since
// git returns relative paths (e.g. "docs/foo.md" not "/docs/foo.md")
// and the regex is already anchored with ^...$, so the "/" is redundant.
pattern := strings.TrimPrefix(tokens[0], "!")
pattern = strings.TrimPrefix(pattern, "/")
expr := fmt.Sprintf("^%s$", pattern)
rule.Rule, err = regexp2.Compile(expr, regexp2.None)
if err != nil {
warnings = append(warnings, fmt.Sprintf("incorrect codeowner regexp: %s", err))
return nil, warnings
}
// Bound matching time so user-supplied patterns cannot stall PR creation via catastrophic backtracking.
rule.Rule.MatchTimeout = codeOwnerMatchTimeout
for _, user := range tokens[1:] {
user = strings.TrimPrefix(user, "@")
// Only @org/team can contain slashes
if strings.Contains(user, "/") {
s := strings.Split(user, "/")
if len(s) != 2 {
warnings = append(warnings, "incorrect codeowner group: "+user)
continue
}
orgName := s[0]
teamName := s[1]
org, err := org_model.GetOrgByName(ctx, orgName)
if err != nil {
warnings = append(warnings, "incorrect codeowner organization: "+user)
continue
}
teams, err := org.LoadTeams(ctx)
if err != nil {
warnings = append(warnings, "incorrect codeowner team: "+user)
continue
}
for _, team := range teams {
if team.Name == teamName {
rule.Teams = append(rule.Teams, team)
}
}
} else {
u, err := user_model.GetUserByName(ctx, user)
if err != nil {
warnings = append(warnings, "incorrect codeowner user: "+user)
continue
}
rule.Users = append(rule.Users, u)
}
}
if (len(rule.Users) == 0) && (len(rule.Teams) == 0) {
warnings = append(warnings, "no users/groups matched")
return nil, warnings
}
return rule, warnings
}
func TokenizeCodeOwnersLine(line string) []string {
if len(line) == 0 {
return nil
}
line = strings.TrimSpace(line)
line = strings.ReplaceAll(line, "\t", " ")
tokens := make([]string, 0)
escape := false
token := ""
for _, char := range line {
if escape {
token += string(char)
escape = false
} else if string(char) == "\\" {
escape = true
} else if string(char) == "#" {
break
} else if string(char) == " " {
if len(token) > 0 {
tokens = append(tokens, token)
token = ""
}
} else {
token += string(char)
}
}
if len(token) > 0 {
tokens = append(tokens, token)
}
return tokens
}
// InsertPullRequests inserted pull requests // InsertPullRequests inserted pull requests
func InsertPullRequests(ctx context.Context, prs ...*PullRequest) error { func InsertPullRequests(ctx context.Context, prs ...*PullRequest) error {
return db.WithTx(ctx, func(ctx context.Context) error { return db.WithTx(ctx, func(ctx context.Context) error {

View File

@@ -44,6 +44,7 @@ func TestPullRequest(t *testing.T) {
t.Run("ParseCodeOwnersLine", testParseCodeOwnersLine) t.Run("ParseCodeOwnersLine", testParseCodeOwnersLine)
t.Run("CodeOwnerAbsolutePathPatterns", testCodeOwnerAbsolutePathPatterns) t.Run("CodeOwnerAbsolutePathPatterns", testCodeOwnerAbsolutePathPatterns)
t.Run("CodeOwnerPatternMatchTimeout", testCodeOwnerPatternMatchTimeout) t.Run("CodeOwnerPatternMatchTimeout", testCodeOwnerPatternMatchTimeout)
t.Run("CodeOwnerSectionsAndAnnotations", testCodeOwnerSectionsAndAnnotations)
t.Run("GetApprovers", testGetApprovers) t.Run("GetApprovers", testGetApprovers)
t.Run("GetPullRequestByMergedCommit", testGetPullRequestByMergedCommit) t.Run("GetPullRequestByMergedCommit", testGetPullRequestByMergedCommit)
t.Run("Migrate_InsertPullRequests", testMigrateInsertPullRequests) t.Run("Migrate_InsertPullRequests", testMigrateInsertPullRequests)
@@ -335,7 +336,7 @@ func testParseCodeOwnersLine(t *testing.T) {
given := []CodeOwnerTest{ given := []CodeOwnerTest{
{Line: "", Tokens: nil}, {Line: "", Tokens: nil},
{Line: "# comment", Tokens: []string{}}, {Line: "# comment", Tokens: nil},
{Line: "!.* @user1 @org1/team1", Tokens: []string{"!.*", "@user1", "@org1/team1"}}, {Line: "!.* @user1 @org1/team1", Tokens: []string{"!.*", "@user1", "@org1/team1"}},
{Line: `.*\\.js @user2 #comment`, Tokens: []string{`.*\.js`, "@user2"}}, {Line: `.*\\.js @user2 #comment`, Tokens: []string{`.*\.js`, "@user2"}},
{Line: `docs/(aws|google|azure)/[^/]*\\.(md|txt) @org3 @org2/team2`, Tokens: []string{`docs/(aws|google|azure)/[^/]*\.(md|txt)`, "@org3", "@org2/team2"}}, {Line: `docs/(aws|google|azure)/[^/]*\\.(md|txt) @org3 @org2/team2`, Tokens: []string{`docs/(aws|google|azure)/[^/]*\.(md|txt)`, "@org3", "@org2/team2"}},
@@ -347,7 +348,7 @@ func testParseCodeOwnersLine(t *testing.T) {
for _, g := range given { for _, g := range given {
tokens := issues_model.TokenizeCodeOwnersLine(g.Line) tokens := issues_model.TokenizeCodeOwnersLine(g.Line)
assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed") assert.Equal(t, g.Tokens, tokens, "Codeowners tokenizer failed: input=%s", g.Line)
} }
} }
@@ -448,3 +449,49 @@ func testMigrateInsertPullRequests(t *testing.T) {
unittest.CheckConsistencyFor(t, &issues_model.Issue{}, &issues_model.PullRequest{}) unittest.CheckConsistencyFor(t, &issues_model.Issue{}, &issues_model.PullRequest{})
} }
func testCodeOwnerSectionsAndAnnotations(t *testing.T) {
rules, warnings := issues_model.GetCodeOwnersFromContent(t.Context(), "*.js @user5\n[Docs] @user1\n*.md\n")
require.Empty(t, warnings)
require.Len(t, rules, 2)
for _, rule := range rules {
assert.False(t, rule.ReviewRequest.Has())
}
rules, warnings = issues_model.GetCodeOwnersFromContent(t.Context(), `## review-request=false # global
*.js @user5 ## review-request=true # line
[Documentation] @user1 ## review-request=true # section
*.md
/docs/* @user5 ## review-request=false
[Other Section] @user5
*.txt
`)
match := func(rule *issues_model.CodeOwnerRule, path string) bool {
v, _ := rule.Rule.MatchString(path)
return v
}
require.Empty(t, warnings)
require.Len(t, rules, 4)
assert.True(t, rules[0].ReviewRequest.Value())
assert.True(t, match(rules[0], "src/app.js"))
assert.False(t, match(rules[0], "appXjs"))
assert.Equal(t, "Documentation", rules[1].Section)
assert.Equal(t, "user1", rules[1].Users[0].Name)
assert.True(t, rules[1].ReviewRequest.Value())
assert.Equal(t, "user5", rules[2].Users[0].Name)
assert.False(t, rules[2].ReviewRequest.Value())
assert.True(t, match(rules[2], "docs/guide.md"))
assert.False(t, match(rules[2], "other/docs/guide.md"))
assert.False(t, rules[3].ReviewRequest.Value())
rules, warnings = issues_model.GetCodeOwnersFromContent(t.Context(), "*.js @user5 ## review-request=invalid\n*[ @user5\n")
require.Len(t, rules, 1)
assert.False(t, rules[0].ReviewRequest.Has())
require.Len(t, warnings, 2)
rules, warnings = issues_model.GetCodeOwnersFromContent(t.Context(), `file\#name @user5 ## review-request=false`)
require.Empty(t, warnings)
require.Len(t, rules, 1)
assert.True(t, match(rules[0], "file#name"))
assert.False(t, rules[0].ReviewRequest.Value())
}

View File

@@ -196,6 +196,14 @@ func Compile(pattern string, separators ...rune) (Glob, error) {
return initGlobCompiler(&globCompiler{}, pattern, separators) return initGlobCompiler(&globCompiler{}, pattern, separators)
} }
func CompileToRegexp(pattern string, separators ...rune) (*regexp.Regexp, error) {
compiler := &globCompiler{}
if _, err := initGlobCompiler(compiler, pattern, separators); err != nil {
return nil, err
}
return compiler.regexp, nil
}
func CompileWorkflow(pattern string) (Glob, error) { func CompileWorkflow(pattern string) (Glob, error) {
return initGlobCompiler(&globCompiler{ return initGlobCompiler(&globCompiler{
regexpQuestion: true, regexpQuestion: true,

View File

@@ -1731,6 +1731,7 @@
"repo.issues.review.content.empty": "You need to leave a comment indicating the requested change(s).", "repo.issues.review.content.empty": "You need to leave a comment indicating the requested change(s).",
"repo.issues.review.reject": "requested changes %s", "repo.issues.review.reject": "requested changes %s",
"repo.issues.review.wait": "was requested for review %s", "repo.issues.review.wait": "was requested for review %s",
"repo.codeowners": "Code owners",
"repo.issues.review.codeowners_rules": "CODEOWNERS rules", "repo.issues.review.codeowners_rules": "CODEOWNERS rules",
"repo.issues.review.add_review_request": "requested review from %s %s", "repo.issues.review.add_review_request": "requested review from %s %s",
"repo.issues.review.remove_review_request": "removed review request for %s %s", "repo.issues.review.remove_review_request": "removed review request for %s %s",

View File

@@ -9,10 +9,12 @@ import (
"image" "image"
"io" "io"
"path" "path"
"slices"
"strings" "strings"
git_model "gitea.dev/models/git" git_model "gitea.dev/models/git"
issue_model "gitea.dev/models/issues" issue_model "gitea.dev/models/issues"
org_model "gitea.dev/models/organization"
"gitea.dev/models/renderhelper" "gitea.dev/models/renderhelper"
user_model "gitea.dev/models/user" user_model "gitea.dev/models/user"
"gitea.dev/modules/actions" "gitea.dev/modules/actions"
@@ -153,9 +155,53 @@ func handleFileViewRenderImage(ctx *context.Context, fInfo *fileInfo, prefetchBu
return true return true
} }
type fileCodeOwner struct {
CodeOwnerName string
CodeOwnerLink string
}
func prepareFileCodeOwners(ctx *context.Context) {
rules, warnings1, err := issue_service.GetCodeOwnerRules(ctx, ctx.Repo.GitRepo, ctx.Repo.Commit)
if msg, _ := util.ErrorUnwrapForUser(err); msg != "" {
ctx.Flash.Error(msg, true)
return
} else if err != nil {
log.Error("GetCodeOwnerRules: failed to read CODEOWNERS file at repo %s commit %s: %v", ctx.Repo.Repository.LogString(), ctx.Repo.CommitID, err)
return
}
matching, warnings2 := issue_service.MatchCodeOwnerRules(rules, []string{ctx.Repo.TreePath})
warnings := slices.Concat(warnings1, warnings2)
if len(warnings) > 0 {
ctx.Flash.Warning(strings.Join(warnings, "\n"), true)
}
var owners []fileCodeOwner
seen := make(map[string]bool)
add := func(name, link string) {
if !seen[link] {
owners = append(owners, fileCodeOwner{name, link})
seen[link] = true
}
}
for _, rule := range matching {
for _, user := range rule.Users {
add("@"+user.Name, user.HomeLink())
}
for _, team := range rule.Teams {
org, err := org_model.GetOrgByID(ctx, team.OrgID)
if err != nil {
continue
}
add("@"+org.Name+"/"+team.Name, org.HomeLink()+"/teams/"+util.PathEscapeSegments(team.LowerName))
}
}
ctx.Data["FileCodeOwners"] = owners
}
func prepareFileView(ctx *context.Context, entry *git.TreeEntry) { func prepareFileView(ctx *context.Context, entry *git.TreeEntry) {
ctx.Data["IsViewFile"] = true ctx.Data["IsViewFile"] = true
ctx.Data["HideRepoInfo"] = true ctx.Data["HideRepoInfo"] = true
prepareFileCodeOwners(ctx)
if !prepareLatestCommitInfo(ctx) { if !prepareLatestCommitInfo(ctx) {
return return

View File

@@ -5,20 +5,24 @@ package issue
import ( import (
"context" "context"
"errors"
"fmt" "fmt"
"slices" "slices"
"sync"
"time" "time"
"gitea.dev/models/db" "gitea.dev/models/db"
git_model "gitea.dev/models/git" git_model "gitea.dev/models/git"
issues_model "gitea.dev/models/issues" issues_model "gitea.dev/models/issues"
org_model "gitea.dev/models/organization" org_model "gitea.dev/models/organization"
repo_model "gitea.dev/models/repo"
user_model "gitea.dev/models/user" user_model "gitea.dev/models/user"
"gitea.dev/modules/git" "gitea.dev/modules/git"
"gitea.dev/modules/log" "gitea.dev/modules/log"
"gitea.dev/modules/optional" "gitea.dev/modules/optional"
"gitea.dev/modules/setting" "gitea.dev/modules/util"
lru "github.com/hashicorp/golang-lru/v2"
"xorm.io/builder" "xorm.io/builder"
) )
@@ -40,61 +44,82 @@ func IsCodeOwnerFile(f string) bool {
} }
// Get all code owner rules for a given pr + repo combination. // Get all code owner rules for a given pr + repo combination.
func getCodeOwnerRules(ctx context.Context, repo *git.Repository, pr *issues_model.PullRequest) ([]*issues_model.CodeOwnerRule, error) { func getCodeOwnerRules(ctx context.Context, repo *git.Repository, pr *issues_model.PullRequest) ([]*issues_model.CodeOwnerRule, []string, error) {
if err := pr.LoadBaseRepo(ctx); err != nil { if err := pr.LoadBaseRepo(ctx); err != nil {
return nil, err return nil, nil, err
} }
if err := pr.LoadIssue(ctx); err != nil { if err := pr.LoadIssue(ctx); err != nil {
return nil, err return nil, nil, err
} }
pr.Issue.Repo = pr.BaseRepo pr.Issue.Repo = pr.BaseRepo
if pr.BaseRepo.IsFork { if pr.BaseRepo.IsFork {
return nil, nil return nil, nil, nil
} }
commit, err := repo.GetBranchCommit(ctx, pr.BaseBranch) commit, err := repo.GetBranchCommit(ctx, pr.BaseBranch)
if err != nil { if err != nil {
return nil, err return nil, nil, err
} }
return GetCodeOwnerRules(ctx, repo, commit)
}
var data string const maxCodeOwnersFileSize = 3 * 1024 * 1024 // 3M as GitHub
type repoCommitCodeOwnersItem struct {
content string
warnings []string
}
var repoCommitCodeOwnersCache = sync.OnceValue(func() *lru.Cache[string, *repoCommitCodeOwnersItem] {
cache, _ := lru.New[string, *repoCommitCodeOwnersItem](50)
return cache
})
func getCodeOwnerRulesContent(ctx context.Context, repo *git.Repository, commit *git.Commit) (string, error) {
for _, file := range codeOwnerFiles { for _, file := range codeOwnerFiles {
blob, err := commit.GetBlobByPath(ctx, repo, file) blob, err := commit.GetBlobByPath(ctx, repo, file)
if err != nil { if errors.Is(err, util.ErrNotExist) {
continue // no CODEOWNERS at this path, try the next candidate continue // no CODEOWNERS at this path, try the next candidate
} else if err != nil {
return "", err
} }
// A truncated CODEOWNERS would silently drop rules, so fail closed rather // A truncated CODEOWNERS would silently drop rules, so fail closed rather
// than evaluate an incomplete file (the gate must not under-enforce). // than evaluate an incomplete file (the gate must not under-enforce).
if blob.Size(ctx) > setting.UI.MaxDisplayFileSize { if blob.Size(ctx) > maxCodeOwnersFileSize {
return nil, fmt.Errorf("CODEOWNERS file %q exceeds the maximum readable size", file) return "", util.ErrorWrap(util.ErrUnprocessableContent, "CODEOWNERS file exceeds the maximum readable size")
} }
// The file exists but is unreadable: propagate instead of swallowing, so // The file exists but is unreadable: propagate instead of swallowing, so
// callers can fail closed instead of treating it as "no code owners". // callers can fail closed instead of treating it as "no code owners".
data, err = blob.GetBlobContent(ctx, setting.UI.MaxDisplayFileSize) data, err := blob.GetBlobContent(ctx, maxCodeOwnersFileSize)
if err != nil { if err != nil {
return nil, err log.Error("GetCodeOwnerRules: failed to read CODEOWNERS file %q at commit %s: %v", file, commit.ID, err)
return "", util.ErrorWrap(util.ErrUnprocessableContent, "failed to read CODEOWNERS file")
} }
break return data, nil
} }
return "", nil
}
// no code owner file = no one to approve // GetCodeOwnerRules reads CODEOWNERS from the supplied commit.
if data == "" { func GetCodeOwnerRules(ctx context.Context, repo *git.Repository, commit *git.Commit) (rules []*issues_model.CodeOwnerRule, warnings []string, err error) {
return nil, nil key := fmt.Sprintf("%s:%s", repo.GitRepoManagedID(), commit.ID)
it, ok := repoCommitCodeOwnersCache().Get(key)
if !ok {
content, err := getCodeOwnerRulesContent(ctx, repo, commit)
if err != nil {
return nil, nil, err
}
it = &repoCommitCodeOwnersItem{content, warnings}
repoCommitCodeOwnersCache().Add(key, it)
} }
if len(it.warnings) != 0 {
rules, warnings := issues_model.GetCodeOwnersFromContent(ctx, data) return nil, it.warnings, nil
for _, w := range warnings {
log.Warn("CODEOWNERS parsing for PR %s#%d: %s", pr.BaseRepo.FullName(), pr.ID, w)
} }
if len(rules) == 0 { rules, warnings = issues_model.GetCodeOwnersFromContent(ctx, it.content) // it loads database for users and teams, so we can't cache the rules
return nil, nil return rules, warnings, nil
}
return rules, nil
} }
// Get the matching code owner rules for a given pr + repo combination. The returned // Get the matching code owner rules for a given pr + repo combination. The returned
@@ -110,7 +135,7 @@ func getMatchingCodeOwnerRules(ctx context.Context, pr *issues_model.PullRequest
} }
defer closer.Close() defer closer.Close()
rules, err := getCodeOwnerRules(ctx, repo, pr) rules, _, err := getCodeOwnerRules(ctx, repo, pr)
if err != nil { if err != nil {
return nil, false, err return nil, false, err
} }
@@ -131,30 +156,53 @@ func getMatchingCodeOwnerRules(ctx context.Context, pr *issues_model.PullRequest
return nil, false, err return nil, false, err
} }
matchingRules = make([]*issues_model.CodeOwnerRule, 0) matchingRules, warnings := MatchCodeOwnerRules(rules, changedFiles)
complete = true complete = len(warnings) == 0
if !complete {
log.Warn("CODEOWNERS matching for PR %s#%d exceeded its time budget; some rules were not evaluated", pr.BaseRepo.FullName(), pr.ID)
}
return matchingRules, complete, nil
}
// Bound the total time spent matching rules×files. The per-rule MatchTimeout // MatchCodeOwnerRules selects matching rules, with the last match winning within each named section.
// only caps a single match; without an aggregate budget a crafted CODEOWNERS func MatchCodeOwnerRules(rules []*issues_model.CodeOwnerRule, paths []string) (_ []*issues_model.CodeOwnerRule, warnings []string) {
// plus a PR touching many files could still exhaust CPU inside this loop. selected := make(map[*issues_model.CodeOwnerRule]bool)
matchDeadline := time.Now().Add(codeOwnerMatchBudget) deadline := time.Now().Add(codeOwnerMatchBudget)
ruleLoop: for _, path := range paths {
for _, rule := range rules { sections := make(map[string]*issues_model.CodeOwnerRule)
for _, f := range changedFiles { for _, rule := range rules {
if time.Now().After(matchDeadline) { if rule.Section == "" && selected[rule] {
log.Warn("CODEOWNERS matching for PR %s#%d exceeded its time budget; some rules were not evaluated", pr.BaseRepo.FullName(), pr.ID) continue
complete = false
break ruleLoop
} }
matched, _ := rule.Rule.MatchString(f) // err only happens when timeouts, any error can be considered as not matched if time.Now().After(deadline) {
if matched != rule.Negative { return nil, []string{"CODEOWNERS matching exceeded its time budget"}
matchingRules = append(matchingRules, rule) }
break patternMatched, err := rule.Rule.MatchString(path)
// err only happens when timeouts, any error can be considered as not matched
if err != nil {
return nil, []string{fmt.Sprintf("CODEOWNERS rule %q times out for matching against path %q", rule.Rule.String(), path)}
}
ruleMatched := patternMatched != rule.Negative
if !ruleMatched {
continue
}
if rule.Section == "" {
selected[rule] = true
} else {
sections[rule.Section] = rule
} }
} }
for _, rule := range sections {
selected[rule] = true
}
} }
matching := make([]*issues_model.CodeOwnerRule, 0, len(selected))
return matchingRules, complete, nil for _, rule := range rules {
if selected[rule] {
matching = append(matching, rule)
}
}
return matching, nil
} }
func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.ProtectedBranch, pr *issues_model.PullRequest) bool { func HasAllRequiredCodeownerReviews(ctx context.Context, pb *git_model.ProtectedBranch, pr *issues_model.PullRequest) bool {
@@ -279,8 +327,27 @@ func pullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque
issue := pr.Issue issue := pr.Issue
uniqUsers := make(map[int64]*user_model.User) uniqUsers := make(map[int64]*user_model.User)
uniqTeams := make(map[string]*org_model.Team) uniqTeams := make(map[string]*org_model.Team)
// TODO: should a user in a team be notified if the user themselves doesn't accept notification? Leave the decisions to the future ....
watchingPullRequests := make(map[int64]bool)
for _, rule := range matchingRules { for _, rule := range matchingRules {
if !rule.ReviewRequest.ValueOrDefault(true) {
continue
}
for _, u := range rule.Users { for _, u := range rule.Users {
if !rule.ReviewRequest.Has() && uniqUsers[u.ID] == nil {
watching, ok := watchingPullRequests[u.ID]
if !ok {
watch, err := repo_model.GetWatch(ctx, u.ID, issue.RepoID)
if err != nil {
return nil, err
}
watching = watch.IsWatching() && watch.IncludePullRequests
watchingPullRequests[u.ID] = watching
}
if !watching {
continue
}
}
uniqUsers[u.ID] = u uniqUsers[u.ID] = u
} }
for _, t := range rule.Teams { for _, t := range rule.Teams {

View File

@@ -4,13 +4,16 @@
package issue package issue
import ( import (
"strings"
"testing" "testing"
"gitea.dev/models/db" "gitea.dev/models/db"
issues_model "gitea.dev/models/issues" issues_model "gitea.dev/models/issues"
org_model "gitea.dev/models/organization" org_model "gitea.dev/models/organization"
repo_model "gitea.dev/models/repo"
"gitea.dev/models/unittest" "gitea.dev/models/unittest"
user_model "gitea.dev/models/user" user_model "gitea.dev/models/user"
"gitea.dev/modules/optional"
"github.com/stretchr/testify/assert" "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
@@ -30,8 +33,8 @@ func TestPullRequestCodeOwnersReview(t *testing.T) {
team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: 2}) team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: 2})
rules := []*issues_model.CodeOwnerRule{ rules := []*issues_model.CodeOwnerRule{
{Users: []*user_model.User{poster, reviewer, requestedReviewer}, Teams: []*org_model.Team{team}}, {Users: []*user_model.User{poster, reviewer, requestedReviewer}, Teams: []*org_model.Team{team}, ReviewRequest: optional.Some(true)},
{Users: []*user_model.User{reviewer}, Teams: []*org_model.Team{team}}, {Users: []*user_model.User{reviewer}, Teams: []*org_model.Team{team}, ReviewRequest: optional.Some(true)},
} }
for _, tc := range []struct { for _, tc := range []struct {
@@ -63,7 +66,12 @@ func TestPullRequestCodeOwnersReview(t *testing.T) {
before, err := db.GetEngine(ctx).Count(&issues_model.Review{IssueID: pr.IssueID}) before, err := db.GetEngine(ctx).Count(&issues_model.Review{IssueID: pr.IssueID})
require.NoError(t, err) require.NoError(t, err)
notifiers, err := pullRequestCodeOwnersReview(ctx, pr, rules) disabled := []*issues_model.CodeOwnerRule{{Users: []*user_model.User{reviewer}, Teams: []*org_model.Team{team}, ReviewRequest: optional.Some(false)}}
notifiers, err := pullRequestCodeOwnersReview(ctx, pr, disabled)
require.NoError(t, err)
assert.Empty(t, notifiers)
notifiers, err = pullRequestCodeOwnersReview(ctx, pr, rules)
require.NoError(t, err) require.NoError(t, err)
var users, teams int var users, teams int
@@ -95,3 +103,81 @@ func TestPullRequestCodeOwnersReview(t *testing.T) {
}) })
} }
} }
func TestMatchCodeOwnerRules(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
rules, warnings := issues_model.GetCodeOwnersFromContent(t.Context(), strings.TrimSpace(`
*.md @user1
[Docs] @user5
*.md
README.md @user4
[Docs] @user2
other.md
[Independent] @user1
*.md
`))
require.Empty(t, warnings)
for _, tc := range []struct {
paths []string
lineNums []int
}{
{[]string{"README.md"}, []int{1, 4, 8}},
{[]string{"README.md", "guide.md"}, []int{1, 3, 4, 8}},
{[]string{"other.md"}, []int{1, 6, 8}},
{[]string{"file.go"}, nil},
} {
t.Run(strings.Join(tc.paths, ","), func(t *testing.T) {
matching, warnings := MatchCodeOwnerRules(rules, tc.paths)
assert.Empty(t, warnings)
var lineNums []int
for _, rule := range matching {
lineNums = append(lineNums, rule.LineNum)
}
assert.Equal(t, tc.lineNums, lineNums)
})
}
}
func TestPullRequestCodeOwnersReviewWatch(t *testing.T) {
require.NoError(t, unittest.PrepareTestDatabase())
pr := unittest.AssertExistsAndLoadBean(t, &issues_model.PullRequest{IssueID: 12})
require.NoError(t, pr.LoadIssue(t.Context()))
require.NoError(t, pr.LoadBaseRepo(t.Context()))
require.NoError(t, pr.Issue.LoadRepo(t.Context()))
reviewer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
for _, tc := range []struct {
name string
mode repo_model.WatchMode
includePullRequests bool
content string
want bool
}{
{"not watching", repo_model.WatchModeNone, true, "*.md @user4", false},
{"ignoring", repo_model.WatchModeDont, true, "*.md @user4", false},
{"watching pull requests", repo_model.WatchModeCustom, true, "*.md @user4", true},
{"not watching pull requests", repo_model.WatchModeCustom, false, "*.md @user4", false},
{"explicit true without watch", repo_model.WatchModeNone, false, "*.md @user4 ## review-request=true", true},
{"explicit true overrides opt out", repo_model.WatchModeCustom, false, "*.md @user4 ## review-request=true", true},
{"explicit false overrides watch", repo_model.WatchModeCustom, true, "*.md @user4 ## review-request=false", false},
{"explicit true on another rule", repo_model.WatchModeCustom, false, "*.md @user4\n*.js @user4 ## review-request=true", true},
} {
t.Run(tc.name, func(t *testing.T) {
ctx, committer, err := db.TxContext(t.Context())
require.NoError(t, err)
defer committer.Close()
require.NoError(t, repo_model.WatchRepoWithOptions(ctx, reviewer, pr.BaseRepo, repo_model.WatchOptions{
Mode: tc.mode, WatchPullRequests: tc.includePullRequests, WatchIssues: true,
}))
rules, warnings := issues_model.GetCodeOwnersFromContent(ctx, tc.content)
require.Empty(t, warnings)
notifiers, err := pullRequestCodeOwnersReview(ctx, pr, rules)
require.NoError(t, err)
if tc.want {
require.Len(t, notifiers, 1)
assert.Equal(t, reviewer.ID, notifiers[0].Reviewer.ID)
} else {
assert.Empty(t, notifiers)
}
})
}
}

View File

@@ -23,6 +23,14 @@
{{end}} {{end}}
{{end}} {{end}}
</div> </div>
{{if .FileCodeOwners}}
<div class="file-codeowners ui segment flex-text-block tw-flex-wrap">
{{svg "octicon-shield-check" 16}} <span>{{ctx.Locale.Tr "repo.codeowners"}}</span>
{{range $one := .FileCodeOwners}}
<a href="{{$one.CodeOwnerLink}}">{{$one.CodeOwnerName}}</a>
{{end}}
</div>
{{end}}
{{end}} {{end}}
<h4 class="file-header ui top attached header flex-left-right"> <h4 class="file-header ui top attached header flex-left-right">

View File

@@ -94,6 +94,12 @@ func TestPullView_CodeOwner(t *testing.T) {
}, true) }, true)
assert.NoError(t, err) assert.NoError(t, err)
for _, reviewer := range []*user_model.User{user5, user8} {
require.NoError(t, repo_model.WatchRepoWithOptions(t.Context(), reviewer, repo, repo_model.WatchOptions{
Mode: repo_model.WatchModeCustom, WatchPullRequests: true,
}))
}
// create code owner branch protection // create code owner branch protection
protectBranch := git_model.ProtectedBranch{ protectBranch := git_model.ProtectedBranch{
BlockOnCodeownerReviews: true, BlockOnCodeownerReviews: true,
@@ -118,6 +124,31 @@ func TestPullView_CodeOwner(t *testing.T) {
}) })
assert.NoError(t, err) assert.NoError(t, err)
t.Run("File owners", func(t *testing.T) {
session := loginUser(t, "user2")
resp := session.MakeRequest(t, NewRequest(t, "GET", "/user2/test_codeowner/src/branch/master/README.md"), http.StatusOK)
doc := NewHTMLParser(t, resp.Body)
assert.Equal(t, "@user5", doc.Find(".file-codeowners a").Text())
_, err := files_service.ChangeRepoFiles(t.Context(), repo, user2, &files_service.ChangeRepoFilesOptions{
OldBranch: repo.DefaultBranch,
NewBranch: "file-owners",
Files: []*files_service.ChangeRepoFile{{
Operation: "update",
TreePath: "CODEOWNERS",
ContentReader: strings.NewReader("## review-request=false\n[Docs] @user8 @org3/team1\n*.md\n"),
}},
})
require.NoError(t, err)
resp = session.MakeRequest(t, NewRequest(t, "GET", "/user2/test_codeowner/src/branch/file-owners/README.md"), http.StatusOK)
doc = NewHTMLParser(t, resp.Body)
assert.Equal(t, "@user8", doc.Find(".file-codeowners a[href='/user8']").Text())
assert.Equal(t, "@org3/team1", doc.Find(".file-codeowners a[href='/org3/teams/team1']").Text())
resp = session.MakeRequest(t, NewRequest(t, "GET", "/user2/test_codeowner/src/branch/file-owners/CODEOWNERS"), http.StatusOK)
doc = NewHTMLParser(t, resp.Body)
assert.Zero(t, doc.Find(".file-codeowners").Length())
})
readmeBranchOptions := func(branch, content string) *files_service.ChangeRepoFilesOptions { readmeBranchOptions := func(branch, content string) *files_service.ChangeRepoFilesOptions {
return &files_service.ChangeRepoFilesOptions{ return &files_service.ChangeRepoFilesOptions{
NewBranch: branch, NewBranch: branch,