diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index acb9f176d8..bf63d1727c 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -866,8 +866,8 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) rebaseName := options.RebaseNames[include] var ( - parentMatched []bool - parentDirs []string + parentMatchInfo []fileutils.MatchInfo + parentDirs []string ) walkRoot := getWalkRoot(srcPath, include) @@ -902,13 +902,14 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) break } parentDirs = parentDirs[:len(parentDirs)-1] - parentMatched = parentMatched[:len(parentMatched)-1] + parentMatchInfo = parentMatchInfo[:len(parentMatchInfo)-1] } - if len(parentMatched) != 0 { - skip, err = pm.MatchesUsingParentResult(relFilePath, parentMatched[len(parentMatched)-1]) + var matchInfo fileutils.MatchInfo + if len(parentMatchInfo) != 0 { + skip, matchInfo, err = pm.MatchesUsingParentResults(relFilePath, parentMatchInfo[len(parentMatchInfo)-1]) } else { - skip, err = pm.MatchesOrParentMatches(relFilePath) + skip, matchInfo, err = pm.MatchesUsingParentResults(relFilePath, fileutils.MatchInfo{}) } if err != nil { logrus.Errorf("Error matching %s: %v", relFilePath, err) @@ -917,7 +918,7 @@ func TarWithOptions(srcPath string, options *TarOptions) (io.ReadCloser, error) if f.IsDir() { parentDirs = append(parentDirs, relFilePath) - parentMatched = append(parentMatched, skip) + parentMatchInfo = append(parentMatchInfo, matchInfo) } } diff --git a/pkg/fileutils/fileutils.go b/pkg/fileutils/fileutils.go index dac522ccb1..4c495fddfc 100644 --- a/pkg/fileutils/fileutils.go +++ b/pkg/fileutils/fileutils.go @@ -84,9 +84,9 @@ func NewPatternMatcher(patterns []string) (*PatternMatcher, error) { // // 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. +// Deprecated: This implementation is buggy (it only checks a single parent dir +// against the pattern) and will be removed soon. Use either +// MatchesOrParentMatches or MatchesUsingParentResults instead. func (pm *PatternMatcher) Matches(file string) (bool, error) { matched := false file = filepath.FromSlash(file) @@ -172,6 +172,11 @@ func (pm *PatternMatcher) MatchesOrParentMatches(file string) (bool, error) { // The "file" argument should be a slash-delimited path. // // MatchesUsingParentResult is not safe to call concurrently. +// +// Deprecated: this function does behave correctly in some cases (see +// https://github.com/docker/buildx/issues/850). +// +// Use MatchesUsingParentResults instead. func (pm *PatternMatcher) MatchesUsingParentResult(file string, parentMatched bool) (bool, error) { matched := parentMatched file = filepath.FromSlash(file) @@ -196,6 +201,78 @@ func (pm *PatternMatcher) MatchesUsingParentResult(file string, parentMatched bo return matched, nil } +// MatchInfo tracks information about parent dir matches while traversing a +// filesystem. +type MatchInfo struct { + parentMatched []bool +} + +// MatchesUsingParentResults 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 passes in +// intermediate results from matching the parent directory. +// +// The "file" argument should be a slash-delimited path. +// +// MatchesUsingParentResults is not safe to call concurrently. +func (pm *PatternMatcher) MatchesUsingParentResults(file string, parentMatchInfo MatchInfo) (bool, MatchInfo, error) { + parentMatched := parentMatchInfo.parentMatched + if len(parentMatched) != 0 && len(parentMatched) != len(pm.patterns) { + return false, MatchInfo{}, errors.New("wrong number of values in parentMatched") + } + + file = filepath.FromSlash(file) + matched := false + + matchInfo := MatchInfo{ + parentMatched: make([]bool, len(pm.patterns)), + } + for i, pattern := range pm.patterns { + match := false + // If the parent matched this pattern, we don't need to recheck. + if len(parentMatched) != 0 { + match = parentMatched[i] + } + + if !match { + // 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 + } + + var err error + match, err = pattern.match(file) + if err != nil { + return false, matchInfo, err + } + + // If the zero value of MatchInfo was passed in, we don't have + // any information about the parent dir's match results, and we + // apply the same logic as MatchesOrParentMatches. + if len(parentMatched) == 0 { + if parentPath := filepath.Dir(file); parentPath != "." { + parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) + // 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 + } + } + } + } + } + matchInfo.parentMatched[i] = match + + if match { + matched = !pattern.exclusion + } + } + return matched, matchInfo, nil +} + // Exclusions returns true if any of the patterns define exclusions func (pm *PatternMatcher) Exclusions() bool { return pm.exclusions diff --git a/pkg/fileutils/fileutils_test.go b/pkg/fileutils/fileutils_test.go index 2d66b431d8..3e3c498a10 100644 --- a/pkg/fileutils/fileutils_test.go +++ b/pkg/fileutils/fileutils_test.go @@ -309,6 +309,12 @@ type matchesTestCase struct { pass bool } +type multiPatternTestCase struct { + patterns []string + text string + pass bool +} + func TestMatches(t *testing.T) { tests := []matchesTestCase{ {"**", "file", true}, @@ -377,6 +383,10 @@ func TestMatches(t *testing.T) { {"a(b)c/def", "a(b)c/xyz", false}, {"a.|)$(}+{bc", "a.|)$(}+{bc", true}, } + multiPatternTests := []multiPatternTestCase{ + {[]string{"**", "!util/docker/web"}, "util/docker/web/foo", false}, + {[]string{"**", "!util/docker/web", "util/docker/web/foo"}, "util/docker/web/foo", true}, + } if runtime.GOOS != "windows" { tests = append(tests, []matchesTestCase{ @@ -392,6 +402,14 @@ func TestMatches(t *testing.T) { res, _ := pm.MatchesOrParentMatches(test.text) assert.Check(t, is.Equal(test.pass, res), desc) } + + for _, test := range multiPatternTests { + desc := fmt.Sprintf("patterns=%q text=%q", test.patterns, test.text) + pm, err := NewPatternMatcher(test.patterns) + 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) { @@ -415,6 +433,38 @@ func TestMatches(t *testing.T) { } }) + t.Run("MatchesUsingParentResults", func(t *testing.T) { + check := func(pm *PatternMatcher, text string, pass bool, desc string) { + parentPath := filepath.Dir(filepath.FromSlash(text)) + parentPathDirs := strings.Split(parentPath, string(os.PathSeparator)) + + parentMatchInfo := MatchInfo{} + if parentPath != "." { + for i := range parentPathDirs { + _, parentMatchInfo, _ = pm.MatchesUsingParentResults(strings.Join(parentPathDirs[:i+1], "/"), parentMatchInfo) + } + } + + res, _, _ := pm.MatchesUsingParentResults(text, parentMatchInfo) + assert.Check(t, is.Equal(pass, res), desc) + } + + 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) + + check(pm, test.text, test.pass, desc) + } + + for _, test := range multiPatternTests { + desc := fmt.Sprintf("pattern=%q text=%q", test.patterns, test.text) + pm, err := NewPatternMatcher(test.patterns) + assert.NilError(t, err, desc) + + check(pm, test.text, test.pass, desc) + } + }) } func TestCleanPatterns(t *testing.T) {