diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 50c7fb7ba..00ca01174 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -26,6 +26,10 @@ type naiveNotify struct { // Paths that we're watching that should be passed up to the caller. // Note that we may have to watch ancestors of these paths // in order to fulfill the API promise. + // + // We often need to check if paths are a child of a path in + // the notify list. It might be better to store this in a tree + // structure, so we can filter the list quickly. notifyList map[string]bool ignore PathMatcher @@ -226,7 +230,7 @@ func (d *naiveNotify) shouldNotify(path string) bool { } return true } - // TODO(dmiller): maybe use a prefix tree here? + for root := range d.notifyList { if ospath.IsChild(root, path) { return true @@ -245,7 +249,29 @@ func (d *naiveNotify) shouldSkipDir(path string) (bool, error) { if err != nil { return false, errors.Wrap(err, "shouldSkipDir") } - return skip, nil + + if skip { + return true, nil + } + + // Suppose we're watching + // /src/.tiltignore + // but the .tiltignore file doesn't exist. + // + // Our watcher will create an inotify watch on /src/. + // + // But then we want to make sure we don't recurse from /src/ down to /src/node_modules. + // + // To handle this case, we only want to traverse dirs that are: + // - A child of a directory that's in our notify list, or + // - A parent of a directory that's in our notify list + // (i.e., to cover the "path doesn't exist" case). + for root := range d.notifyList { + if ospath.IsChild(root, path) || ospath.IsChild(path, root) { + return false, nil + } + } + return true, nil } func (d *naiveNotify) add(path string) error { diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index e3af9b6f1..a54228efe 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -7,10 +7,13 @@ import ( "fmt" "os" "os/exec" + "path/filepath" "runtime" "strconv" "strings" "testing" + + "github.com/stretchr/testify/require" ) func TestDontWatchEachFile(t *testing.T) { @@ -96,20 +99,48 @@ func TestDontWatchEachFile(t *testing.T) { } f.events = nil + n, err := inotifyNodes() + require.NoError(t, err) + if n > 10 { + t.Fatalf("watching more than 10 files: %d", n) + } +} + +func inotifyNodes() (int, error) { pid := os.Getpid() output, err := exec.Command("bash", "-c", fmt.Sprintf( "find /proc/%d/fd -lname anon_inode:inotify -printf '%%hinfo/%%f\n' | xargs cat | grep -c '^inotify'", pid)).Output() if err != nil { - t.Fatalf("error running command to determine number of watched files: %v", err) + return 0, fmt.Errorf("error running command to determine number of watched files: %v", err) } n, err := strconv.Atoi(strings.TrimSpace(string(output))) if err != nil { - t.Fatalf("couldn't parse number of watched files: %v", err) + return 0, fmt.Errorf("couldn't parse number of watched files: %v", err) + } + return n, nil +} + +func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { + if runtime.GOOS != "linux" { + t.Skip("This test uses linux-specific inotify checks") } - if n > 10 { - t.Fatalf("watching more than 10 files: %d", n) + f := newNotifyFixture(t) + + watched := f.TempDir("watched") + f.watch(filepath.Join(watched, ".tiltignore")) + + excludedDir := f.JoinPath(watched, "excluded") + for i := 0; i < 10; i++ { + f.WriteFile(f.JoinPath(excludedDir, fmt.Sprintf("%d", i), "data.txt"), "initial data") + } + f.fsync() + + n, err := inotifyNodes() + require.NoError(t, err) + if n > 5 { + t.Fatalf("watching more than 5 files: %d", n) } }