Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(pattern-matcher): add CanSkipDir method in patternMatcher #1656

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 35 additions & 9 deletions pkg/fileutils/fileutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func NewPatternMatcher(patterns []string) (*PatternMatcher, error) {
pm.exclusions = true
}
// Do some syntax checking on the pattern.
// filepath's Match() has some really weird rules that are inconsistent
// filepath.Match() has some really weird rules that are inconsistent
// so instead of trying to dup their logic, just call Match() for its
// error state and if there is an error in the pattern return it.
// If this becomes an issue we can remove this since its really only
Expand Down Expand Up @@ -89,11 +89,11 @@ func (pm *PatternMatcher) Matches(file string) (bool, error) {
}

type MatchResult struct {
isMatched bool
matches, excludes uint
BlackHole1 marked this conversation as resolved.
Show resolved Hide resolved
isMatched, canSkipDir bool
matches, excludes uint
}

// Excludes returns true if the overall result is matched
// IsMatched returns true if the overall result is matched
func (m *MatchResult) IsMatched() bool {
return m.isMatched
}
Expand All @@ -108,12 +108,20 @@ func (m *MatchResult) Excludes() uint {
return m.excludes
}

// CanSkipDir returns true if it is possible to use filepath.SkipDir
// Only when the pattern is very simple (does not start with ! and does not contain ? and *)
// and is not influenced by other patterns, will it return true.
func (m *MatchResult) CanSkipDir() bool {
return m.canSkipDir
}

// MatchesResult verifies the provided filepath against all patterns.
// It returns the `*MatchResult` result for the patterns on success, otherwise
// an error. This method is not safe to be called concurrently.
func (pm *PatternMatcher) MatchesResult(file string) (res *MatchResult, err error) {
file = filepath.FromSlash(file)
res = &MatchResult{false, 0, 0}
res = &MatchResult{false, false, 0, 0}
matchPatterns := make([]*Pattern, 0, len(pm.patterns))

for _, pattern := range pm.patterns {
negative := false
Expand All @@ -129,16 +137,34 @@ func (pm *PatternMatcher) MatchesResult(file string) (res *MatchResult, err erro

if match {
res.isMatched = !negative
if negative {
res.excludes++
} else {
if res.isMatched {
res.matches++
matchPatterns = append(matchPatterns, pattern)
} else {
res.excludes++
}
}
}

if res.matches > 0 {
if res.isMatched {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous implementation had a bug. When the rule is ["1", "!1"], it would also enter this conditional statement, but it should not.

logrus.Debugf("Skipping excluded path: %s", file)

isPatternsEqual := true
first := matchPatterns[0].String()
if len(matchPatterns) != 1 {
for _, pattern := range matchPatterns[1:] {
if first != pattern.String() {
isPatternsEqual = false
break
}
}
}

if isPatternsEqual {
if !strings.Contains(first, "*") && !strings.Contains(first, "?") {
res.canSkipDir = true
}
}
}

return res, nil
Expand Down
35 changes: 19 additions & 16 deletions pkg/fileutils/fileutils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ func TestMatches(t *testing.T) {
assert.Equal(t, err, filepath.ErrBadPattern)
} else {
assert.Nil(t, err)
assert.Equal(t, test.match, res.isMatched, desc)
assert.Equal(t, test.match, res.IsMatched(), desc)
}
}
})
Expand Down Expand Up @@ -591,22 +591,24 @@ func TestMatch(t *testing.T) {

func TestMatchesAmount(t *testing.T) {
testData := []struct {
patterns []string
input string
matches uint
excludes uint
isMatch bool
patterns []string
input string
matches uint
excludes uint
isMatch bool
canSkipDir bool
}{
{[]string{"1", "2", "3"}, "2", 1, 0, true},
{[]string{"2", "!2", "!2"}, "2", 1, 2, false},
{[]string{"1", "2", "2"}, "2", 2, 0, true},
{[]string{"1", "2", "2", "2"}, "2", 3, 0, true},
{[]string{"/prefix/path", "/prefix/other"}, "/prefix/path", 1, 0, true},
{[]string{"/prefix*", "!/prefix/path"}, "/prefix/match", 1, 0, true},
{[]string{"/prefix*", "!/prefix/path"}, "/prefix/path", 1, 0, true},
{[]string{"/prefix*", "!/prefix/path"}, "prefix/path", 0, 1, false},
{[]string{"/prefix*", "!./prefix/path"}, "prefix/path", 0, 1, false},
{[]string{"/prefix*", "!prefix/path"}, "prefix/path", 0, 1, false},
{[]string{"1", "2", "3"}, "2", 1, 0, true, true},
{[]string{"!1", "1"}, "1", 1, 1, true, true},
{[]string{"2", "!2", "!2"}, "2", 1, 2, false, false},
{[]string{"1", "2", "2"}, "2", 2, 0, true, true},
{[]string{"1", "2", "2", "2"}, "2", 3, 0, true, true},
{[]string{"/prefix/path", "/prefix/other"}, "/prefix/path", 1, 0, true, true},
{[]string{"/prefix*", "!/prefix/path"}, "/prefix/match", 1, 0, true, false},
{[]string{"/prefix*", "!/prefix/path"}, "/prefix/path", 1, 0, true, false},
{[]string{"/prefix*", "!/prefix/path"}, "prefix/path", 0, 1, false, false},
{[]string{"/prefix*", "!./prefix/path"}, "prefix/path", 0, 1, false, false},
{[]string{"/prefix*", "!prefix/path"}, "prefix/path", 0, 1, false, false},
}

for _, testCase := range testData {
Expand All @@ -618,6 +620,7 @@ func TestMatchesAmount(t *testing.T) {
assert.Equal(t, testCase.excludes, res.Excludes(), desc)
assert.Equal(t, testCase.matches, res.Matches(), desc)
assert.Equal(t, testCase.isMatch, res.IsMatched(), desc)
assert.Equal(t, testCase.canSkipDir, res.CanSkipDir(), desc)

isMatch, err := pm.IsMatch(testCase.input)
require.NoError(t, err)
Expand Down