From 712b4f172ff427c34c05bab899bf21bcf3237025 Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Thu, 25 Feb 2021 21:17:15 +0100 Subject: [PATCH 1/2] Add support for syntax which finds directory anywhere in repository --- internal/check/file_exists.go | 32 ++++++++++++++++++++++++++++-- internal/check/file_exists_test.go | 24 ++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) diff --git a/internal/check/file_exists.go b/internal/check/file_exists.go index 01b38bd..56fff46 100644 --- a/internal/check/file_exists.go +++ b/internal/check/file_exists.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/mszostok/codeowners-validator/internal/ctxutil" @@ -28,6 +29,7 @@ func (f *FileExist) Check(ctx context.Context, in Input) (Output, error) { fullPath := filepath.Join(in.RepoDir, f.fnmatchPattern(entry.Pattern)) matches, err := zglob.Glob(fullPath) + switch { case err == nil: case errors.Is(err, os.ErrNotExist): @@ -38,7 +40,20 @@ func (f *FileExist) Check(ctx context.Context, in Input) (Output, error) { return Output{}, errors.Wrapf(err, "while checking if there is any file in %s matching pattern %s", in.RepoDir, entry.Pattern) } - if len(matches) == 0 { + // Glob returns also matched directories, but only files count + foundFile := false + for _, name := range matches { + fi, err := os.Stat(name) + if err != nil { + return Output{}, errors.Wrap(err, "while checking if found path is a file") + } + if fi.Mode().IsRegular() { + foundFile = true + break + } + } + + if !foundFile { msg := fmt.Sprintf("%q does not match any files in repository", entry.Pattern) bldr.ReportIssue(msg, WithEntry(entry)) } @@ -48,10 +63,23 @@ func (f *FileExist) Check(ctx context.Context, in Input) (Output, error) { } func (*FileExist) fnmatchPattern(pattern string) string { - if len(pattern) >= 2 && pattern[:1] == "*" && pattern[1:2] != "*" { + noOfChars := len(pattern) + if noOfChars >= 2 && pattern[:1] == "*" && pattern[1:2] != "*" { return "**/" + pattern } + if noOfChars > 1 && pattern[:1] != "/" { + return "**/" + pattern + "**/*" + } + + lastIdx := strings.LastIndex(pattern, "/") + // handle such pattern: **/foo @pico + // we need to add the **/* to find all files, without that Glob + // will return only the first match which can be still a directory + if lastIdx != -1 && filepath.Ext(pattern[lastIdx+1:]) == "" { + return pattern + "**/*" + } + return pattern } diff --git a/internal/check/file_exists_test.go b/internal/check/file_exists_test.go index ea0dd07..fd0e7f2 100644 --- a/internal/check/file_exists_test.go +++ b/internal/check/file_exists_test.go @@ -150,6 +150,30 @@ func TestFileExists(t *testing.T) { newErrIssue(`"a/**/b" does not match any files in repository`), }, }, + "Should match any file in an `apps` directory anywhere in your repository": { + codeownersInput: ` + apps/ @octocat + `, + paths: []string{ + "a/somewhere/apps/the/a/the/b/the/c/the/d/the/e/z/somewhere/appz/the/f/the/g/the/h/the/i/the/j/foo.js", + }, + }, + "Should match files like `docs/getting-started.md`": { + codeownersInput: ` + docs/* docs@example.com + `, + paths: []string{ + "/docs/getting-started.md", + }, + }, + "Should not match nested files like `docs/build-app/troubleshooting.md`": { + codeownersInput: ` + docs/* docs@example.com + `, + expectedIssues: []check.Issue{ + newErrIssue(`"docs/*" does not match any files in repository`), + }, + }, } for tn, tc := range tests { From f4d748a75e9ffecc017a186e144a786a69286ab1 Mon Sep 17 00:00:00 2001 From: Mateusz Szostok Date: Thu, 25 Feb 2021 21:30:16 +0100 Subject: [PATCH 2/2] Add missing check --- internal/check/file_exists.go | 11 ++++++++--- internal/check/file_exists_test.go | 3 +++ 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/internal/check/file_exists.go b/internal/check/file_exists.go index 56fff46..e8f6dc5 100644 --- a/internal/check/file_exists.go +++ b/internal/check/file_exists.go @@ -68,21 +68,26 @@ func (*FileExist) fnmatchPattern(pattern string) string { return "**/" + pattern } - if noOfChars > 1 && pattern[:1] != "/" { + if noOfChars > 1 && pattern[:1] != "/" && !strings.HasSuffix(pattern, "*") { return "**/" + pattern + "**/*" } lastIdx := strings.LastIndex(pattern, "/") - // handle such pattern: **/foo @pico + // handle such pattern: `**/foo` // we need to add the **/* to find all files, without that Glob // will return only the first match which can be still a directory - if lastIdx != -1 && filepath.Ext(pattern[lastIdx+1:]) == "" { + if lastIdx != -1 && !hasSingleAsterisk(pattern) { return pattern + "**/*" } return pattern } +// do not match patterns like `docs/*` but match `docs/**` +func hasSingleAsterisk(pattern string) bool { + return strings.HasSuffix(pattern, "*") && !strings.HasSuffix(pattern, "**") +} + func (*FileExist) Name() string { return "File Exist Checker" } diff --git a/internal/check/file_exists_test.go b/internal/check/file_exists_test.go index fd0e7f2..4a12cb3 100644 --- a/internal/check/file_exists_test.go +++ b/internal/check/file_exists_test.go @@ -170,6 +170,9 @@ func TestFileExists(t *testing.T) { codeownersInput: ` docs/* docs@example.com `, + paths: []string{ + "/docs/build-app/troubleshooting.md", + }, expectedIssues: []check.Issue{ newErrIssue(`"docs/*" does not match any files in repository`), },