Merge pull request #248 from colincross/match

Simplify pathtools.Match

pathtools/fs.go

@@ -61,6 +61,9 @@ func MockFs(files map[string][]byte) FileSystem {
 		fs.dirs[dir] = true
 	}
 
+	fs.dirs["."] = true
+	fs.dirs["/"] = true
+
 	for f := range fs.files {
 		fs.all = append(fs.all, f)
 	}
@@ -330,8 +333,8 @@ func (m *mockFs) glob(pattern string) ([]string, error) {
 			if err != nil {
 				return nil, err
 			}
-			if f == "." && f != pattern {
-				// filepath.Glob won't return "." unless the pattern was "."
+			if (f == "." || f == "/") && f != pattern {
+				// filepath.Glob won't return "." or "/" unless the pattern was "." or "/"
 				match = false
 			}
 			if match {

pathtools/glob.go

@@ -25,8 +25,9 @@ import (
 	"github.com/google/blueprint/deptools"
 )
 
-var GlobMultipleRecursiveErr = errors.New("pattern contains multiple **")
-var GlobLastRecursiveErr = errors.New("pattern ** as last path element")
+var GlobMultipleRecursiveErr = errors.New("pattern contains multiple '**'")
+var GlobLastRecursiveErr = errors.New("pattern has '**' as last path element")
+var GlobInvalidRecursiveErr = errors.New("pattern contains other characters between '**' and path separator")
 
 // Glob returns the list of files and directories that match the given pattern
 // but do not match the given exclude patterns, along with the list of
@@ -118,7 +119,7 @@ func glob(fs FileSystem, pattern string, hasRecursive bool,
 			// as a dependency.
 			var matchDirs []string
 			for len(matchDirs) == 0 {
-				pattern, _ = saneSplit(pattern)
+				pattern = filepath.Dir(pattern)
 				matchDirs, err = fs.glob(pattern)
 				if err != nil {
 					return matches, dirs, err
@@ -136,6 +137,8 @@ func glob(fs FileSystem, pattern string, hasRecursive bool,
 			return matches, dirs, GlobMultipleRecursiveErr
 		}
 		hasRecursive = true
+	} else if strings.Contains(file, "**") {
+		return matches, dirs, GlobInvalidRecursiveErr
 	}
 
 	dirMatches, dirs, err := glob(fs, dir, hasRecursive, follow)
@@ -242,7 +245,7 @@ func filterDotFiles(matches []string) []string {
 }
 
 // Match returns true if name matches pattern using the same rules as filepath.Match, but supporting
-// hierarchical patterns (a/*) and recursive globs (**).
+// recursive globs (**).
 func Match(pattern, name string) (bool, error) {
 	if filepath.Base(pattern) == "**" {
 		return false, GlobLastRecursiveErr
@@ -262,16 +265,35 @@ func Match(pattern, name string) (bool, error) {
 
 	for {
 		var patternFile, nameFile string
-		pattern, patternFile = saneSplit(pattern)
-		name, nameFile = saneSplit(name)
+		pattern, patternFile = filepath.Dir(pattern), filepath.Base(pattern)
 
 		if patternFile == "**" {
-			return matchPrefix(pattern, filepath.Join(name, nameFile))
+			if strings.Contains(pattern, "**") {
+				return false, GlobMultipleRecursiveErr
+			}
+			// Test if the any prefix of name matches the part of the pattern before **
+			for {
+				if name == "." || name == "/" {
+					return name == pattern, nil
+				}
+				if match, err := filepath.Match(pattern, name); err != nil {
+					return false, err
+				} else if match {
+					return true, nil
+				}
+				name = filepath.Dir(name)
+			}
+		} else if strings.Contains(patternFile, "**") {
+			return false, GlobInvalidRecursiveErr
 		}
 
-		if nameFile == "" && patternFile == "" {
+		name, nameFile = filepath.Dir(name), filepath.Base(name)
+
+		if nameFile == "." && patternFile == "." {
 			return true, nil
-		} else if nameFile == "" || patternFile == "" {
+		} else if nameFile == "/" && patternFile == "/" {
+			return true, nil
+		} else if nameFile == "." || patternFile == "." || nameFile == "/" || patternFile == "/" {
 			return false, nil
 		}
 
@@ -282,56 +304,6 @@ func Match(pattern, name string) (bool, error) {
 	}
 }
 
-// matchPrefix returns true if the beginning of name matches pattern using the same rules as
-// filepath.Match, but supporting hierarchical patterns (a/*).  Recursive globs (**) are not
-// supported, they should have been handled in Match().
-func matchPrefix(pattern, name string) (bool, error) {
-	if len(pattern) > 0 && pattern[0] == '/' {
-		if len(name) > 0 && name[0] == '/' {
-			pattern = pattern[1:]
-			name = name[1:]
-		} else {
-			return false, nil
-		}
-	}
-
-	for {
-		var patternElem, nameElem string
-		patternElem, pattern = saneSplitFirst(pattern)
-		nameElem, name = saneSplitFirst(name)
-
-		if patternElem == "." {
-			patternElem = ""
-		}
-		if nameElem == "." {
-			nameElem = ""
-		}
-
-		if patternElem == "**" {
-			return false, GlobMultipleRecursiveErr
-		}
-
-		if patternElem == "" {
-			return true, nil
-		} else if nameElem == "" {
-			return false, nil
-		}
-
-		match, err := filepath.Match(patternElem, nameElem)
-		if err != nil || !match {
-			return match, err
-		}
-	}
-}
-
-func saneSplitFirst(path string) (string, string) {
-	i := strings.IndexRune(path, filepath.Separator)
-	if i < 0 {
-		return path, ""
-	}
-	return path[:i], path[i+1:]
-}
-
 func GlobPatternList(patterns []string, prefix string) (globedList []string, depDirs []string, err error) {
 	var (
 		matches []string

pathtools/glob_test.go

@@ -18,6 +18,7 @@ import (
 	"os"
 	"path/filepath"
 	"reflect"
+	"strings"
 	"testing"
 )
 
@@ -215,6 +216,14 @@ var globTestCases = []globTestCase{
 		pattern: "**/**",
 		err:     GlobLastRecursiveErr,
 	},
+	{
+		pattern: "a**/",
+		err:     GlobInvalidRecursiveErr,
+	},
+	{
+		pattern: "**a/",
+		err:     GlobInvalidRecursiveErr,
+	},
 
 	// exclude tests
 	{
@@ -778,6 +787,16 @@ func TestMatch(t *testing.T) {
 		{"a/**/*/", "a/b/", true},
 		{"a/**/*/", "a/b/c", false},
 
+		{"**/*", "a/", false},
+		{"**/*", "a/a", true},
+		{"**/*", "a/b/", false},
+		{"**/*", "a/b/c", true},
+
+		{"**/*/", "a/", true},
+		{"**/*/", "a/a", false},
+		{"**/*/", "a/b/", true},
+		{"**/*/", "a/b/c", false},
+
 		{`a/\*\*/\*`, `a/**/*`, true},
 		{`a/\*\*/\*`, `a/a/*`, false},
 		{`a/\*\*/\*`, `a/**/a`, false},
@@ -826,6 +845,38 @@ func TestMatch(t *testing.T) {
 		{`a/?`, `a/a`, true},
 		{`a/\?`, `a/?`, true},
 		{`a/\?`, `a/a`, false},
+
+		{"/a/*", "/a/", false},
+		{"/a/*", "/a/a", true},
+		{"/a/*", "/a/b/", false},
+		{"/a/*", "/a/b/c", false},
+
+		{"/a/*/", "/a/", false},
+		{"/a/*/", "/a/a", false},
+		{"/a/*/", "/a/b/", true},
+		{"/a/*/", "/a/b/c", false},
+
+		{"/a/**/*", "/a/", false},
+		{"/a/**/*", "/a/a", true},
+		{"/a/**/*", "/a/b/", false},
+		{"/a/**/*", "/a/b/c", true},
+
+		{"/**/*", "/a/", false},
+		{"/**/*", "/a/a", true},
+		{"/**/*", "/a/b/", false},
+		{"/**/*", "/a/b/c", true},
+
+		{"/**/*/", "/a/", true},
+		{"/**/*/", "/a/a", false},
+		{"/**/*/", "/a/b/", true},
+		{"/**/*/", "/a/b/c", false},
+
+		{`a`, `/a`, false},
+		{`/a`, `a`, false},
+		{`*`, `/a`, false},
+		{`/*`, `a`, false},
+		{`**/*`, `/a`, false},
+		{`/**/*`, `a`, false},
 	}
 
 	for _, test := range testCases {
@@ -839,4 +890,36 @@ func TestMatch(t *testing.T) {
 			}
 		})
 	}
+
+	// Run the same test cases through Glob
+	for _, test := range testCases {
+		// Glob and Match disagree on matching directories
+		if strings.HasSuffix(test.name, "/") || strings.HasSuffix(test.pattern, "/") {
+			continue
+		}
+		t.Run("glob:"+test.pattern+","+test.name, func(t *testing.T) {
+			mockFiles := map[string][]byte{
+				test.name: nil,
+			}
+
+			mock := MockFs(mockFiles)
+
+			matches, _, err := mock.Glob(test.pattern, nil, DontFollowSymlinks)
+			t.Log(test.name, test.pattern, matches)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			match := false
+			for _, x := range matches {
+				if x == test.name {
+					match = true
+				}
+			}
+
+			if match != test.match {
+				t.Errorf("want: %v, got %v", test.match, match)
+			}
+		})
+	}
 }