Merge pull request #42676 from aaronlehmann/patternmatcher-doublestar-bug

fileutils: Fix incorrect handling of "**/foo" pattern

builder/remotecontext/detect.go

@@ -130,7 +130,7 @@ func removeDockerfile(c modifiableContext, filesToRemove ...string) error {
 	f.Close()
 	filesToRemove = append([]string{".dockerignore"}, filesToRemove...)
 	for _, fileToRemove := range filesToRemove {
-		if rm, _ := fileutils.Matches(fileToRemove, excludes); rm {
+		if rm, _ := fileutils.MatchesOrParentMatches(fileToRemove, excludes); rm {
 			if err := c.Remove(fileToRemove); err != nil {
 				logrus.Errorf("failed to remove %s: %v", fileToRemove, err)
 			}

pkg/archive/archive.go

@@ -808,6 +808,11 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error)
 		for _, include := range options.IncludeFiles {
 			rebaseName := options.RebaseNames[include]
 
+			var (
+				parentMatched []bool
+				parentDirs    []string
+			)
+
 			walkRoot := getWalkRoot(srcPath, include)
 			filepath.Walk(walkRoot, func(filePath string, f os.FileInfo, err error) error {
 				if err != nil {
@@ -834,11 +839,29 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error)
 				// is asking for that file no matter what - which is true
 				// for some files, like .dockerignore and Dockerfile (sometimes)
 				if include != relFilePath {
-					skip, err = pm.Matches(relFilePath)
+					for len(parentDirs) != 0 {
+						lastParentDir := parentDirs[len(parentDirs)-1]
+						if strings.HasPrefix(relFilePath, lastParentDir+string(os.PathSeparator)) {
+							break
+						}
+						parentDirs = parentDirs[:len(parentDirs)-1]
+						parentMatched = parentMatched[:len(parentMatched)-1]
+					}
+
+					if len(parentMatched) != 0 {
+						skip, err = pm.MatchesUsingParentResult(relFilePath, parentMatched[len(parentMatched)-1])
+					} else {
+						skip, err = pm.MatchesOrParentMatches(relFilePath)
+					}
 					if err != nil {
 						logrus.Errorf("Error matching %s: %v", relFilePath, err)
 						return err
 					}
+
+					if f.IsDir() {
+						parentDirs = append(parentDirs, relFilePath)
+						parentMatched = append(parentMatched, skip)
+					}
 				}
 
 				if skip {

pkg/fileutils/fileutils.go

@@ -55,8 +55,16 @@ func NewPatternMatcher(patterns []string) (*PatternMatcher, error) {
 	return pm, nil
 }
 
-// Matches matches path against all the patterns. Matches is not safe to be
-// called concurrently
+// Matches returns true if "file" matches any of the patterns
+// and isn't excluded by any of the subsequent patterns.
+//
+// The "file" argument should be a slash-delimited path.
+//
+// Matches is not safe to call concurrently.
+//
+// This implementation is buggy (it only checks a single parent dir against the
+// pattern) and will be removed soon. Use either MatchesOrParentMatches or
+// MatchesUsingParentResult instead.
 func (pm *PatternMatcher) Matches(file string) (bool, error) {
 	matched := false
 	file = filepath.FromSlash(file)
@@ -64,10 +72,11 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) {
 	parentPathDirs := strings.Split(parentPath, string(os.PathSeparator))
 
 	for _, pattern := range pm.patterns {
-		negative := false
-
-		if pattern.exclusion {
-			negative = true
+		// Skip evaluation if this is an inclusion and the filename
+		// already matched the pattern, or it's an exclusion and it has
+		// not matched the pattern yet.
+		if pattern.exclusion != matched {
+			continue
 		}
 
 		match, err := pattern.match(file)
@@ -83,13 +92,88 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) {
 		}
 
 		if match {
-			matched = !negative
+			matched = !pattern.exclusion
+		}
+	}
+
+	return matched, nil
+}
+
+// MatchesOrParentMatches returns true if "file" matches any of the patterns
+// and isn't excluded by any of the subsequent patterns.
+//
+// The "file" argument should be a slash-delimited path.
+//
+// Matches is not safe to call concurrently.
+func (pm *PatternMatcher) MatchesOrParentMatches(file string) (bool, error) {
+	matched := false
+	file = filepath.FromSlash(file)
+	parentPath := filepath.Dir(file)
+	parentPathDirs := strings.Split(parentPath, string(os.PathSeparator))
+
+	for _, pattern := range pm.patterns {
+		// Skip evaluation if this is an inclusion and the filename
+		// already matched the pattern, or it's an exclusion and it has
+		// not matched the pattern yet.
+		if pattern.exclusion != matched {
+			continue
+		}
+
+		match, err := pattern.match(file)
+		if err != nil {
+			return false, err
+		}
+
+		if !match && parentPath != "." {
+			// Check to see if the pattern matches one of our parent dirs.
+			for i := range parentPathDirs {
+				match, _ = pattern.match(strings.Join(parentPathDirs[:i+1], string(os.PathSeparator)))
+				if match {
+					break
+				}
+			}
+		}
+
+		if match {
+			matched = !pattern.exclusion
 		}
 	}
 
 	return matched, nil
 }
 
+// MatchesUsingParentResult returns true if "file" matches any of the patterns
+// and isn't excluded by any of the subsequent patterns. The functionality is
+// the same as Matches, but as an optimization, the caller keeps track of
+// whether the parent directory matched.
+//
+// The "file" argument should be a slash-delimited path.
+//
+// MatchesUsingParentResult is not safe to call concurrently.
+func (pm *PatternMatcher) MatchesUsingParentResult(file string, parentMatched bool) (bool, error) {
+	matched := parentMatched
+	file = filepath.FromSlash(file)
+
+	for _, pattern := range pm.patterns {
+		// Skip evaluation if this is an inclusion and the filename
+		// already matched the pattern, or it's an exclusion and it has
+		// not matched the pattern yet.
+		if pattern.exclusion != matched {
+			continue
+		}
+
+		match, err := pattern.match(file)
+		if err != nil {
+			return false, err
+		}
+
+		if match {
+			matched = !pattern.exclusion
+		}
+	}
+	return matched, nil
+}
+
 // Exclusions returns true if any of the patterns define exclusions
 func (pm *PatternMatcher) Exclusions() bool {
 	return pm.exclusions
@@ -118,7 +202,6 @@ func (p *Pattern) Exclusion() bool {
 }
 
 func (p *Pattern) match(path string) (bool, error) {
-
 	if p.regexp == nil {
 		if err := p.compile(); err != nil {
 			return false, filepath.ErrBadPattern
@@ -210,6 +293,9 @@ func (p *Pattern) compile() error {
 
 // Matches returns true if file matches any of the patterns
 // and isn't excluded by any of the subsequent patterns.
+//
+// This implementation is buggy (it only checks a single parent dir against the
+// pattern) and will be removed soon. Use MatchesOrParentMatches instead.
 func Matches(file string, patterns []string) (bool, error) {
 	pm, err := NewPatternMatcher(patterns)
 	if err != nil {
@@ -225,6 +311,23 @@ func Matches(file string, patterns []string) (bool, error) {
 	return pm.Matches(file)
 }
 
+// MatchesOrParentMatches returns true if file matches any of the patterns
+// and isn't excluded by any of the subsequent patterns.
+func MatchesOrParentMatches(file string, patterns []string) (bool, error) {
+	pm, err := NewPatternMatcher(patterns)
+	if err != nil {
+		return false, err
+	}
+	file = filepath.Clean(file)
+
+	if file == "." {
+		// Don't let them exclude everything, kind of silly.
+		return false, nil
+	}
+
+	return pm.MatchesOrParentMatches(file)
+}
+
 // CopyFile copies from src to dst until either EOF is reached
 // on src or an error occurs. It verifies src exists and removes
 // the dst if it exists.

pkg/fileutils/fileutils_test.go

@@ -328,6 +328,8 @@ func TestMatches(t *testing.T) {
 		{"dir/**", "dir/file/", true},
 		{"dir/**", "dir/dir2/file", true},
 		{"dir/**", "dir/dir2/file/", true},
+		{"**/dir", "dir", true},
+		{"**/dir", "dir/file", true},
 		{"**/dir2/*", "dir/dir2/file", true},
 		{"**/dir2/*", "dir/dir2/file/", true},
 		{"**/dir2/**", "dir/dir2/dir3/file", true},
@@ -380,13 +382,37 @@ func TestMatches(t *testing.T) {
 		}...)
 	}
 
-	for _, test := range tests {
-		desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text)
-		pm, err := NewPatternMatcher([]string{test.pattern})
-		assert.NilError(t, err, desc)
-		res, _ := pm.Matches(test.text)
-		assert.Check(t, is.Equal(test.pass, res), desc)
-	}
+	t.Run("MatchesOrParentMatches", func(t *testing.T) {
+		for _, test := range tests {
+			desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text)
+			pm, err := NewPatternMatcher([]string{test.pattern})
+			assert.NilError(t, err, desc)
+			res, _ := pm.MatchesOrParentMatches(test.text)
+			assert.Check(t, is.Equal(test.pass, res), desc)
+		}
+	})
+
+	t.Run("MatchesUsingParentResult", func(t *testing.T) {
+		for _, test := range tests {
+			desc := fmt.Sprintf("pattern=%q text=%q", test.pattern, test.text)
+			pm, err := NewPatternMatcher([]string{test.pattern})
+			assert.NilError(t, err, desc)
+
+			parentPath := filepath.Dir(filepath.FromSlash(test.text))
+			parentPathDirs := strings.Split(parentPath, string(os.PathSeparator))
+
+			parentMatched := false
+			if parentPath != "." {
+				for i := range parentPathDirs {
+					parentMatched, _ = pm.MatchesUsingParentResult(strings.Join(parentPathDirs[:i+1], "/"), parentMatched)
+				}
+			}
+
+			res, _ := pm.MatchesUsingParentResult(test.text, parentMatched)
+			assert.Check(t, is.Equal(test.pass, res), desc)
+		}
+	})
+
 }
 
 func TestCleanPatterns(t *testing.T) {