diff --git a/pkg/compose/watch.go b/pkg/compose/watch.go index 0af3650ede..b6d21f1d9f 100644 --- a/pkg/compose/watch.go +++ b/pkg/compose/watch.go @@ -198,9 +198,12 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti eg, ctx := errgroup.WithContext(ctx) var ( - rules []watchRule - paths []string + rules []watchRule + paths []string + ignoresByWatchPath map[string]watch.PathMatcher ) + ignoresByWatchPath = make(map[string]watch.PathMatcher) + for serviceName, service := range project.Services { config, err := loadDevelopmentConfig(service, project) if err != nil { @@ -254,9 +257,19 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti } } } - paths = append(paths, trigger.Path) - } + var ignore watch.PathMatcher + ignore, err = watch.NewDockerPatternMatcher(trigger.Path, trigger.Ignore) + if err != nil { + return nil, err + } + if existingMatcher, exists := ignoresByWatchPath[trigger.Path]; exists { + ignore = watch.NewIntersectMatcher(existingMatcher, ignore) + } else { + paths = append(paths, trigger.Path) + } + ignoresByWatchPath[trigger.Path] = ignore + } serviceWatchRules, err := getWatchRules(config, service) if err != nil { return nil, err @@ -268,7 +281,7 @@ func (s *composeService) watch(ctx context.Context, project *types.Project, opti return nil, fmt.Errorf("none of the selected services is configured for watch, consider setting a 'develop' section") } - watcher, err := watch.NewWatcher(paths) + watcher, err := watch.NewWatcher(paths, ignoresByWatchPath) if err != nil { return nil, err } @@ -591,7 +604,8 @@ func (s *composeService) handleWatchBatch(ctx context.Context, project *types.Pr } options.LogTo.Log( api.WatchLogger, - fmt.Sprintf("service(s) %q restarted", services)) + fmt.Sprintf("service(s) %q restarted", services), + ) } eg, ctx := errgroup.WithContext(ctx) @@ -751,7 +765,8 @@ func (s *composeService) initialSync(ctx context.Context, project *types.Project dockerIgnores, watch.EphemeralPathMatcher(), dotGitIgnore, - triggerIgnore) + triggerIgnore, + ) pathsToCopy, err := s.initialSyncFiles(ctx, project, service, trigger, ignoreInitialSync) if err != nil { diff --git a/pkg/watch/notify.go b/pkg/watch/notify.go index d63f5caf28..de91721a3b 100644 --- a/pkg/watch/notify.go +++ b/pkg/watch/notify.go @@ -80,8 +80,8 @@ func (EmptyMatcher) MatchesEntireDir(f string) (bool, error) { return false, nil var _ PathMatcher = EmptyMatcher{} -func NewWatcher(paths []string) (Notify, error) { - return newWatcher(paths) +func NewWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { + return newWatcher(paths, ignore) } const WindowsBufferSizeEnvVar = "COMPOSE_WATCH_WINDOWS_BUFFER_SIZE" @@ -134,3 +134,48 @@ func (c CompositePathMatcher) MatchesEntireDir(f string) (bool, error) { } var _ PathMatcher = CompositePathMatcher{} + +// intersectPathMatcher matches iff every matcher matches. With several develop.watch +// triggers on one watch root, skip/filter a path only when every trigger's ignores agree. +type intersectPathMatcher struct { + Matchers []PathMatcher +} + +// NewIntersectMatcher returns a PathMatcher that matches iff every matcher matches. +func NewIntersectMatcher(matchers ...PathMatcher) PathMatcher { + if len(matchers) == 0 { + return EmptyMatcher{} + } + if len(matchers) == 1 { + return matchers[0] + } + return intersectPathMatcher{Matchers: matchers} +} + +func (i intersectPathMatcher) Matches(f string) (bool, error) { + for _, t := range i.Matchers { + ret, err := t.Matches(f) + if err != nil { + return false, err + } + if !ret { + return false, nil + } + } + return true, nil +} + +func (i intersectPathMatcher) MatchesEntireDir(f string) (bool, error) { + for _, t := range i.Matchers { + ret, err := t.MatchesEntireDir(f) + if err != nil { + return false, err + } + if !ret { + return false, nil + } + } + return true, nil +} + +var _ PathMatcher = intersectPathMatcher{} diff --git a/pkg/watch/notify_test.go b/pkg/watch/notify_test.go index b1ec9b9644..1ecf22e2d0 100644 --- a/pkg/watch/notify_test.go +++ b/pkg/watch/notify_test.go @@ -50,6 +50,124 @@ func TestWindowsBufferSize(t *testing.T) { }) } +func TestNewIntersectMatcher(t *testing.T) { + root := t.TempDir() + + vendorOnly, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + tmpOnly, err := DockerIgnoreTesterFromContents(root, "tmp/\n") + assert.NilError(t, err) + + inter := NewIntersectMatcher(vendorOnly, tmpOnly) + + vendorFile := filepath.Join(root, "vendor", "a.go") + matches, err := inter.Matches(vendorFile) + assert.NilError(t, err) + assert.Assert(t, !matches, "only one trigger ignores vendor; intersection must not treat path as ignored") + + bothIgnoreBuild1, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + bothIgnoreBuild2, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + interBuild := NewIntersectMatcher(bothIgnoreBuild1, bothIgnoreBuild2) + buildFile := filepath.Join(root, "build", "out") + matches, err = interBuild.Matches(buildFile) + assert.NilError(t, err) + assert.Assert(t, matches) + + dirEntire1, err := DockerIgnoreTesterFromContents(root, "cache/\n") + assert.NilError(t, err) + dirEntire2, err := DockerIgnoreTesterFromContents(root, "cache/\n") + assert.NilError(t, err) + interDir := NewIntersectMatcher(dirEntire1, dirEntire2) + entire, err := interDir.MatchesEntireDir(filepath.Join(root, "cache")) + assert.NilError(t, err) + assert.Assert(t, entire) + + partialEntire := NewIntersectMatcher(vendorOnly, tmpOnly) + entire, err = partialEntire.MatchesEntireDir(filepath.Join(root, "vendor")) + assert.NilError(t, err) + assert.Assert(t, !entire, "must not skip whole dir unless every matcher agrees it is entirely ignorable") +} + +func TestWatchRespectsDockerignore(t *testing.T) { + f := newNotifyFixture(t) + + root := f.TempDir("root") + ignore, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: ignore} + f.watch(root) + f.fsync() + f.events = nil + + kept := filepath.Join(root, "src", "main.go") + f.WriteFile(kept, "package main\n") + f.assertEvents(filepath.Join(root, "src"), kept) + f.events = nil + + ignored := filepath.Join(root, "vendor", "mod", "x.go") + f.WriteFile(ignored, "module x\n") + f.assertEvents() +} + +func TestWatchPerRootIgnoresDoNotLeak(t *testing.T) { + f := newNotifyFixture(t) + + rootA := f.TempDir("root-a") + rootB := f.TempDir("root-b") + ignoreA, err := DockerIgnoreTesterFromContents(rootA, "vendor/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{rootA: ignoreA} + f.watch(rootA) + f.watch(rootB) + f.fsync() + f.events = nil + + ignoredUnderA := filepath.Join(rootA, "vendor", "x.go") + f.WriteFile(ignoredUnderA, "ignored\n") + f.assertEvents() + + keptUnderB := filepath.Join(rootB, "vendor", "x.go") + f.WriteFile(keptUnderB, "kept\n") + f.assertEvents(filepath.Join(rootB, "vendor"), keptUnderB) +} + +func TestWatchIntersectMatcherRequiresAllTriggers(t *testing.T) { + f := newNotifyFixture(t) + + root := f.TempDir("root") + ignoreVendor, err := DockerIgnoreTesterFromContents(root, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(root, "tmp/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: NewIntersectMatcher(ignoreVendor, ignoreTmp)} + f.watch(root) + f.fsync() + f.events = nil + + vendorFile := filepath.Join(root, "vendor", "x", "go.mod") + f.WriteFile(vendorFile, "module x\n") + f.assertEvents(filepath.Join(root, "vendor"), filepath.Join(root, "vendor", "x"), vendorFile) + + ignoreBuild1, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(root, "build/\n") + assert.NilError(t, err) + + f.ignores = map[string]PathMatcher{root: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)} + f.rebuildWatcher() + f.fsync() + f.events = nil + + buildFile := filepath.Join(root, "build", "out", "a") + f.WriteFile(buildFile, "artifact\n") + f.assertEvents() +} + func TestNoEvents(t *testing.T) { f := newNotifyFixture(t) f.assertEvents() @@ -493,9 +611,10 @@ type notifyFixture struct { cancel func() out *bytes.Buffer *TempDirFixture - notify Notify - paths []string - events []FileEvent + notify Notify + paths []string + ignores map[string]PathMatcher + events []FileEvent } func newNotifyFixture(t *testing.T) *notifyFixture { @@ -526,7 +645,7 @@ func (f *notifyFixture) rebuildWatcher() { } // create a new watcher - notify, err := NewWatcher(f.paths) + notify, err := NewWatcher(f.paths, f.ignores) if err != nil { f.T().Fatal(err) } diff --git a/pkg/watch/paths.go b/pkg/watch/paths.go index c0c893cd99..54cada5740 100644 --- a/pkg/watch/paths.go +++ b/pkg/watch/paths.go @@ -39,3 +39,15 @@ func greatestExistingAncestor(path string) (string, error) { return path, nil } + +func greatestExistingAncestors(paths []string) ([]string, error) { + result := make([]string, 0, len(paths)) + for _, path := range paths { + newP, err := greatestExistingAncestor(path) + if err != nil { + return nil, fmt.Errorf("finding ancestor of %s: %w", path, err) + } + result = append(result, newP) + } + return result, nil +} diff --git a/pkg/watch/paths_test.go b/pkg/watch/paths_test.go index 60b4a15e7b..f6d5b9ef30 100644 --- a/pkg/watch/paths_test.go +++ b/pkg/watch/paths_test.go @@ -41,3 +41,25 @@ func TestGreatestExistingAncestor(t *testing.T) { _, err = greatestExistingAncestor(missingTopLevel) assert.ErrorContains(t, err, "cannot watch root directory") } + +func TestGreatestExistingAncestorsMovesIgnoreToAncestor(t *testing.T) { + f := NewTempDirFixture(t) + + missing := f.JoinPath("missing", "child", "file.txt") + + paths, err := greatestExistingAncestors([]string{missing}) + assert.NilError(t, err) + assert.Equal(t, 1, len(paths)) + assert.Equal(t, f.Path(), paths[0]) +} + +func TestGreatestExistingAncestorsIntersectsIgnoreOnAncestor(t *testing.T) { + f := NewTempDirFixture(t) + + missing := f.JoinPath("missing", "child", "file.txt") + + paths, err := greatestExistingAncestors([]string{missing}) + assert.NilError(t, err) + assert.Equal(t, 1, len(paths)) + assert.Equal(t, f.Path(), paths[0]) +} diff --git a/pkg/watch/watcher_darwin.go b/pkg/watch/watcher_darwin.go index a63612aedf..19e27951d1 100644 --- a/pkg/watch/watcher_darwin.go +++ b/pkg/watch/watcher_darwin.go @@ -26,6 +26,7 @@ import ( "time" "github.com/fsnotify/fsevents" + "github.com/sirupsen/logrus" pathutil "github.com/docker/compose/v5/internal/paths" ) @@ -38,8 +39,13 @@ type fseventNotify struct { errors chan error stop chan struct{} - pathsWereWatching map[string]any - closeOnce sync.Once + // watchPaths are the paths registered with the FSEvents stream. + watchPaths []string + // notifyList holds the original trigger paths. + notifyList map[string]bool + // ignore maps each trigger path (from notifyList) to its own isolated PathMatcher + ignore map[string]PathMatcher + closeOnce sync.Once } func (d *fseventNotify) loop() { @@ -55,39 +61,49 @@ func (d *fseventNotify) loop() { for _, e := range events { e.Path = filepath.Join(string(os.PathSeparator), e.Path) - _, isPathWereWatching := d.pathsWereWatching[e.Path] - if e.Flags&fsevents.ItemIsDir == fsevents.ItemIsDir && e.Flags&fsevents.ItemCreated == fsevents.ItemCreated && isPathWereWatching { + isTriggerRoot := d.notifyList[e.Path] + if e.Flags&fsevents.ItemIsDir == fsevents.ItemIsDir && e.Flags&fsevents.ItemCreated == fsevents.ItemCreated && isTriggerRoot { // This is the first create for the path that we're watching. We always get exactly one of these // even after we get the HistoryDone event. Skip it. continue } + if !d.shouldNotify(e.Path) { + continue + } + d.events <- NewFileEvent(e.Path) } } } } -// Add a path to be watched. Should only be called during initialization. -func (d *fseventNotify) initAdd(name string) { +// addStreamPath registers an existing ancestor path with the FSEvents stream. +func (d *fseventNotify) addStreamPath(name string) { d.stream.Paths = append(d.stream.Paths, name) - - if d.pathsWereWatching == nil { - d.pathsWereWatching = make(map[string]any) - } - d.pathsWereWatching[name] = struct{}{} } func (d *fseventNotify) Start() error { - if len(d.stream.Paths) == 0 { + if len(d.watchPaths) == 0 { return nil } + watchPaths, err := greatestExistingAncestors(d.watchPaths) + if err != nil { + return err + } + watchPaths = pathutil.EncompassingPaths(watchPaths) + + d.stream.Paths = nil + for _, path := range watchPaths { + d.addStreamPath(path) + } + d.closeOnce = sync.Once{} numberOfWatches.Add(int64(len(d.stream.Paths))) - err := d.stream.Start() + err = d.stream.Start() if err != nil { return err } @@ -115,7 +131,40 @@ func (d *fseventNotify) Errors() chan error { return d.errors } -func newWatcher(paths []string) (Notify, error) { +func (d *fseventNotify) shouldNotify(path string) bool { + if d.notifyList[path] { + stat, err := os.Lstat(path) + isDir := err == nil && stat.IsDir() + return !isDir + } + + for root := range d.notifyList { + if pathutil.IsChild(root, path) { + if !d.shouldIgnore(root, path) { + return true + } + } + } + return false +} + +func (d *fseventNotify) shouldIgnore(dir, path string) bool { + if len(d.ignore) == 0 { + return false + } + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.Matches(path) + if err != nil { + logrus.Debugf("error checking ignored path %q: %v", path, err) + return false + } + return matches + } + return false +} + +func newWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { dw := &fseventNotify{ stream: &fsevents.EventStream{ Latency: 50 * time.Millisecond, @@ -129,15 +178,21 @@ func newWatcher(paths []string) (Notify, error) { stop: make(chan struct{}), } - paths = pathutil.EncompassingPaths(paths) + watchPaths := pathutil.EncompassingPaths(paths) + + notifyList := make(map[string]bool, len(paths)) for _, path := range paths { - path, err := filepath.Abs(path) + abs, err := filepath.Abs(path) if err != nil { return nil, fmt.Errorf("newWatcher: %w", err) } - dw.initAdd(path) + notifyList[abs] = true } + dw.ignore = ignore + dw.notifyList = notifyList + dw.watchPaths = watchPaths + return dw, nil } diff --git a/pkg/watch/watcher_darwin_test.go b/pkg/watch/watcher_darwin_test.go index 50eb442b06..25ecd938a9 100644 --- a/pkg/watch/watcher_darwin_test.go +++ b/pkg/watch/watcher_darwin_test.go @@ -19,15 +19,24 @@ package watch import ( + "os" + "path/filepath" "testing" "gotest.tools/v3/assert" ) +func newFseventNotifyFixture(repo string, ignore map[string]PathMatcher) *fseventNotify { + return &fseventNotify{ + notifyList: map[string]bool{repo: true}, + ignore: ignore, + } +} + func TestFseventNotifyCloseIdempotent(t *testing.T) { // Create a watcher with a temporary directory tmpDir := t.TempDir() - watcher, err := newWatcher([]string{tmpDir}) + watcher, err := newWatcher([]string{tmpDir}, nil) assert.NilError(t, err) // Start the watcher @@ -46,3 +55,155 @@ func TestFseventNotifyCloseIdempotent(t *testing.T) { err = watcher.Close() assert.NilError(t, err) } + +func TestFseventNotifyShouldNotifyNilIgnore(t *testing.T) { + repo := t.TempDir() + child := filepath.Join(repo, "child.txt") + assert.NilError(t, os.WriteFile(child, []byte("x"), 0o644)) + + d := newFseventNotifyFixture(repo, nil) + assert.Assert(t, d.shouldNotify(child), "expected file under watched root to notify") + assert.Assert(t, !d.shouldNotify(repo), "expected directory event at watched root to be skipped") +} + +func TestFseventNotifyShouldNotifyWatchedFileRoot(t *testing.T) { + repo := t.TempDir() + fileRoot := filepath.Join(repo, "watched.go") + assert.NilError(t, os.WriteFile(fileRoot, []byte("package main\n"), 0o644)) + + d := newFseventNotifyFixture(fileRoot, nil) + assert.Assert(t, d.shouldNotify(fileRoot), "expected file that is the watch root to notify") +} + +func TestFseventNotifyShouldNotifyOutsideWatchedTree(t *testing.T) { + repo := t.TempDir() + other := t.TempDir() + + d := newFseventNotifyFixture(repo, nil) + outPath := filepath.Join(other, "outside.txt") + assert.NilError(t, os.WriteFile(outPath, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(outPath), "expected path outside watch roots not to notify") +} + +func TestFseventNotifyShouldNotifyRespectsDockerignore(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) + kept := filepath.Join(repo, "src", "main.go") + assert.NilError(t, os.MkdirAll(filepath.Dir(kept), 0o755)) + assert.NilError(t, os.WriteFile(kept, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(kept), "expected non-ignored path to notify") + + ignored := filepath.Join(repo, "vendor", "mod", "x.go") + assert.NilError(t, os.MkdirAll(filepath.Dir(ignored), 0o755)) + assert.NilError(t, os.WriteFile(ignored, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(ignored), "expected dockerignored path not to notify") +} + +func TestFseventNotifyShouldNotifyDockerignoreNegation(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) + + ignoredChild := filepath.Join(repo, "bazel-bin", "cache", "out") + assert.NilError(t, os.MkdirAll(filepath.Dir(ignoredChild), 0o755)) + assert.NilError(t, os.WriteFile(ignoredChild, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(ignoredChild), "expected ignored subtree under bazel-bin not to notify") + + excepted := filepath.Join(repo, "bazel-bin", "app-binary", "binary") + assert.NilError(t, os.MkdirAll(filepath.Dir(excepted), 0o755)) + assert.NilError(t, os.WriteFile(excepted, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(excepted), "expected negated dockerignore path to notify") +} + +func TestFseventNotifyShouldNotifyIntersectMatcher(t *testing.T) { + repo := t.TempDir() + ignoreVendor, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(repo, "tmp/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: NewIntersectMatcher(ignoreVendor, ignoreTmp)}) + vendorFile := filepath.Join(repo, "vendor", "x", "go.mod") + assert.NilError(t, os.MkdirAll(filepath.Dir(vendorFile), 0o755)) + assert.NilError(t, os.WriteFile(vendorFile, []byte("module x\n"), 0o644)) + assert.Assert(t, d.shouldNotify(vendorFile), "vendor must notify when only one intersect matcher ignores it") + + ignoreBuild1, err := DockerIgnoreTesterFromContents(repo, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(repo, "build/\n") + assert.NilError(t, err) + d2 := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)}) + buildFile := filepath.Join(repo, "build", "out", "a") + assert.NilError(t, os.MkdirAll(filepath.Dir(buildFile), 0o755)) + assert.NilError(t, os.WriteFile(buildFile, []byte("x"), 0o644)) + assert.Assert(t, !d2.shouldNotify(buildFile), "expected path ignored by every intersect matcher not to notify") +} + +func TestFseventNotifyShouldNotifyAnyRootSaysOK(t *testing.T) { + repoRoot := t.TempDir() + srcRoot := filepath.Join(repoRoot, "src") + assert.NilError(t, os.MkdirAll(srcRoot, 0o755)) + + // Service A watches repoRoot and ignores the entire src/ subtree. + // Service B watches repoRoot/src and ignores only node_modules/. + // A path is notified if ANY containing root's matcher does not suppress it. + parentIgnore, err := DockerIgnoreTesterFromContents(repoRoot, "src/\n") + assert.NilError(t, err) + childIgnore, err := DockerIgnoreTesterFromContents(srcRoot, "node_modules/\n") + assert.NilError(t, err) + + d := &fseventNotify{ + notifyList: map[string]bool{repoRoot: true, srcRoot: true}, + ignore: map[string]PathMatcher{ + repoRoot: parentIgnore, + srcRoot: childIgnore, + }, + } + + // srcRoot does not ignore foo.ts, so it is notified even though repoRoot ignores src/. + fooFile := filepath.Join(srcRoot, "foo.ts") + assert.NilError(t, os.WriteFile(fooFile, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(fooFile), + "file under child root must be notified; srcRoot does not ignore it even though repoRoot ignores src/") + + // Every containing root ignores this path (repoRoot via src/, srcRoot via node_modules/). + nodeModulesFile := filepath.Join(srcRoot, "node_modules", "dep.js") + assert.NilError(t, os.MkdirAll(filepath.Dir(nodeModulesFile), 0o755)) + assert.NilError(t, os.WriteFile(nodeModulesFile, []byte("x"), 0o644)) + assert.Assert(t, !d.shouldNotify(nodeModulesFile), + "node_modules file must not be notified; all containing roots ignore it") + + // repoRoot does not ignore main.go (outside src/), so it is notified. + otherFile := filepath.Join(repoRoot, "main.go") + assert.NilError(t, os.WriteFile(otherFile, []byte("x"), 0o644)) + assert.Assert(t, d.shouldNotify(otherFile), + "file outside src/ must be notified; repoRoot does not ignore it") +} + +func TestFseventNotifyShouldIgnoreDockerignoreDirectory(t *testing.T) { + repo := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) + bazelBin := filepath.Join(repo, "bazel-bin") + assert.NilError(t, os.MkdirAll(bazelBin, 0o755)) + assert.Assert(t, d.shouldIgnore(repo, bazelBin), "expected directory path to match dockerignore") +} + +func TestFseventNotifyShouldIgnoreLooksUpMatcherByWatchRoot(t *testing.T) { + repo := t.TempDir() + other := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repo, "vendor/\n") + assert.NilError(t, err) + + d := newFseventNotifyFixture(repo, map[string]PathMatcher{repo: ignore}) + vendorFile := filepath.Join(repo, "vendor", "x.go") + assert.Assert(t, d.shouldIgnore(repo, vendorFile), "expected matcher keyed to watched root to apply") + assert.Assert(t, !d.shouldIgnore(other, vendorFile), "expected unrelated watch root not to apply matcher") +} diff --git a/pkg/watch/watcher_naive.go b/pkg/watch/watcher_naive.go index 7798025e8b..900f3282da 100644 --- a/pkg/watch/watcher_naive.go +++ b/pkg/watch/watcher_naive.go @@ -37,14 +37,16 @@ import ( // // All OS-specific codepaths are handled by fsnotify. 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. + // pathsToWatch are the actual paths registered with the OS watcher. + pathsToWatch []string + // notifyList holds the original trigger paths. // // 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 maps each trigger path (from notifyList) to its isolated PathMatcher. + ignore map[string]PathMatcher isWatcherRecursive bool watcher *fsnotify.Watcher @@ -59,12 +61,7 @@ func (d *naiveNotify) Start() error { return nil } - pathsToWatch := []string{} - for path := range d.notifyList { - pathsToWatch = append(pathsToWatch, path) - } - - pathsToWatch, err := greatestExistingAncestors(pathsToWatch) + pathsToWatch, err := greatestExistingAncestors(d.pathsToWatch) if err != nil { return err } @@ -228,7 +225,9 @@ func (d *naiveNotify) shouldNotify(path string) bool { for root := range d.notifyList { if pathutil.IsChild(root, path) { - return true + if !d.shouldIgnore(root, path) { + return true + } } } return false @@ -240,26 +239,53 @@ func (d *naiveNotify) shouldSkipDir(path string) bool { return false } - // 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). + // Only walk directories under a notifyList path or under an ancestor of one + // Ignore a path only if every parent root's ignore matcher agrees for root := range d.notifyList { - if pathutil.IsChild(root, path) || pathutil.IsChild(path, root) { + if pathutil.IsChild(path, root) { return false } + if pathutil.IsChild(root, path) { + if !d.shouldIgnoreEntireDir(root, path) { + return false + } + } } return true } +func (d *naiveNotify) shouldIgnoreEntireDir(dir, path string) bool { + if len(d.ignore) == 0 { + return false + } + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.MatchesEntireDir(path) + if err != nil { + logrus.Debugf("error checking ignored directory %q: %v", path, err) + return false + } + return matches + } + return false +} + +func (d *naiveNotify) shouldIgnore(dir, path string) bool { + if len(d.ignore) == 0 { + return false + } + + if ignore, exists := d.ignore[dir]; exists && ignore != nil { + matches, err := ignore.Matches(path) + if err != nil { + logrus.Debugf("error checking ignored path %q: %v", path, err) + return false + } + return matches + } + return false +} + func (d *naiveNotify) add(path string) error { err := d.watcher.Add(path) if err != nil { @@ -270,7 +296,7 @@ func (d *naiveNotify) add(path string) error { return nil } -func newWatcher(paths []string) (Notify, error) { +func newWatcher(paths []string, ignore map[string]PathMatcher) (Notify, error) { fsw, err := fsnotify.NewWatcher() if err != nil { if strings.Contains(err.Error(), "too many open files") && runtime.GOOS == "linux" { @@ -286,20 +312,25 @@ func newWatcher(paths []string) (Notify, error) { isWatcherRecursive := err == nil wrappedEvents := make(chan FileEvent) - notifyList := make(map[string]bool, len(paths)) + + watchRoots := paths if isWatcherRecursive { - paths = pathutil.EncompassingPaths(paths) + watchRoots = pathutil.EncompassingPaths(paths) } + + notifyList := make(map[string]bool, len(paths)) for _, path := range paths { - path, err := filepath.Abs(path) + abs, err := filepath.Abs(path) if err != nil { return nil, fmt.Errorf("newWatcher: %w", err) } - notifyList[path] = true + notifyList[abs] = true } wmw := &naiveNotify{ + pathsToWatch: watchRoots, notifyList: notifyList, + ignore: ignore, watcher: fsw, events: fsw.Events, wrappedEvents: wrappedEvents, @@ -311,15 +342,3 @@ func newWatcher(paths []string) (Notify, error) { } var _ Notify = &naiveNotify{} - -func greatestExistingAncestors(paths []string) ([]string, error) { - result := []string{} - for _, p := range paths { - newP, err := greatestExistingAncestor(p) - if err != nil { - return nil, fmt.Errorf("finding ancestor of %s: %w", p, err) - } - result = append(result, newP) - } - return result, nil -} diff --git a/pkg/watch/watcher_naive_test.go b/pkg/watch/watcher_naive_test.go index b188de9390..8771c9080b 100644 --- a/pkg/watch/watcher_naive_test.go +++ b/pkg/watch/watcher_naive_test.go @@ -123,7 +123,8 @@ func inotifyNodes() (int, error) { pid := os.Getpid() output, err := exec.Command("/bin/sh", "-c", fmt.Sprintf( - "find /proc/%d/fd -lname anon_inode:inotify -printf '%%hinfo/%%f\n' | xargs cat | grep -c '^inotify'", pid)).Output() + "find /proc/%d/fd -lname anon_inode:inotify -printf '%%hinfo/%%f\n' | xargs cat | grep -c '^inotify'", pid, + )).Output() if err != nil { return 0, fmt.Errorf("error running command to determine number of watched files: %w\n %s", err, output) } @@ -157,3 +158,197 @@ func TestDontRecurseWhenWatchingParentsOfNonExistentFiles(t *testing.T) { t.Fatalf("watching more than 5 files: %d", n) } } + +func TestShouldSkipDirWithNegatedChildException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: ignore}, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, !d.shouldSkipDir(bazelBin), "expected bazel-bin to remain traversable for negated child patterns") +} + +func TestShouldIgnorePathStillMatchesDirectoryPattern(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n!bazel-bin/app-binary\n") + assert.NilError(t, err) + + d := &naiveNotify{ignore: map[string]PathMatcher{repoRoot: ignore}} + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldIgnore(repoRoot, bazelBin), "expected bazel-bin path to match ignore pattern") +} + +func TestShouldIgnoreLooksUpMatcherByWatchRoot(t *testing.T) { + repoRoot := t.TempDir() + otherRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + + d := &naiveNotify{ignore: map[string]PathMatcher{repoRoot: ignore}} + + vendorFile := filepath.Join(repoRoot, "vendor", "x.go") + assert.Assert(t, d.shouldIgnore(repoRoot, vendorFile), "expected matcher keyed to watched root to apply") + assert.Assert(t, !d.shouldIgnore(otherRoot, vendorFile), "expected unrelated watch root not to apply matcher") +} + +func TestShouldIgnoreEntireDirLooksUpMatcherByWatchRoot(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: ignore}, + notifyList: map[string]bool{repoRoot: true}, + } + + vendorDir := filepath.Join(repoRoot, "vendor") + assert.Assert(t, d.shouldIgnoreEntireDir(repoRoot, vendorDir), "expected directory matcher keyed to watched root to apply") +} + +func TestShouldSkipDirForIgnoredSubtreeWithoutException(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "bazel-bin/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: ignore}, + notifyList: map[string]bool{repoRoot: true}, + } + + bazelBin := filepath.Join(repoRoot, "bazel-bin") + assert.Assert(t, d.shouldSkipDir(bazelBin), "expected fully ignored directory subtree to be skipped") +} + +func TestShouldSkipDirDoesNotSkipAncestorOfWatchedPath(t *testing.T) { + repoRoot := t.TempDir() + ignore, err := DockerIgnoreTesterFromContents(repoRoot, "parent/\n") + assert.NilError(t, err) + + watchedPath := filepath.Join(repoRoot, "parent", "child", "non-existent") + d := &naiveNotify{ + ignore: map[string]PathMatcher{watchedPath: ignore}, + notifyList: map[string]bool{watchedPath: true}, + } + + parent := filepath.Join(repoRoot, "parent") + assert.Assert(t, !d.shouldSkipDir(parent), "expected parent directory to remain traversable when it contains a watched path") +} + +func TestShouldSkipDirRequiresAllContainingRootsToAgree(t *testing.T) { + repoRoot := t.TempDir() + srcRoot := filepath.Join(repoRoot, "src") + + // Service A watches repoRoot but does NOT list node_modules in its ignores. + // Service B watches src and ignores node_modules. + // node_modules under src must NOT be skipped — because service A (which also + // covers the path) has no rule for it. All containing roots must agree to skip. + rootIgnore, err := DockerIgnoreTesterFromContents(repoRoot, "need_perm_dir/\n") + assert.NilError(t, err) + childIgnore, err := DockerIgnoreTesterFromContents(srcRoot, "node_modules/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: rootIgnore, srcRoot: childIgnore}, + notifyList: map[string]bool{repoRoot: true, srcRoot: true}, + } + + nodeModulesDir := filepath.Join(srcRoot, "node_modules") + assert.Assert(t, !d.shouldSkipDir(nodeModulesDir), + "node_modules under child root must not be skipped when a containing root (repoRoot) has no matching ignore rule") + + // A legitimate subdir under src is also not skipped. + componentsDir := filepath.Join(srcRoot, "components") + assert.Assert(t, !d.shouldSkipDir(componentsDir), + "non-ignored directory under child root must remain watched") +} + +func TestShouldSkipDirNotVetoedByUnrelatedChildTrigger(t *testing.T) { + repoRoot := t.TempDir() + srcRoot := filepath.Join(repoRoot, "src") + + // Service A watches repoRoot and ignores root-owned-dir/. + // Service B watches repoRoot/src with an unrelated ignore. + // root-owned-dir is outside src, so service B has no opinion about it. + // The directory must still be skipped so the walker never enters it. + rootIgnore, err := DockerIgnoreTesterFromContents(repoRoot, "root-owned-dir/\n") + assert.NilError(t, err) + childIgnore, err := DockerIgnoreTesterFromContents(srcRoot, "node_modules/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: rootIgnore, srcRoot: childIgnore}, + notifyList: map[string]bool{repoRoot: true, srcRoot: true}, + } + + rootOwnedDir := filepath.Join(repoRoot, "root-owned-dir") + assert.Assert(t, d.shouldSkipDir(rootOwnedDir), + "root-owned-dir must be skipped; child trigger must not veto parent's ignore") +} + +func TestShouldNotifyAnyRootSaysOK(t *testing.T) { + repoRoot := t.TempDir() + srcRoot := filepath.Join(repoRoot, "src") + + // Service A watches repoRoot and ignores the entire src/ subtree. + // Service B watches repoRoot/src and ignores only node_modules/. + // A path is notified if ANY containing root's matcher does not suppress it. + parentIgnore, err := DockerIgnoreTesterFromContents(repoRoot, "src/\n") + assert.NilError(t, err) + childIgnore, err := DockerIgnoreTesterFromContents(srcRoot, "node_modules/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: parentIgnore, srcRoot: childIgnore}, + notifyList: map[string]bool{repoRoot: true, srcRoot: true}, + } + + // A regular source file under src/ is notified because srcRoot's matcher does + // not ignore it, even though repoRoot ignores all of src/. + fooFile := filepath.Join(srcRoot, "foo.ts") + assert.Assert(t, d.shouldNotify(fooFile), + "file under child root must be notified; srcRoot does not ignore it even though repoRoot ignores src/") + + // A file inside node_modules is not notified: every containing root ignores it + // (repoRoot ignores src/, srcRoot ignores node_modules/). + nodeModulesFile := filepath.Join(srcRoot, "node_modules", "dep.js") + assert.Assert(t, !d.shouldNotify(nodeModulesFile), + "node_modules file must not be notified; all containing roots ignore it") + + // A file outside src/ is notified because repoRoot does not ignore it. + otherFile := filepath.Join(repoRoot, "main.go") + assert.Assert(t, d.shouldNotify(otherFile), + "file outside src/ must be notified; repoRoot does not ignore it") +} + +func TestShouldSkipDirIntersectRequiresAllTriggersToAgree(t *testing.T) { + repoRoot := t.TempDir() + ignoreVendor, err := DockerIgnoreTesterFromContents(repoRoot, "vendor/\n") + assert.NilError(t, err) + ignoreTmp, err := DockerIgnoreTesterFromContents(repoRoot, "tmp/\n") + assert.NilError(t, err) + + d := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: NewIntersectMatcher(ignoreVendor, ignoreTmp)}, + notifyList: map[string]bool{repoRoot: true}, + } + + vendorDir := filepath.Join(repoRoot, "vendor") + assert.Assert(t, !d.shouldSkipDir(vendorDir), "vendor must remain watched when another trigger does not ignore it") + + ignoreBuild1, err := DockerIgnoreTesterFromContents(repoRoot, "build/\n") + assert.NilError(t, err) + ignoreBuild2, err := DockerIgnoreTesterFromContents(repoRoot, "build/\n") + assert.NilError(t, err) + d2 := &naiveNotify{ + ignore: map[string]PathMatcher{repoRoot: NewIntersectMatcher(ignoreBuild1, ignoreBuild2)}, + notifyList: map[string]bool{repoRoot: true}, + } + buildDir := filepath.Join(repoRoot, "build") + assert.Assert(t, d2.shouldSkipDir(buildDir), "when every trigger ignores the same subtree, watcher may skip it") +}