From 020c217f0248e73fae66e6585c76ab991f627788 Mon Sep 17 00:00:00 2001
From: Nick Santos <nick.santos@docker.com>
Date: Tue, 25 Jun 2024 20:41:36 -0600
Subject: [PATCH] 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
https://github.com/tilt-dev/tilt/issues/6393

Signed-off-by: Nick Santos <nick.santos@docker.com>
---
 filter.go      | 25 +++++++++++++++------
 filter_test.go | 60 ++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/filter.go b/filter.go
index 9cee8ad2..059cbe1d 100644
--- a/filter.go
+++ b/filter.go
@@ -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
 )
@@ -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 {
 			for len(parentDirs) != 0 {
 				lastParentDir := parentDirs[len(parentDirs)-1].pathWithSep
 				if strings.HasPrefix(path, lastParentDir) {
@@ -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)
@@ -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
@@ -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
@@ -372,6 +381,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
index 406425da..40aeaa59 100644
--- a/filter_test.go
+++ b/filter_test.go
@@ -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")