From d35db42583b668edea7655d72607463e4800be02 Mon Sep 17 00:00:00 2001 From: Johannes Batzill Date: Thu, 6 Jun 2024 16:58:01 +0000 Subject: [PATCH] Update CODEOWNERS to match github (#2069) --- app/api/controller/pullreq/codeowner.go | 1 + app/api/usererror/translate.go | 10 +- app/services/codeowners/service.go | 218 ++++++-- app/services/codeowners/service_test.go | 660 +++++++++++++++++++----- types/codeowners.go | 1 + 5 files changed, 713 insertions(+), 177 deletions(-) diff --git a/app/api/controller/pullreq/codeowner.go b/app/api/controller/pullreq/codeowner.go index d05ff6b5e..588ac335a 100644 --- a/app/api/controller/pullreq/codeowner.go +++ b/app/api/controller/pullreq/codeowner.go @@ -76,6 +76,7 @@ func mapCodeOwnerEvaluation(ownerEvaluation *codeowners.Evaluation) []types.Code } } codeOwnerEvaluationEntries[i] = types.CodeOwnerEvaluationEntry{ + LineNumber: entry.LineNumber, Pattern: entry.Pattern, OwnerEvaluations: ownerEvaluations, UserGroupOwnerEvaluations: userGroupOwnerEvaluations, diff --git a/app/api/usererror/translate.go b/app/api/usererror/translate.go index eea9172b4..08b753903 100644 --- a/app/api/usererror/translate.go +++ b/app/api/usererror/translate.go @@ -107,7 +107,15 @@ func Translate(ctx context.Context, err error) *Error { case errors.As(err, &codeOwnersTooLargeError): return UnprocessableEntityf(codeOwnersTooLargeError.Error()) case errors.As(err, &codeOwnersFileParseError): - return UnprocessableEntityf(codeOwnersFileParseError.Error()) + return NewWithPayload( + http.StatusUnprocessableEntity, + codeOwnersFileParseError.Error(), + map[string]any{ + "line_number": codeOwnersFileParseError.LineNumber, + "line": codeOwnersFileParseError.Line, + "err": codeOwnersFileParseError.Err.Error(), + }, + ) // lock errors case errors.As(err, &lockError): return errorFromLockError(lockError) diff --git a/app/services/codeowners/service.go b/app/services/codeowners/service.go index b49458047..2e5494d8d 100644 --- a/app/services/codeowners/service.go +++ b/app/services/codeowners/service.go @@ -19,6 +19,7 @@ import ( "context" "fmt" "io" + "sort" "strings" "github.com/harness/gitness/app/services/usergroup" @@ -31,6 +32,7 @@ import ( "github.com/bmatcuk/doublestar/v4" "github.com/rs/zerolog/log" + "golang.org/x/exp/slices" ) const ( @@ -44,6 +46,19 @@ const ( var ( ErrNotFound = errors.New("file not found") + + // escapableControlCharactersInPattern are control characters that are used to + // control the parsing of the codeowners file. + escapableControlCharactersInPattern = []rune{' ', '\t', '#'} + // escapableSpecialCharactersInPattern are special characters that are available in the pattern syntax + // to allow for more complex pattern matching. + escapableSpecialCharactersInPattern = []rune{'*', '?', '[', ']', '{', '}', '-', '!', '^'} + ErrFileParseInvalidEscapingInPattern = errors.New( + "a pattern requires '\\' to be escaped with another '\\', or used to escape control characters " + + "[space, tab, '#'] or any of the available special characters " + + "['*', '?', '[', ']', '{', '}', '-', '!', '^']", + ) + ErrFileParseTrailingBackslashInPattern = errors.New("a pattern can't end with a trailing '\\'") ) // TooLargeError represents an error if codeowners file is too large. @@ -67,15 +82,21 @@ func (e *TooLargeError) Is(target error) bool { // FileParseError represents an error if codeowners file is not parsable. type FileParseError struct { - line string + LineNumber int64 + Line string + Err error } func (e *FileParseError) Error() string { return fmt.Sprintf( - "The repository's CODEOWNERS file has an invalid line: %s", e.line, + "The repository's CODEOWNERS file has an error at line %d: %s", e.LineNumber, e.Err, ) } +func (e *FileParseError) Unwrap() error { + return e.Err +} + func (e *FileParseError) Is(target error) bool { _, ok := target.(*FileParseError) return ok @@ -105,8 +126,19 @@ type CodeOwners struct { } type Entry struct { + // LineNumber is the line number of the code owners entry. + LineNumber int64 + + // Pattern is a glob star pattern used to match the entry against a given file path. Pattern string - Owners []string + // Owners is the list of owners for the given pattern. + // NOTE: Could be empty in case of an entry that clears previously defined ownerships. + Owners []string +} + +// IsOwnershipReset returns true iff the entry resets any previously defined ownerships. +func (e *Entry) IsOwnershipReset() bool { + return len(e.Owners) == 0 } type Evaluation struct { @@ -115,6 +147,7 @@ type Evaluation struct { } type EvaluationEntry struct { + LineNumber int64 Pattern string OwnerEvaluations []OwnerEvaluation UserGroupOwnerEvaluations []UserGroupOwnerEvaluation @@ -174,29 +207,72 @@ func (s *Service) get( } func (s *Service) parseCodeOwner(codeOwnersContent string) ([]Entry, error) { + var lineNumber int64 var codeOwners []Entry scanner := bufio.NewScanner(strings.NewReader(codeOwnersContent)) for scanner.Scan() { - line := scanner.Text() - line = strings.TrimSpace(line) + lineNumber++ + originalLine := scanner.Text() + + line := strings.TrimSpace(originalLine) if line == "" || strings.HasPrefix(line, "#") { continue } - parts := strings.Split(line, " ") - if len(parts) < 2 { - return nil, &FileParseError{line} + isSeparator := func(r rune) bool { return r == ' ' || r == '\t' } + lineAsRunes := []rune(line) + pattern := strings.Builder{} + + // important to iterate over runes and not bytes to support utf-8 encoding. + for len(lineAsRunes) > 0 { + if isSeparator(lineAsRunes[0]) || lineAsRunes[0] == '#' { + break + } + + if lineAsRunes[0] == '\\' { + // ensure pattern doesn't end with trailing backslash. + if len(lineAsRunes) == 1 { + return nil, &FileParseError{ + LineNumber: lineNumber, + Line: originalLine, + Err: ErrFileParseTrailingBackslashInPattern, + } + } + + switch { + // escape character and special characters need to stay escaped ("\\", "\*", ...) + case lineAsRunes[1] == '\\' || slices.Contains(escapableSpecialCharactersInPattern, lineAsRunes[1]): + pattern.WriteRune('\\') + lineAsRunes = lineAsRunes[1:] + + // control characters aren't special characters in pattern syntax, so escaping should be removed. + case slices.Contains(escapableControlCharactersInPattern, lineAsRunes[1]): + lineAsRunes = lineAsRunes[1:] + + default: + return nil, &FileParseError{ + LineNumber: lineNumber, + Line: originalLine, + Err: ErrFileParseInvalidEscapingInPattern, + } + } + } + + pattern.WriteRune(lineAsRunes[0]) + lineAsRunes = lineAsRunes[1:] } - pattern := parts[0] - owners := parts[1:] - - codeOwner := Entry{ - Pattern: pattern, - Owners: owners, + // remove inline comment (can't be escaped in owners, only pattern supports escaping) + if i := slices.Index(lineAsRunes, '#'); i >= 0 { + lineAsRunes = lineAsRunes[:i] } - codeOwners = append(codeOwners, codeOwner) + codeOwners = append(codeOwners, Entry{ + LineNumber: lineNumber, + Pattern: pattern.String(), + // could be empty list in case of removing ownership + Owners: strings.FieldsFunc(string(lineAsRunes), isSeparator), + }) } if err := scanner.Err(); err != nil { return nil, fmt.Errorf("error reading input: %w", err) @@ -290,7 +366,6 @@ func (s *Service) getApplicableCodeOwnersForPR( return nil, err } - var filteredEntries []Entry diffFileStats, err := s.git.DiffFileNames(ctx, &git.DiffParams{ ReadParams: git.CreateReadParams(repo), BaseRef: pr.MergeBaseSHA, @@ -300,15 +375,33 @@ func (s *Service) getApplicableCodeOwnersForPR( return nil, fmt.Errorf("failed to get diff file stat: %w", err) } - for _, entry := range codeOwners.Entries { - ok, err := contains(entry.Pattern, diffFileStats.Files) - if err != nil { - return nil, err - } - if ok { - filteredEntries = append(filteredEntries, entry) + entryIDs := map[int]struct{}{} + for _, file := range diffFileStats.Files { + // last rule that matches wins (hence simply go in reverse order) + for i := len(codeOwners.Entries) - 1; i >= 0; i-- { + pattern := codeOwners.Entries[i].Pattern + if ok, err := match(pattern, file); err != nil { + return nil, fmt.Errorf("failed to match pattern %q for file %q: %w", pattern, file, err) + } else if ok { + entryIDs[i] = struct{}{} + break + } } } + + filteredEntries := make([]Entry, 0, len(entryIDs)) + for i := range entryIDs { + if !codeOwners.Entries[i].IsOwnershipReset() { + filteredEntries = append(filteredEntries, codeOwners.Entries[i]) + } + } + + // sort output to match order of occurrence in source CODEOWNERS file + sort.Slice( + filteredEntries, + func(i, j int) bool { return filteredEntries[i].LineNumber <= filteredEntries[j].LineNumber }, + ) + return &CodeOwners{ FileSHA: codeOwners.FileSHA, Entries: filteredEntries, @@ -367,6 +460,7 @@ func (s *Service) Evaluate( } if len(ownerEvaluations) != 0 || len(userGroupOwnerEvaluations) != 0 { evaluationEntries = append(evaluationEntries, EvaluationEntry{ + LineNumber: entry.LineNumber, Pattern: entry.Pattern, OwnerEvaluations: ownerEvaluations, UserGroupOwnerEvaluations: userGroupOwnerEvaluations, @@ -493,24 +587,64 @@ func findReviewerInList(email string, uid string, reviewers []*types.PullReqRevi return nil } -// We match a pattern list against a target -// doubleStar match allows to match / separated path wisely. -// A path foo/bar will match against pattern ** or foo/* -// Also, for a directory ending with / we have to return true for all files in that directory, -// hence we append ** for it. -func contains(pattern string, targets []string) (bool, error) { - for _, target := range targets { - // in case of / ending rule, owner owns the whole directory hence append ** - if strings.HasSuffix(pattern, "/") { - pattern += "**" - } - match, err := doublestar.PathMatch(pattern, target) - if err != nil { - return false, fmt.Errorf("failed to match pattern due to error: %w", err) - } - if match { - return true, nil - } +// Match matches a file path against the provided CODEOWNERS pattern. +// The code follows the .gitignore syntax closely (similar to github): +// https://git-scm.com/docs/gitignore#_pattern_format +// +// IMPORTANT: It seems that doublestar has a bug, as `*k/**` matches `k` but `k*/**` doesnt (incorrect)'. +// Because of that, we currently match patterns like `test*` only partially: +// - `test2`, `test/abc`, `test2/abc` are matching +// - `test` is not matching +// As a workaround, the user will have to add the same rule without a trailing `*` for now. +func match(pattern string, path string) (bool, error) { + if pattern == "" { + return false, fmt.Errorf("empty pattern not allowed") } - return false, nil + if path == "" { + return false, fmt.Errorf("empty path not allowed") + } + + // catch easy cases immediately to simplify code + if pattern == "/" || pattern == "*" || pattern == "**" { + return true, nil + } + + // cleanup path to simplify matching (always start with "/" and remove trailing "/") + if path[0] != '/' { + path = "/" + path + } + if path[len(path)-1] == '/' { + path = path[0 : len(path)-1] + } + + // if the pattern contains a slash anywhere but at the end, it's treated as an absolute path. + // Otherwise, the pattern can match on any level. + if !strings.Contains(pattern[:len(pattern)-1], "/") { + pattern = "**/" + pattern + } else if pattern[0] != '/' { + pattern = "/" + pattern + } + + // if the pattern ends with "/**", then it matches everything inside. + // Since doublestar matches pattern "x/**" with target "x", we replace it with "x/*/**". + if strings.HasSuffix(pattern, "/**") { + pattern = pattern[:len(pattern)-3] + "/*/**" + } + + // If CODEOWNERS matches a file, it also matches a folder with the same name, and anything inside that folder. + // Special case is a rule ending with "/", it only matches files inside the folder, not the folder itself. + // Since doublestar matches pattern "x/**" with target "x", we extend the pattern with "*/**" in such a case. + // Another special case is "/*", where the user explicitly stops nested matching. + if pattern[len(pattern)-1] == '/' { + pattern += "*/**" + } else if !strings.HasSuffix(pattern, "/**") && !strings.HasSuffix(pattern, "/*") { + pattern += "/**" + } + + match, err := doublestar.PathMatch(pattern, path) + if err != nil { + return false, fmt.Errorf("failed doublestar path match: %w", err) + } + + return match, nil } diff --git a/app/services/codeowners/service_test.go b/app/services/codeowners/service_test.go index 720b6931b..21ab5a24f 100644 --- a/app/services/codeowners/service_test.go +++ b/app/services/codeowners/service_test.go @@ -15,7 +15,6 @@ package codeowners import ( - "context" "reflect" "testing" @@ -24,11 +23,6 @@ import ( ) func TestService_ParseCodeOwner(t *testing.T) { - content1 := "**/contracts/openapi/v1/ mankrit.singh@harness.io ashish.sanodia@harness.io\n" - content2 := "**/contracts/openapi/v1/ mankrit.singh@harness.io ashish.sanodia@harness.io\n" + - "/scripts/api mankrit.singh@harness.io ashish.sanodia@harness.io" - content3 := "# codeowner file \n**/contracts/openapi/v1/ mankrit.singh@harness.io ashish.sanodia@harness.io\n" + - "#\n/scripts/api mankrit.singh@harness.io ashish.sanodia@harness.io" type fields struct { repoStore store.RepoStore git git.Interface @@ -46,36 +40,211 @@ func TestService_ParseCodeOwner(t *testing.T) { }{ { name: "Code owners Single", - args: args{codeOwnersContent: content1}, - want: []Entry{{ - Pattern: "**/contracts/openapi/v1/", - Owners: []string{"mankrit.singh@harness.io", "ashish.sanodia@harness.io"}, - }, - }, - }, - { - name: "Code owners Multiple", - args: args{codeOwnersContent: content2}, - want: []Entry{{ - Pattern: "**/contracts/openapi/v1/", - Owners: []string{"mankrit.singh@harness.io", "ashish.sanodia@harness.io"}, - }, + args: args{`**/contracts/openapi/v1/ user1@harness.io user2@harness.io`}, + want: []Entry{ { - Pattern: "/scripts/api", - Owners: []string{"mankrit.singh@harness.io", "ashish.sanodia@harness.io"}, + LineNumber: 1, + Pattern: "**/contracts/openapi/v1/", + Owners: []string{"user1@harness.io", "user2@harness.io"}, }, }, }, { - name: "Code owners With comments", - args: args{codeOwnersContent: content3}, - want: []Entry{{ - Pattern: "**/contracts/openapi/v1/", - Owners: []string{"mankrit.singh@harness.io", "ashish.sanodia@harness.io"}, - }, + name: "Code owners Multiple", + args: args{` +**/contracts/openapi/v1/ user1@harness.io user2@harness.io +/scripts/api user3@harness.io user4@harness.io + `}, + want: []Entry{ { - Pattern: "/scripts/api", - Owners: []string{"mankrit.singh@harness.io", "ashish.sanodia@harness.io"}, + LineNumber: 2, + Pattern: "**/contracts/openapi/v1/", + Owners: []string{"user1@harness.io", "user2@harness.io"}, + }, + { + LineNumber: 3, + Pattern: "/scripts/api", + Owners: []string{"user3@harness.io", "user4@harness.io"}, + }, + }, + }, + { + name: " Code owners with full line comments", + args: args{` +# codeowner file +**/contracts/openapi/v1/ user1@harness.io user2@harness.io +# +/scripts/api user1@harness.io user2@harness.io + `}, + want: []Entry{ + { + LineNumber: 3, + Pattern: "**/contracts/openapi/v1/", + Owners: []string{"user1@harness.io", "user2@harness.io"}, + }, + { + LineNumber: 5, + Pattern: "/scripts/api", + Owners: []string{"user1@harness.io", "user2@harness.io"}, + }, + }, + }, + { + name: " Code owners with reset", + args: args{` +* user1@harness.io +/scripts/api + `}, + want: []Entry{ + { + LineNumber: 2, + Pattern: "*", + Owners: []string{"user1@harness.io"}, + }, + { + LineNumber: 3, + Pattern: "/scripts/api", + Owners: []string{}, + }, + }, + }, + { + name: " Code owners with escaped characters in pattern", + args: args{` +# escaped escape character +\\ user1@harness.io +# escaped control characters (are unescaped) +\ \ \# user2@harness.io +# escaped special pattern syntax characters (stay escaped) +\*\?\[\]\{\}\-\!\^ user3@harness.io +# mix of escapes +\\\ \*\\\\\? user4@harness.io + `}, + want: []Entry{ + { + + LineNumber: 3, + Pattern: "\\\\", + Owners: []string{"user1@harness.io"}, + }, + { + + LineNumber: 5, + Pattern: " #", + Owners: []string{"user2@harness.io"}, + }, + { + LineNumber: 7, + Pattern: "\\*\\?\\[\\]\\{\\}\\-\\!\\^", + Owners: []string{"user3@harness.io"}, + }, + { + LineNumber: 9, + Pattern: "\\\\ \\*\\\\\\\\\\?", + Owners: []string{"user4@harness.io"}, + }, + }, + }, + { + name: " Code owners with multiple spaces as divider", + args: args{` +* user1@harness.io user2@harness.io + `}, + want: []Entry{ + { + LineNumber: 2, + Pattern: "*", + Owners: []string{"user1@harness.io", "user2@harness.io"}, + }, + }, + }, + { + name: " Code owners with invalid escaping standalone '\\'", + args: args{` +\ + `}, + wantErr: true, + }, + { + name: " Code owners with invalid escaping unsupported char", + args: args{` +\a + `}, + wantErr: true, + }, + { + name: " Code owners with utf8", + args: args{` +D∆NCE user@h∆rness.io + `}, + want: []Entry{ + { + LineNumber: 2, + Pattern: "D∆NCE", + Owners: []string{"user@h∆rness.io"}, + }, + }, + }, + { + name: " Code owners with tabs and spaces", + args: args{` +a\ user1@harness.io user2@harness.io user3@harness.io + + `}, + want: []Entry{ + { + LineNumber: 2, + Pattern: "a ", + Owners: []string{"user1@harness.io", "user2@harness.io", "user3@harness.io"}, + }, + }, + }, + { + name: " Code owners with inline comments", + args: args{` +a #user1@harness.io +b # user1@harness.io +c # +d# +e# user1@harness.io +f user1@harness.io#user2@harness.io +g user1@harness.io # user2@harness.io + `}, + want: []Entry{ + { + LineNumber: 2, + Pattern: "a", + Owners: []string{}, + }, + { + LineNumber: 3, + Pattern: "b", + Owners: []string{}, + }, + { + LineNumber: 4, + Pattern: "c", + Owners: []string{}, + }, + { + LineNumber: 5, + Pattern: "d", + Owners: []string{}, + }, + { + LineNumber: 6, + Pattern: "e", + Owners: []string{}, + }, + { + LineNumber: 7, + Pattern: "f", + Owners: []string{"user1@harness.io"}, + }, + { + LineNumber: 8, + Pattern: "g", + Owners: []string{"user1@harness.io"}, }, }, }, @@ -99,183 +268,406 @@ func TestService_ParseCodeOwner(t *testing.T) { } } -func Test_contains(t *testing.T) { - target1 := []string{"random"} - target2 := []string{"random/xyz"} - target3 := []string{"abhinav/path"} - target4 := []string{"abc/xyz"} - target5 := []string{"abc/xyz", "random"} - target6 := []string{"doc/frotz"} - target7 := []string{"filename"} - target8 := []string{"as/foo"} - target9 := []string{"foo/bar"} - target10 := []string{"a/x/y/b"} - target11 := []string{"foo/getting-started.md"} - target12 := []string{"foo.txt"} - target13 := []string{"/scripts/filename.txt"} - target14 := []string{"path/to/file.txt"} - target15 := []string{"path/to/foo"} - target16 := []string{"foo/build-app/troubleshooting.md"} - +func Test_match(t *testing.T) { type args struct { - ctx context.Context - pattern string - target []string + pattern string + matchingTargets []string + nonMatchingTargets []string } tests := []struct { name string args args - want bool }{ { - name: "Test * pattern", + name: "root", args: args{ - ctx: nil, - pattern: "*", - target: target1, + pattern: "/", + matchingTargets: []string{"a.txt", "x/a.txt", "x/y/a.txt"}, + nonMatchingTargets: []string{}, }, - want: true, }, { - name: "Test ** pattern", + name: "file exact match", args: args{ - ctx: nil, - pattern: "**", - target: target2, + pattern: "/a.txt", + matchingTargets: []string{"a.txt", "a.txt/b.go", "a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt2", "b.txt", "a.go", "x/a.txt"}, }, - want: true, }, { - name: "Test ** pattern on fixed path", + name: "file exact match with directory", args: args{ - ctx: nil, - pattern: "abc/xyz", - target: target3, + pattern: "/x/a.txt", + matchingTargets: []string{"x/a.txt", "x/a.txt/b.txt", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt", "x/a.txt2", "x/b.txt", "x/a.go"}, }, - want: false, }, { - name: "Test abc/xyz pattern", + name: "file relative match", args: args{ - ctx: nil, - pattern: "abc/xyz", - target: target4, + pattern: "a.txt", + matchingTargets: []string{"a.txt", "x/a.txt", "x/y/a.txt", "x/y/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt2", "b.txt", "a.go", "x/a.txt2"}, }, - want: true, }, { - name: "Test incorrect pattern negative", + name: "file relative match with directory", args: args{ - ctx: nil, - pattern: "random/xyz", - target: target5, + pattern: "x/a.txt", + matchingTargets: []string{"x/a.txt", "x/a.txt/b.go", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt2", "b.txt", "a.go", "x/a.txt2", "v/x/a.txt", "y/a.txt"}, }, - want: false, }, { - name: "Test file path with **", + name: "folder exact match", args: args{ - ctx: nil, - pattern: "**", - target: target14, + pattern: "/x/", + matchingTargets: []string{"x/a.txt", "x/b.go", "x/y/a.txt"}, + nonMatchingTargets: []string{"x", "a.txt", "y/a.txt", "w/x/a.txt"}, }, - want: true, }, { - name: "Test / pattern", + name: "folder relative match", args: args{ - ctx: nil, - pattern: "doc/frotz", - target: target6, + pattern: "x/", + matchingTargets: []string{"x/a.txt", "x/b.txt", "w/x/a.txt", "w/x/y/a.txt"}, + nonMatchingTargets: []string{"x", "w/x", "a.txt", "y/a.txt"}, }, - want: true, }, { - name: "Test ? pattern", + name: "match-all", args: args{ - ctx: nil, - pattern: "?ilename", - target: target7, + pattern: "*", + matchingTargets: []string{"a", "a.txt", "x/a.txt", "x/y/a.txt"}, + nonMatchingTargets: []string{}, }, - want: true, }, { - name: "Test /** pattern", + name: "match-all in relative dir", args: args{ - ctx: nil, - pattern: "**/foo", - target: target8, + pattern: "x/*", + matchingTargets: []string{"x/a.txt"}, + nonMatchingTargets: []string{"x", "y/a.txt", "w/x/b.go", "x/a.txt/b.txt"}, }, - want: true, }, { - name: "Test **/ with deep nesting", + name: "match-all in absolute dir", args: args{ - ctx: nil, - pattern: "**/foo", - target: target15, + pattern: "/x/*", + matchingTargets: []string{"x/a.txt", "x/b.go"}, + nonMatchingTargets: []string{"x", "y/a.txt", "w/x/a.txt", "x/a.txt/b.go"}, }, - want: true, }, { - name: "Test **/ pattern", + name: "file match-all type", args: args{ - ctx: nil, - pattern: "foo/**", - target: target9, + pattern: "*.txt", + matchingTargets: []string{"a.txt", "x/a.txt", "x/y/a.txt", "x/y/a.txt/b.go", "x/y/a.txt/c.ar"}, + nonMatchingTargets: []string{"a.txt2", "a.go"}, }, - want: true, }, { - name: "Test a/**/b pattern", + name: "file match-all type in root folder", args: args{ - ctx: nil, - pattern: "a/x/y/b", - target: target10, + pattern: "/*.txt", + matchingTargets: []string{"a.txt", "a.txt/b.go", "a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt2", "a.go", "x/a.txt", "x/y/a.txt"}, }, - want: true, }, { - name: "Test /* pattern positive", + name: "file match-all type in absolute sub folder", args: args{ - ctx: nil, - pattern: "foo/*", - target: target11, + pattern: "/x/*.txt", + matchingTargets: []string{"x/a.txt", "x/a.txt/b.go", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt", "x/a.txt2", "x/a.go", "w/x/a.txt", "y/a.txt"}, }, - want: true, }, { - name: "Test /* pattern negative", + name: "file match-all types in relative sub folder", args: args{ - ctx: nil, - pattern: "foo/*", - target: target16, + pattern: "x/*.txt", + matchingTargets: []string{"x/a.txt", "x/a.txt/b.go", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt", "x/a.txt2", "x/a.go", "w/x/a.txt", "y/a.txt"}, }, - want: false, }, { - name: "Test * for files", + name: "inner match-all", args: args{ - ctx: nil, - pattern: "*.txt", - target: target12, + pattern: "/x/*/a.txt", + matchingTargets: []string{"x/y/a.txt", "x/y/a.txt/b.go", "x/y/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt", "x/a.txt", "w/x/y/a.txt", "x/y/z/a.txt"}, }, - want: true, }, { - name: "Test /a/", + name: "escaped match-all", args: args{ - ctx: nil, - pattern: "/scripts/", - target: target13, + pattern: "\\*", + matchingTargets: []string{"*", "x/y/*", "x/y/*/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt"}, + }, + }, + /* + TODO: Fix bug in doublestar library, currently doesn't match `a.` ... + { + name: "trailing match-all on string", + args: args{ + pattern: "a.*", + matchingTargets: []string{"a.", "a.txt", "x/a.txt", "x/a.txt/b.go", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"atxt", "b.txt"}, + }, + }, + */ + { + name: "globstar", + args: args{ + pattern: "**", + matchingTargets: []string{"a", "a.txt", "x/a.txt", "x/y/a.txt"}, + nonMatchingTargets: []string{}, + }, + }, + { + name: "trailing globstar absolute path", + args: args{ + pattern: "/x/**", + matchingTargets: []string{"x/a.txt", "x/b.txt", "x/y/a.txt"}, + nonMatchingTargets: []string{"a.txt", "x", "y/a.txt", "w/x/a.txt"}, + }, + }, + { + name: "trailing globstar relative path", + args: args{ + pattern: "x/**", + matchingTargets: []string{"x/a.txt", "x/b.txt", "x/y/a.txt"}, + nonMatchingTargets: []string{"a.txt", "x", "y/a.txt", "w/x/a.txt"}, + }, + }, + /* + TODO: Fix bug in doublestar library, currently doesn't match `a.` ... + { + name: "trailing globstar on string", + args: args{ + pattern: "a.**", + matchingTargets: []string{"a.", "a.txt", "x/a.txt", "x/a.txt/b.go", "x/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"atxt", "b.txt"}, + }, + }, + */ + { + name: "leading globstar", + args: args{ + pattern: "**/a.txt", + matchingTargets: []string{"a.txt", "x/a.txt", "x/y/a.txt", "x/y/a.txt/b.go", "x/y/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"b.txt", "a.txt2"}, + }, + }, + { + name: "surrounding globstar", + args: args{ + pattern: "**/x/**", + matchingTargets: []string{"x/a.txt", "w/x/a.txt", "x/y/a.txt", "w/x/y/a.txt"}, + nonMatchingTargets: []string{"a.txt", "x", "w/x"}, + }, + }, + { + name: "inner globstar", + args: args{ + pattern: "/x/**/a.txt", + matchingTargets: []string{ + "x/a.txt", "x/y/a.txt", "x/y/z/a.txt", "x/y/z/a.txt/b.go", "x/y/z/a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.txt", "w/x/a.txt", "y/a.txt"}, + }, + }, + { + name: "multi-inner globstar", + args: args{ + pattern: "/x/**/z/**/a.txt", + matchingTargets: []string{ + "x/z/a.txt", + "x/y/z/l/a.txt", + "x/y/yy/z/l/ll/a.txt", + "x/y/yy/z/l/ll/a.txt/b.go", + "x/y/yy/z/l/ll/a.txt/b.go/c.ar", + }, + nonMatchingTargets: []string{"a.txt", "x/a.txt", "z/a.txt", "w/x/a.txt", "y/a.txt"}, + }, + }, + { + name: "dirty globstar", + args: args{ + pattern: "/a**.txt", + matchingTargets: []string{"a.txt", "abc.txt", "a.txt/b.go", "a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{"a/b/.txt"}, + }, + }, + { + name: "escaped globstar", + args: args{ + pattern: "\\*\\*", + matchingTargets: []string{"**", "x/**", "**/y", "x/**/y"}, + nonMatchingTargets: []string{"x"}, + }, + }, + { + name: "partially escaped globstar", + args: args{ + pattern: "*\\*", + matchingTargets: []string{"*", "**", "a*", "x/*", "x/a*", "*/y", "a*/", "x/*/y", "x/a*/y"}, + nonMatchingTargets: []string{"x"}, + }, + }, + { + name: "single wildchar", + args: args{ + pattern: "/a.?xt", + matchingTargets: []string{"a.txt", "a.xxt/b.go", "a.xxt/b.go/c.ar"}, + nonMatchingTargets: []string{"x/a.txt", "z/a.txt", "w/x/a.txt", "y/a.txt", "a./xt"}, + }, + }, + { + name: "escaped single wildchar", + args: args{ + pattern: "/a.\\?xt", + matchingTargets: []string{"a.?xt", "a.?xt/b.go", "a.?xt/b.go/c.ar"}, + nonMatchingTargets: []string{"a.\\?xt", "a.txt", "x/a.?xt"}, + }, + }, + { + name: "class", + args: args{ + pattern: "/[abc].txt", + matchingTargets: []string{"a.txt", "b.txt", "c.txt"}, + nonMatchingTargets: []string{"[a-c].txt", "d.txt", "A.txt"}, + }, + }, + { + name: "range class", + args: args{ + pattern: "/[a-c].txt", + matchingTargets: []string{"a.txt", "b.txt", "c.txt"}, + nonMatchingTargets: []string{"[a-c].txt", "d.txt", "A.txt"}, + }, + }, + { + name: "escaped class", + args: args{ + pattern: "/\\[a-c\\].txt", + matchingTargets: []string{"[a-c].txt"}, + nonMatchingTargets: []string{"\\[a-c\\].txt", "a.txt", "b.txt", "c.txt"}, + }, + }, + { + name: "class escaped control chars", + args: args{ + pattern: "/[\\!\\^\\-a-c].txt", + matchingTargets: []string{"a.txt", "b.txt", "c.txt", "^.txt", "!.txt", "-.txt"}, + nonMatchingTargets: []string{"d.txt", "[\\!\\^\\-a-c].txt", "[!^-a-c].txt"}, + }, + }, + { + name: "inverted class ^", + args: args{ + pattern: "/[^a-c].txt", + matchingTargets: []string{"d.txt", "B.txt"}, + nonMatchingTargets: []string{"a.txt", "b.txt", "c.txt", "[^a-c].txt", "[a-c].txt"}, + }, + }, + { + name: "escaped inverted class ^", + args: args{ + pattern: "/\\[^a-c\\].txt", + matchingTargets: []string{"[^a-c].txt"}, + nonMatchingTargets: []string{"\\[^a-c\\].txt", "a.txt", "b.txt", "c.txt", "d.txt", "[a-c].txt"}, + }, + }, + { + name: "inverted class !", + args: args{ + pattern: "/[!a-c].txt", + matchingTargets: []string{"d.txt", "B.txt"}, + nonMatchingTargets: []string{"a.txt", "b.txt", "c.txt", "[!a-c].txt", "[a-c].txt"}, + }, + }, + { + name: "escaped inverted class !", + args: args{ + pattern: "/\\[!a-c\\].txt", + matchingTargets: []string{"[!a-c].txt"}, + nonMatchingTargets: []string{"\\[!a-c\\].txt", "a.txt", "b.txt", "c.txt", "d.txt", "[a-c].txt"}, + }, + }, + { + name: "alternate matches", + args: args{ + pattern: "/{a,b,[c-d],e?,f\\*}.txt", + matchingTargets: []string{ + "a.txt", "b.txt", "c.txt", "d.txt", "e2.txt", "f*.txt", "a.txt/b.go", "a.txt/b.go/c.ar"}, + nonMatchingTargets: []string{ + "{a,b,[c-d],e?,f\\*}.txt", "{a,b,[c-d],e?,f*}.txt", "e.txt", "f.txt", "g.txt", "ab.txt"}, + }, + }, + { + name: "space", + args: args{ + pattern: "/a b.txt", + matchingTargets: []string{"a b.txt"}, + nonMatchingTargets: []string{"a.txt", "b.txt", "ab.txt", "a b.txt"}, + }, + }, + { + name: "tab", + args: args{ + pattern: "/a b.txt", + matchingTargets: []string{"a b.txt"}, + nonMatchingTargets: []string{"a.txt", "b.txt", "ab.txt", "a b.txt", "a b.txt"}, + }, + }, + { + // Note: it's debatable which behavior is correct - for now keep doublestar default behavior on this. + // Keeping UT to ensure we don't accidentally change behavior. + name: "escaped backslash", + args: args{ + pattern: "/a\\\\/b.txt", + matchingTargets: []string{"a\\/b.txt"}, + nonMatchingTargets: []string{"a\\\\/b.txt", "a/b.txt", "a/b.txt/c.ar"}, + }, + }, + { + // Note: it's debatable which behavior is correct - for now keep doublestar default behavior on this. + // Keeping UT to ensure we don't accidentally change behavior. + name: "escaped path separator", + args: args{ + pattern: "/a\\/b.txt", + matchingTargets: []string{"a/b.txt", "a/b.txt/c.ar"}, + nonMatchingTargets: []string{"a\\/b.txt"}, }, - want: true, }, } + testMatch := func(pattern string, target string, want bool) { + got, err := match(pattern, target) + if err != nil { + t.Errorf("failed with error: %s", err) + } else if got != want { + t.Errorf("match(%q, %q) = %t but wanted %t)", pattern, target, got, want) + } + } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got, _ := contains(tt.args.pattern, tt.args.target); got != tt.want { - t.Errorf("contains() = %v, want %v", got, tt.want) + for _, target := range tt.args.matchingTargets { + if len(target) > 0 && target[0] == '/' { + t.Errorf("target shouldn't start with leading '/'") + } + + testMatch(tt.args.pattern, target, true) + testMatch(tt.args.pattern, "/"+target, true) + } + for _, target := range tt.args.nonMatchingTargets { + if len(target) > 0 && target[0] == '/' { + t.Errorf("target shouldn't start with leading '/'") + } + + testMatch(tt.args.pattern, target, false) + testMatch(tt.args.pattern, "/"+target, false) } }) } diff --git a/types/codeowners.go b/types/codeowners.go index 7b30befd8..9b02d80d8 100644 --- a/types/codeowners.go +++ b/types/codeowners.go @@ -26,6 +26,7 @@ type CodeOwnerEvaluation struct { } type CodeOwnerEvaluationEntry struct { + LineNumber int64 `json:"line_number"` Pattern string `json:"pattern"` OwnerEvaluations []OwnerEvaluation `json:"owner_evaluations"` UserGroupOwnerEvaluations []UserGroupOwnerEvaluation `json:"user_group_owner_evaluations"`