watch: tfw you have a test that asserts broken file-watch behavior 😢 (docker#1354)

Author: nicks

pkg/watch/notify_test.go

@@ -5,7 +5,6 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"runtime"
 	"strings"
 	"testing"
 	"time"
@@ -128,6 +127,26 @@ func TestWatchNonExistentPath(t *testing.T) {
 	f.assertEvents(path)
 }
 
+func TestWatchNonExistentPathDoesNotFireSiblingEvent(t *testing.T) {
+	f := newNotifyFixture(t)
+	defer f.tearDown()
+
+	root := f.TempDir("root")
+	watchedFile := filepath.Join(root, "a.txt")
+	unwatchedSibling := filepath.Join(root, "b.txt")
+
+	err := f.notify.Add(watchedFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f.fsync()
+
+	d1 := "hello\ngo\n"
+	f.WriteFile(unwatchedSibling, d1)
+	f.assertEvents()
+}
+
 func TestRemove(t *testing.T) {
 	f := newNotifyFixture(t)
 	defer f.tearDown()
@@ -319,18 +338,13 @@ func TestWatchNonexistentDirectory(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	parent := f.JoinPath("root", "parent")
 	file := f.JoinPath("root", "parent", "a")
 
 	f.watch(file)
 	f.fsync()
 	f.events = nil
 	f.WriteFile(file, "hello")
-	if runtime.GOOS == "darwin" {
-		f.assertEvents(file)
-	} else {
-		f.assertEvents(parent, file)
-	}
+	f.assertEvents(file)
 }
 
 type notifyFixture struct {

pkg/watch/watcher_naive.go

@@ -21,7 +21,11 @@ type naiveNotify struct {
 	events        chan fsnotify.Event
 	wrappedEvents chan FileEvent
 	errors        chan error
-	watchList     map[string]bool
+
+	// 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.
+	notifyList map[string]bool
 }
 
 func (d *naiveNotify) Add(name string) error {
@@ -32,24 +36,22 @@ func (d *naiveNotify) Add(name string) error {
 
 	// if it's a file that doesn't exist, watch its parent
 	if os.IsNotExist(err) {
-		err, fileWatched := d.watchUpRecursively(name)
+		err = d.watchAncestorOfMissingPath(name)
 		if err != nil {
-			return errors.Wrapf(err, "watchUpRecursively(%q)", name)
+			return errors.Wrapf(err, "watchAncestorOfMissingPath(%q)", name)
 		}
-		d.watchList[fileWatched] = true
 	} else if fi.IsDir() {
 		err = d.watchRecursively(name)
 		if err != nil {
 			return errors.Wrapf(err, "notify.Add(%q)", name)
 		}
-		d.watchList[name] = true
 	} else {
 		err = d.watcher.Add(name)
 		if err != nil {
 			return errors.Wrapf(err, "notify.Add(%q)", name)
 		}
-		d.watchList[name] = true
 	}
+	d.notifyList[name] = true
 
 	return nil
 }
@@ -71,22 +73,22 @@ func (d *naiveNotify) watchRecursively(dir string) error {
 	})
 }
 
-func (d *naiveNotify) watchUpRecursively(path string) (error, string) {
+func (d *naiveNotify) watchAncestorOfMissingPath(path string) error {
 	if path == string(filepath.Separator) {
-		return fmt.Errorf("cannot watch root directory"), ""
+		return fmt.Errorf("cannot watch root directory")
 	}
 
 	_, err := os.Stat(path)
 	if err != nil && !os.IsNotExist(err) {
-		return errors.Wrapf(err, "os.Stat(%q)", path), ""
+		return errors.Wrapf(err, "os.Stat(%q)", path)
 	}
 
 	if os.IsNotExist(err) {
 		parent := filepath.Dir(path)
-		return d.watchUpRecursively(parent)
+		return d.watchAncestorOfMissingPath(parent)
 	}
 
-	return d.watcher.Add(path), path
+	return d.watcher.Add(path)
 }
 
 func (d *naiveNotify) Close() error {
@@ -122,35 +124,39 @@ func (d *naiveNotify) loop() {
 					Op:   fsnotify.Create,
 					Name: path,
 				}
-				d.sendEventIfWatched(newE)
-				// TODO(dmiller): symlinks 😭
-				err = d.Add(path)
-				if err != nil {
-					log.Printf("Error watching path %s: %s", e.Name, err)
+
+				if d.shouldNotify(newE) {
+					d.wrappedEvents <- FileEvent{newE.Name}
+
+					// TODO(dmiller): symlinks 😭
+					err = d.Add(path)
+					if err != nil {
+						log.Printf("Error watching path %s: %s", e.Name, err)
+					}
 				}
 				return nil
 			})
 			if err != nil {
 				log.Printf("Error walking directory %s: %s", e.Name, err)
 			}
-		} else {
-			d.sendEventIfWatched(e)
+		} else if d.shouldNotify(e) {
+			d.wrappedEvents <- FileEvent{e.Name}
 		}
 	}
 }
 
-func (d *naiveNotify) sendEventIfWatched(e fsnotify.Event) {
-	if _, ok := d.watchList[e.Name]; ok {
-		d.wrappedEvents <- FileEvent{e.Name}
+func (d *naiveNotify) shouldNotify(e fsnotify.Event) bool {
+	if _, ok := d.notifyList[e.Name]; ok {
+		return true
 	} else {
 		// TODO(dmiller): maybe use a prefix tree here?
-		for path := range d.watchList {
+		for path := range d.notifyList {
 			if pathIsChildOf(e.Name, path) {
-				d.wrappedEvents <- FileEvent{e.Name}
-				break
+				return true
 			}
 		}
 	}
+	return false
 }
 
 func NewWatcher() (*naiveNotify, error) {
@@ -166,7 +172,7 @@ func NewWatcher() (*naiveNotify, error) {
 		events:        fsw.Events,
 		wrappedEvents: wrappedEvents,
 		errors:        fsw.Errors,
-		watchList:     map[string]bool{},
+		notifyList:    map[string]bool{},
 	}
 
 	go wmw.loop()