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.

Sign up for GitHub

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

filter: fix a bug with parent dirs #203

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

filter: fix a bug with parent dirs #203

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions filter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,24 @@ type MapFunc func(string, *types.Stat) MapResult
type MapResult int

const (
// Keep the current path and continue.
// Keep the current path and continue walking the file tree.
MapResultKeep MapResult = iota

// Exclude the current path and continue.
// Exclude the current path and continue walking the file tree.
//
// If path is a dir, we'll continue walking the subtree of the directory.
// If a path in that subtree is later included during the walk, the walker
// will backtrack and include the dir.
MapResultExclude

// Exclude the current path, and skip the rest of the dir.
//
// Skip is used as an important performance optimization to prevent
// the walker from visiting large subtrees that it doesn't need to.
//
// If path is a dir, skip the current directory.
// If path is a file, skip the rest of the parent directory.
//
// (This matches the semantics of fs.SkipDir.)
MapResultSkipDir
)
Expand Down Expand Up @@ -197,7 +206,7 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
isDir = dirEntry.IsDir()
}

if fs.includeMatcher != nil || fs.excludeMatcher != nil {
if fs.includeMatcher != nil || fs.excludeMatcher != nil || fs.mapFn != nil {
Copy link
Owner

Choose a reason for hiding this comment

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

One thing I'm not a big fan of is that everything that gets a map goes to this more complicated code-path to support the backtracking, while most of the time the map is really used to reset uid/gid or skip paths recursively.

Would be really ugly, but smth. like MapAllowBacktrack bool would solve that.

I don't know how big issue this is in practice though. It might be that most of our requests set includematches/excludematcher anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

is your concern more about performance or debuggability?

i'd be in favor of adding a much simpler FS implementation purely focused on the reset uid/gid case.

Copy link
Owner

Choose a reason for hiding this comment

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

Performance a bit maybe, but mostly keeping the default code path simpler and preventing potential stability issues.

for len(parentDirs) != 0 {
lastParentDir := parentDirs[len(parentDirs)-1].pathWithSep
if strings.HasPrefix(path, lastParentDir) {
Expand Down Expand Up @@ -298,7 +307,7 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
return walkErr
}

if fs.includeMatcher != nil || fs.excludeMatcher != nil {
if fs.includeMatcher != nil || fs.excludeMatcher != nil || fs.mapFn != nil {
defer func() {
if isDir {
parentDirs = append(parentDirs, dir)
Expand All @@ -310,8 +319,6 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
return nil
}

dir.calledFn = true

fi, err := dirEntry.Info()
if err != nil {
return err
Expand Down Expand Up @@ -357,7 +364,9 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
if fs.mapFn != nil {
result := fs.mapFn(parentStat.Path, parentStat)
if result == MapResultExclude {
continue
// If a directory is marked excluded, but a subpath is included,
// then we should still include the directory.
// Otherwise Buildkit will treat this as an error.
} else if result == MapResultSkipDir {
parentDirs[i].skipFn = true
return filepath.SkipDir
Expand All @@ -372,6 +381,8 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
return err
}
}

dir.calledFn = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

👀 this feels unrelated, were we not setting this before? Was there a case where we were calling the walk target function multiple times? Kinda confused as to why this shows up just here (definitely worth fixing).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ya, i moved this line.

Before this pr, the logic went:

  • check if the directory is skipped by includePatterns or excludePatterns
  • set calledFn to true
  • check if the directory is skipped by mapFn
  • call fn

now, i only set calledFn to true directly before calling fn

if err := fn(stat.Path, &DirEntryInfo{Stat: stat}, nil); err != nil {
return err
}
Expand Down
60 changes: 60 additions & 0 deletions filter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,66 @@ file y/b.txt
`), b.String())
}

// Same as TestWalkerMapSkipDirWithPattern, but
// with a Map() function that returns Exclude instead of SkipDir.
func TestWalkerMapExcludeDirWithPattern(t *testing.T) {
d, err := tmpDir(changeStream([]string{
"ADD x dir",
"ADD x/a.txt file",
"ADD y dir",
"ADD y/b.txt file",
}))
assert.NoError(t, err)
defer os.RemoveAll(d)

b := &bytes.Buffer{}
err = Walk(context.Background(), d, &FilterOpt{
IncludePatterns: []string{"**/*.txt"},
Map: func(_ string, s *types.Stat) MapResult {
if filepath.Base(s.Path) == "x" {
return MapResultExclude
}
return MapResultKeep
},
}, bufWalk(b))
assert.NoError(t, err)

assert.Equal(t, filepath.FromSlash(`dir x
file x/a.txt
dir y
file y/b.txt
`), b.String())
}

func TestWalkerMapPatternImpliesDir(t *testing.T) {
d, err := tmpDir(changeStream([]string{
"ADD x dir",
"ADD x/y dir",
"ADD x/y/a.txt file",
"ADD x/z dir",
"ADD x/z/b.txt file",
}))
assert.NoError(t, err)
defer os.RemoveAll(d)

b := &bytes.Buffer{}
err = Walk(context.Background(), d, &FilterOpt{
Map: func(_ string, s *types.Stat) MapResult {
if s.Path == "x/z/b.txt" {
return MapResultKeep
}

return MapResultExclude
},
}, bufWalk(b))
assert.NoError(t, err)

assert.Equal(t, filepath.FromSlash(`dir x
dir x/z
file x/z/b.txt
`), b.String())
}

func TestWalkerPermissionDenied(t *testing.T) {
if runtime.GOOS == "windows" {
t.Skip("os.Chmod not fully supported on Windows")
Expand Down
Loading