filter: fix a bug with parent dirs

if the Map() function excludes a directory, but includes a file inside that directory, then the walker must backtrack and include the directory. otherwise, buildkit gets confused and sends "changes out of order" errors. part of fixing tilt-dev/tilt#6393 Signed-off-by: Nick Santos <[email protected]>
nicks · Jun 26, 2024 · 740d487 · 740d487
1 parent 91a3fc4
commit 740d487
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 14 deletions.
diff --git a/filter.go b/filter.go
@@ -197,7 +197,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 {
 			for len(parentDirs) != 0 {
 				lastParentDir := parentDirs[len(parentDirs)-1].pathWithSep
 				if strings.HasPrefix(path, lastParentDir) {
@@ -298,7 +298,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)
@@ -310,8 +310,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
@@ -333,6 +331,7 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
 					return nil
 				}
 			}
+
 			for i, parentDir := range parentDirs {
 				if parentDir.skipFn {
 					return filepath.SkipDir
@@ -354,15 +353,6 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
 					return ctx.Err()
 				default:
 				}
-				if fs.mapFn != nil {
-					result := fs.mapFn(parentStat.Path, parentStat)
-					if result == MapResultExclude {
-						continue
-					} else if result == MapResultSkipDir {
-						parentDirs[i].skipFn = true
-						return filepath.SkipDir
-					}
-				}
 
 				parentDirs[i].calledFn = true
 				if err := fn(parentStat.Path, &DirEntryInfo{Stat: parentStat}, nil); err == filepath.SkipDir {
@@ -372,6 +362,8 @@ func (fs *filterFS) Walk(ctx context.Context, target string, fn gofs.WalkDirFunc
 					return err
 				}
 			}
+
+			dir.calledFn = true
 			if err := fn(stat.Path, &DirEntryInfo{Stat: stat}, nil); err != nil {
 				return err
 			}

diff --git a/filter_test.go b/filter_test.go
@@ -342,6 +342,7 @@ func TestWalkerMapSkipDirWithPattern(t *testing.T) {
 	assert.NoError(t, err)
 	defer os.RemoveAll(d)
 
+	// The include pattern takes precence over the map function.
 	b := &bytes.Buffer{}
 	err = Walk(context.Background(), d, &FilterOpt{
 		IncludePatterns: []string{"**/*.txt"},
@@ -354,11 +355,42 @@ func TestWalkerMapSkipDirWithPattern(t *testing.T) {
 	}, bufWalk(b))
 	assert.NoError(t, err)
 
-	assert.Equal(t, filepath.FromSlash(`dir y
+	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")