bugfix: Ignore paths filtering with file outside of paths

check.go

@@ -48,15 +48,57 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) {
 			log.Println("ignore paths configured:", iPaths)
 			log.Println("changed files found:", p.Files)
 
-			switch {
 			// if `paths` is configured && NONE of the changed files match `paths` pattern/s
-			case pullrequest.Patterns(paths)(p) && !pullrequest.Files(paths, false)(p):
-				log.Println("paths excluded pull")
-				continue
+			if pullrequest.Patterns(paths)(p) {
+				matches, err := pullrequest.Files(paths)(p.Files)
+				if err != nil {
+					log.Println("error identifying matching paths")
+					continue
+				}
+
+				if len(matches) == 0 {
+					log.Println("paths excluded pull")
+					continue
+				}
+			}
+
 			// if `ignore_paths` is configured && ALL of the changed files match `ignore_paths` pattern/s
-			case pullrequest.Patterns(iPaths)(p) && pullrequest.Files(iPaths, true)(p):
-				log.Println("ignore paths excluded pull")
-				continue
+			if pullrequest.Patterns(iPaths)(p) {
+				matches, err := pullrequest.Files(iPaths)(p.Files)
+				if err != nil {
+					log.Println("error identifying matching ignore_paths")
+					continue
+				}
+
+				if len(matches) == len(p.Files) {
+					log.Println("ignore paths excluded pull")
+					continue
+				}
+			}
+
+			// Both `paths` and `ignore_paths` are defined, it is possible for the pull request
+			// to contain files outside of `paths`
+			if pullrequest.Patterns(paths)(p) && pullrequest.Patterns(iPaths)(p) {
+				log.Println("paths and ignore_paths both defined")
+				matches, err := pullrequest.Files(paths)(p.Files)
+				if err != nil {
+					log.Println("error identifying matching paths when both paths and ignore_paths are defined")
+					continue
+				}
+
+				if len(matches) > 0 {
+					ignoreMatches, err := pullrequest.Files(iPaths)(matches)
+					if err != nil {
+						log.Println("error identifying matching ignore_paths when both paths and ignore_paths are defined")
+						continue
+					}
+
+					matches = diff(matches, ignoreMatches)
+				}
+
+				if len(matches) == 0 {
+					continue
+				}
 			}
 		}
 
@@ -83,6 +125,30 @@ func Check(request CheckRequest, manager Github) (CheckResponse, error) {
 	return response, nil
 }
 
+// diff returns elements in match that are not found in ignoreMatch
+func diff(match []string, ignoreMatch []string) []string {
+	found := make(map[string]bool)
+
+	for _, f := range match {
+		found[f] = true
+	}
+
+	for _, f := range ignoreMatch {
+		if _, ok := found[f]; ok {
+			found[f] = false
+		}
+	}
+
+	var diff []string
+	for file, ret := range found {
+		if ret {
+			diff = append(diff, file)
+		}
+	}
+
+	return diff
+}
+
 func newVersion(r CheckRequest, p pullrequest.PullRequest) bool {
 	switch {
 	// negative filters

pullrequest/filter.go

@@ -3,6 +3,7 @@ package pullrequest
 import (
 	"log"
 	"regexp"
+	"strings"
 	"time"
 
 	glob "github.com/sabhiram/go-gitignore"
@@ -191,37 +192,30 @@ func Patterns(patterns []string) Filter {
 	}
 }
 
-// Files matches the PRs changed files against a set of glob patterns:
-// (invert == false) returns true if patterns empty OR any of the changed files match the patterns
-// (invert == true) returns true if all of the changed files match the patterns to ignore
-func Files(patterns []string, invert bool) Filter {
-	return func(p PullRequest) bool {
-		matched := make([]int8, 0)
+// Files returns the files that match against a set of glob patterns:
+func Files(patterns []string) func([]string) ([]string, error) {
+	return func(files []string) ([]string, error) {
+		matches := make([]string, 0)
+
 		gc, err := glob.CompileIgnoreLines(patterns...)
 		if err != nil {
-			return false
+			return matches, err
 		}
 
-		for _, f := range p.Files {
+		for _, f := range files {
 			log.Println("comparing patterns to changed file:", f)
-			if gc.MatchesPath("/" + f) {
-				if !invert {
-					log.Println("paths: true -", "matched:", f)
-					return true
-				}
-
-				matched = append(matched, 1)
-				log.Println("matched:", f)
+			fn := f
+			if !strings.HasPrefix(f, "/") {
+				fn = "/" + f
 			}
-		}
 
-		log.Println("invert:", invert, "- matched:", len(matched), "- files:", len(p.Files))
-		if invert && len(matched) == len(p.Files) {
-			log.Println("ignore paths: true")
-			return true
+			if gc.MatchesPath(fn) {
+				matches = append(matches, f)
+			}
 		}
 
-		return false
+		log.Printf("found %d matching files", len(matches))
+		return matches, nil
 	}
 }
 

pullrequest/functional_test.go

@@ -692,125 +692,166 @@ func TestFiles(t *testing.T) {
 	tests := []struct {
 		description string
 		patterns    []string
-		invert      bool
-		pull        pullrequest.PullRequest
-		expect      bool
+		files       []string
+		expect      []string
 	}{
 		{
 			description: "match txt files @ root level",
 			patterns:    []string{"*.txt"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"file1.txt",
-				},
+			files: []string{
+				"file1.txt",
+			},
+			expect: []string{
+				"file1.txt",
 			},
-			expect: true,
 		},
 		{
 			description: "match txt files at any level",
 			patterns:    []string{"**/*.txt"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"test/file2.txt",
-					"test/testing/file2.txt",
-					"test/testing/tested/file2.txt",
-				},
+			files: []string{
+				"test/file2.txt",
+				"test/testing/file2.txt",
+				"test/testing/tested/file2.txt",
+			},
+			expect: []string{
+				"test/file2.txt",
+				"test/testing/file2.txt",
+				"test/testing/tested/file2.txt",
 			},
-			expect: true,
 		},
 		{
 			description: "match any file in test dir",
 			patterns:    []string{"test/*"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"file1.txt",
-					"test/file2.txt",
-				},
+			files: []string{
+				"file1.txt",
+				"test/file2.txt",
+			},
+			expect: []string{
+				"test/file2.txt",
 			},
-			expect: true,
 		},
 		{
 			description: "match any file recursively in test dir",
 			patterns:    []string{"test/**"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"file1.txt",
-					"test/testing/file2.txt",
-					"test/testing/tested/file2.txt",
-				},
+			files: []string{
+				"file1.txt",
+				"test/testing/file2.txt",
+				"test/testing/tested/file2.txt",
+			},
+			expect: []string{
+				"test/testing/file2.txt",
+				"test/testing/tested/file2.txt",
+			},
+		},
+		// {
+		// 	description: "no match in test subdirectory",
+		// 	patterns:    []string{"test/**"},
+		// 	files: []string{
+		// 		"file1.txt",
+		// 		"testing/test/file2.txt",
+		// 		"testing/test/tested/file2.txt",
+		// 	},
+		// 	expect: []string{},
+		// },
+		{
+			description: "No match in test root directory",
+			patterns:    []string{"/test/**"},
+			files: []string{
+				"file1.txt",
+				"testing/test/file2.txt",
+				"testing/test/tested/file2.txt",
+			},
+			expect: []string{},
+		},
+		{
+			description: "Match in test root",
+			patterns:    []string{"/test/*"},
+			files: []string{
+				"file1.txt",
+				"testing/test/file2.txt",
+				"testing/test/tested/file2.txt",
+				"/test/file3.txt",
+			},
+			expect: []string{
+				"/test/file3.txt",
 			},
-			expect: true,
 		},
 		{
 			description: "match multiple files",
 			patterns:    []string{"ci/dockerfiles/**/*", "ci/dockerfiles/*", "ci/tasks/build-image.yml", "ci/pipelines/images.yml"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"ci/Makefile",
-					"ci/pipelines/images.yml",
-					"terraform/Makefile",
-				},
+			files: []string{
+				"ci/Makefile",
+				"ci/pipelines/images.yml",
+				"terraform/Makefile",
+			},
+			expect: []string{
+				"ci/pipelines/images.yml",
 			},
-			expect: true,
 		},
 		{
 			description: "no match /**/*",
 			patterns:    []string{"/ci/**/*"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"src/teams/myteam/README.md",
-					"src/teams/myteam/ci/pipeline.yml",
-				},
+			files: []string{
+				"src/teams/myteam/README.md",
+				"src/teams/myteam/ci/pipeline.yml",
 			},
-			expect: false,
+			expect: []string{},
 		},
 		{
 			description: "match /**/*",
 			patterns:    []string{"/ci/**/*"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"ci/README.md",
-					"ci/pipeline.yml",
-				},
+			files: []string{
+				"ci/README.md",
+				"ci/pipeline.yml",
+			},
+			expect: []string{
+				"ci/README.md",
+				"ci/pipeline.yml",
 			},
-			expect: true,
 		},
 		{
 			description: "match **/*",
 			patterns:    []string{"src/teams/myteam/**/*"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"src/teams/myteam/README.md",
-					"src/teams/myteam/ci/pipeline.yml",
-				},
+			files: []string{
+				"src/teams/myteam/README.md",
+				"src/teams/myteam/ci/pipeline.yml",
+			},
+			expect: []string{
+				"src/teams/myteam/README.md",
+				"src/teams/myteam/ci/pipeline.yml",
 			},
-			expect: true,
 		},
 		{
 			description: "match multiple files 2",
 			patterns:    []string{"src/teams/myteam/**/*", "src/teams/myteam/*"},
-			invert:      false,
-			pull: pullrequest.PullRequest{
-				Files: []string{
-					"src/teams/myteam/README.md",
-					"src/teams/myteam/ci/pipeline.yml",
-				},
+			files: []string{
+				"src/teams/myteam/README.md",
+				"src/teams/myteam/ci/pipeline.yml",
+			},
+			expect: []string{
+				"src/teams/myteam/README.md",
+				"src/teams/myteam/ci/pipeline.yml",
+			},
+		},
+		{
+			description: "match paths ending in file separator",
+			patterns:    []string{"src/teams/myteam/Dockerfile", "src/teams/myteam/ci/", "src/teams/myteam/app/"},
+			files: []string{
+				"src/teams/myteam/ci/README.md",
+				"src/teams/myteam/ci/main.go",
+				"src/teams/yourteam/temp.go",
+			},
+			expect: []string{
+				"src/teams/myteam/ci/README.md",
+				"src/teams/myteam/ci/main.go",
 			},
-			expect: true,
 		},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.description, func(t *testing.T) {
-			out := pullrequest.Files(tc.patterns, tc.invert)(tc.pull)
+			out, err := pullrequest.Files(tc.patterns)(tc.files)
+			assert.Nil(t, err)
 			assert.Equal(t, tc.expect, out)
 		})
 	}