diff --git a/models/issues/code_owner.go b/models/issues/code_owner.go new file mode 100644 index 00000000000..0f192188644 --- /dev/null +++ b/models/issues/code_owner.go @@ -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 +} diff --git a/models/issues/pull.go b/models/issues/pull.go index 91c52250330..b3969d27c00 100644 --- a/models/issues/pull.go +++ b/models/issues/pull.go @@ -10,7 +10,6 @@ import ( "fmt" "io" "strings" - "time" "gitea.dev/models/db" git_model "gitea.dev/models/git" @@ -24,7 +23,6 @@ import ( "gitea.dev/modules/timeutil" "gitea.dev/modules/util" - "github.com/dlclark/regexp2/v2" "xorm.io/builder" ) @@ -825,162 +823,6 @@ func MergeBlockedByOutdatedBranch(protectBranch *git_model.ProtectedBranch, pr * 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 func InsertPullRequests(ctx context.Context, prs ...*PullRequest) error { return db.WithTx(ctx, func(ctx context.Context) error { diff --git a/models/issues/pull_test.go b/models/issues/pull_test.go index 0aebda3290b..89b734a373e 100644 --- a/models/issues/pull_test.go +++ b/models/issues/pull_test.go @@ -44,6 +44,7 @@ func TestPullRequest(t *testing.T) { t.Run("ParseCodeOwnersLine", testParseCodeOwnersLine) t.Run("CodeOwnerAbsolutePathPatterns", testCodeOwnerAbsolutePathPatterns) t.Run("CodeOwnerPatternMatchTimeout", testCodeOwnerPatternMatchTimeout) + t.Run("CodeOwnerSectionsAndAnnotations", testCodeOwnerSectionsAndAnnotations) t.Run("GetApprovers", testGetApprovers) t.Run("GetPullRequestByMergedCommit", testGetPullRequestByMergedCommit) t.Run("Migrate_InsertPullRequests", testMigrateInsertPullRequests) @@ -335,7 +336,7 @@ func testParseCodeOwnersLine(t *testing.T) { given := []CodeOwnerTest{ {Line: "", Tokens: nil}, - {Line: "# comment", Tokens: []string{}}, + {Line: "# comment", Tokens: nil}, {Line: "!.* @user1 @org1/team1", Tokens: []string{"!.*", "@user1", "@org1/team1"}}, {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"}}, @@ -347,7 +348,7 @@ func testParseCodeOwnersLine(t *testing.T) { for _, g := range given { 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{}) } + +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()) +} diff --git a/modules/glob/glob.go b/modules/glob/glob.go index 1bc5a6dec62..363868c8062 100644 --- a/modules/glob/glob.go +++ b/modules/glob/glob.go @@ -196,6 +196,14 @@ func Compile(pattern string, separators ...rune) (Glob, error) { 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) { return initGlobCompiler(&globCompiler{ regexpQuestion: true, diff --git a/options/locale/locale_en-US.json b/options/locale/locale_en-US.json index 47b313d900f..2a242ac870d 100644 --- a/options/locale/locale_en-US.json +++ b/options/locale/locale_en-US.json @@ -1731,6 +1731,7 @@ "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.wait": "was requested for review %s", + "repo.codeowners": "Code owners", "repo.issues.review.codeowners_rules": "CODEOWNERS rules", "repo.issues.review.add_review_request": "requested review from %s %s", "repo.issues.review.remove_review_request": "removed review request for %s %s", diff --git a/routers/web/repo/view_file.go b/routers/web/repo/view_file.go index d003d11102d..b249f672dbc 100644 --- a/routers/web/repo/view_file.go +++ b/routers/web/repo/view_file.go @@ -9,10 +9,12 @@ import ( "image" "io" "path" + "slices" "strings" git_model "gitea.dev/models/git" issue_model "gitea.dev/models/issues" + org_model "gitea.dev/models/organization" "gitea.dev/models/renderhelper" user_model "gitea.dev/models/user" "gitea.dev/modules/actions" @@ -153,9 +155,53 @@ func handleFileViewRenderImage(ctx *context.Context, fInfo *fileInfo, prefetchBu 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) { ctx.Data["IsViewFile"] = true ctx.Data["HideRepoInfo"] = true + prepareFileCodeOwners(ctx) if !prepareLatestCommitInfo(ctx) { return diff --git a/services/issue/pull.go b/services/issue/pull.go index b070bdc49c9..3eebcade21f 100644 --- a/services/issue/pull.go +++ b/services/issue/pull.go @@ -5,20 +5,24 @@ package issue import ( "context" + "errors" "fmt" "slices" + "sync" "time" "gitea.dev/models/db" git_model "gitea.dev/models/git" issues_model "gitea.dev/models/issues" org_model "gitea.dev/models/organization" + repo_model "gitea.dev/models/repo" user_model "gitea.dev/models/user" "gitea.dev/modules/git" "gitea.dev/modules/log" "gitea.dev/modules/optional" - "gitea.dev/modules/setting" + "gitea.dev/modules/util" + lru "github.com/hashicorp/golang-lru/v2" "xorm.io/builder" ) @@ -40,61 +44,82 @@ func IsCodeOwnerFile(f string) bool { } // 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 { - return nil, err + return nil, nil, err } if err := pr.LoadIssue(ctx); err != nil { - return nil, err + return nil, nil, err } pr.Issue.Repo = pr.BaseRepo if pr.BaseRepo.IsFork { - return nil, nil + return nil, nil, nil } commit, err := repo.GetBranchCommit(ctx, pr.BaseBranch) 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 { 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 + } else if err != nil { + return "", err } // A truncated CODEOWNERS would silently drop rules, so fail closed rather // than evaluate an incomplete file (the gate must not under-enforce). - if blob.Size(ctx) > setting.UI.MaxDisplayFileSize { - return nil, fmt.Errorf("CODEOWNERS file %q exceeds the maximum readable size", file) + if blob.Size(ctx) > maxCodeOwnersFileSize { + return "", util.ErrorWrap(util.ErrUnprocessableContent, "CODEOWNERS file exceeds the maximum readable size") } // The file exists but is unreadable: propagate instead of swallowing, so // 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 { - 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 - if data == "" { - return nil, nil +// GetCodeOwnerRules reads CODEOWNERS from the supplied commit. +func GetCodeOwnerRules(ctx context.Context, repo *git.Repository, commit *git.Commit) (rules []*issues_model.CodeOwnerRule, warnings []string, err error) { + 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) } - - rules, warnings := issues_model.GetCodeOwnersFromContent(ctx, data) - for _, w := range warnings { - log.Warn("CODEOWNERS parsing for PR %s#%d: %s", pr.BaseRepo.FullName(), pr.ID, w) + if len(it.warnings) != 0 { + return nil, it.warnings, nil } - if len(rules) == 0 { - return nil, nil - } - - return rules, nil + rules, warnings = issues_model.GetCodeOwnersFromContent(ctx, it.content) // it loads database for users and teams, so we can't cache the rules + return rules, warnings, nil } // 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() - rules, err := getCodeOwnerRules(ctx, repo, pr) + rules, _, err := getCodeOwnerRules(ctx, repo, pr) if err != nil { return nil, false, err } @@ -131,30 +156,53 @@ func getMatchingCodeOwnerRules(ctx context.Context, pr *issues_model.PullRequest return nil, false, err } - matchingRules = make([]*issues_model.CodeOwnerRule, 0) - complete = true + matchingRules, warnings := MatchCodeOwnerRules(rules, changedFiles) + 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 - // only caps a single match; without an aggregate budget a crafted CODEOWNERS - // plus a PR touching many files could still exhaust CPU inside this loop. - matchDeadline := time.Now().Add(codeOwnerMatchBudget) -ruleLoop: - for _, rule := range rules { - for _, f := range changedFiles { - if time.Now().After(matchDeadline) { - log.Warn("CODEOWNERS matching for PR %s#%d exceeded its time budget; some rules were not evaluated", pr.BaseRepo.FullName(), pr.ID) - complete = false - break ruleLoop +// MatchCodeOwnerRules selects matching rules, with the last match winning within each named section. +func MatchCodeOwnerRules(rules []*issues_model.CodeOwnerRule, paths []string) (_ []*issues_model.CodeOwnerRule, warnings []string) { + selected := make(map[*issues_model.CodeOwnerRule]bool) + deadline := time.Now().Add(codeOwnerMatchBudget) + for _, path := range paths { + sections := make(map[string]*issues_model.CodeOwnerRule) + for _, rule := range rules { + if rule.Section == "" && selected[rule] { + continue } - matched, _ := rule.Rule.MatchString(f) // err only happens when timeouts, any error can be considered as not matched - if matched != rule.Negative { - matchingRules = append(matchingRules, rule) - break + if time.Now().After(deadline) { + return nil, []string{"CODEOWNERS matching exceeded its time budget"} + } + 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 + } } - - return matchingRules, complete, nil + matching := make([]*issues_model.CodeOwnerRule, 0, len(selected)) + 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 { @@ -279,8 +327,27 @@ func pullRequestCodeOwnersReview(ctx context.Context, pr *issues_model.PullReque issue := pr.Issue uniqUsers := make(map[int64]*user_model.User) 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 { + if !rule.ReviewRequest.ValueOrDefault(true) { + continue + } 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 } for _, t := range rule.Teams { diff --git a/services/issue/pull_test.go b/services/issue/pull_test.go index 0af605d201e..5714f9b59e3 100644 --- a/services/issue/pull_test.go +++ b/services/issue/pull_test.go @@ -4,13 +4,16 @@ package issue import ( + "strings" "testing" "gitea.dev/models/db" issues_model "gitea.dev/models/issues" org_model "gitea.dev/models/organization" + repo_model "gitea.dev/models/repo" "gitea.dev/models/unittest" user_model "gitea.dev/models/user" + "gitea.dev/modules/optional" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -30,8 +33,8 @@ func TestPullRequestCodeOwnersReview(t *testing.T) { team := unittest.AssertExistsAndLoadBean(t, &org_model.Team{ID: 2}) rules := []*issues_model.CodeOwnerRule{ - {Users: []*user_model.User{poster, reviewer, requestedReviewer}, Teams: []*org_model.Team{team}}, - {Users: []*user_model.User{reviewer}, 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}, ReviewRequest: optional.Some(true)}, } 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}) 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) 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) + } + }) + } +} diff --git a/templates/repo/view_file.tmpl b/templates/repo/view_file.tmpl index 7a42e3a815d..55e5184b72d 100644 --- a/templates/repo/view_file.tmpl +++ b/templates/repo/view_file.tmpl @@ -23,6 +23,14 @@ {{end}} {{end}} + {{if .FileCodeOwners}} +
+ {{svg "octicon-shield-check" 16}} {{ctx.Locale.Tr "repo.codeowners"}} + {{range $one := .FileCodeOwners}} + {{$one.CodeOwnerName}} + {{end}} +
+ {{end}} {{end}}

diff --git a/tests/integration/pull_review_test.go b/tests/integration/pull_review_test.go index bf205ce1eb7..6f9c28d76ad 100644 --- a/tests/integration/pull_review_test.go +++ b/tests/integration/pull_review_test.go @@ -94,6 +94,12 @@ func TestPullView_CodeOwner(t *testing.T) { }, true) 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 protectBranch := git_model.ProtectedBranch{ BlockOnCodeownerReviews: true, @@ -118,6 +124,31 @@ func TestPullView_CodeOwner(t *testing.T) { }) 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 { return &files_service.ChangeRepoFilesOptions{ NewBranch: branch,