[tools] gopls/internal/filewatcher: synthesize events after watching a dir

6 views
Skip to first unread message

Hongxiang Jiang (Gerrit)

unread,
Oct 29, 2025, 4:14:06 PM (8 days ago) Oct 29
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang has uploaded the change for review

Commit message

gopls/internal/filewatcher: synthesize events after watching a dir

Previously, file or subdirectory creations could be missed if they
occurred after a directory was created but before the file watcher was
registered for it. This resulted in missed events for all subsequent
operations within that subdirectory.

This change addresses this gap by performing a breadth-first scan of a
new directory immediately after the watch is established. The scan
iterates over the directory's entries, synthesizes `Create` events for
each one.

Subdirectories discovered during the scan are added to the queue to be
processed recursively. This approach guarantees that no file creation
events are missed, though it may result in duplicate events if the
watcher also captured the creation.

The root dir creation event and all synthesized `Create` are added to
the out atomically to make sure the out slice is always holding a
logically correct file events and ready to flush anytime.

For golang/go#74292
Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160

Change diff

diff --git a/gopls/internal/filewatcher/filewatcher.go b/gopls/internal/filewatcher/filewatcher.go
index 1818b77..19d7113 100644
--- a/gopls/internal/filewatcher/filewatcher.go
+++ b/gopls/internal/filewatcher/filewatcher.go
@@ -184,12 +184,46 @@
 					continue
 				}
 
+				synthesized := []protocol.FileEvent{} // synthesized create events
+
 				if isDir {
 					switch e.Type {
 					case protocol.Created:
-						if err := w.watchDir(event.Name); err != nil {
-							errHandler(err)
+						queue := []string{event.Name}
+						for len(queue) > 0 {
+							cur := queue[0]
+							queue = queue[1:]
+
+							if err := w.watchDir(cur); err != nil {
+								errHandler(err)
+								continue
+							}
+
+							entries, err := os.ReadDir(cur)
+							if err != nil {
+								errHandler(err)
+								continue
+							}
+							for _, entry := range entries {
+								if entry.IsDir() && skipDir(entry.Name()) {
+									continue
+								}
+								if !entry.IsDir() && skipFile(entry.Name()) {
+									continue
+								}
+
+								path := filepath.Join(cur, entry.Name())
+								synthesized = append(synthesized, protocol.FileEvent{
+									URI:  protocol.URIFromPath(path),
+									Type: protocol.Created,
+								})
+
+								if entry.IsDir() {
+									queue = append(queue, path)
+								}
+							}
 						}
+
 					case protocol.Deleted:
 						// Upon removal, we only need to remove the entries from
 						// the map. The [fsnotify.Watcher] removes the watch for
@@ -203,6 +237,10 @@
 					}
 				}
 
+				// Events discovered should be appended to the out slice atomically.
+				// This make sure at any point, the out slice is holding a
+				// logically correct file events that can be flushed anytime.
+				w.mu.Lock()
 				// Some systems emit duplicate change events in close
 				// succession upon file modification. While the current
 				// deduplication is naive and only handles immediate duplicates,
@@ -212,10 +250,11 @@
 				// events means all duplicates, regardless of proximity, should
 				// be removed. Consider checking the entire buffered slice or
 				// using a map for this.
-				w.mu.Lock()
 				if len(w.out) == 0 || w.out[len(w.out)-1] != e {
 					w.out = append(w.out, e)
 				}
+				// Synthesized creation event does not need to deduplicate.
+				w.out = append(w.out, synthesized...)
 				w.mu.Unlock()
 			}
 		}
@@ -246,6 +285,16 @@
 	return strings.HasPrefix(dirName, ".") || strings.HasPrefix(dirName, "_") || dirName == "testdata"
 }
 
+// skipFile reports whether the file should be skipped.
+func skipFile(fileName string) bool {
+	switch strings.TrimPrefix(filepath.Ext(fileName), ".") {
+	case "go", "mod", "sum", "work", "s":
+		return false
+	default:
+		return true
+	}
+}
+
 // WatchDir walks through the directory and all its subdirectories, adding
 // them to the watcher.
 func (w *Watcher) WatchDir(path string) error {
@@ -284,16 +333,11 @@
 	}
 
 	// Filter out events for directories and files that are not of interest.
-	if isDir {
-		if skipDir(filepath.Base(event.Name)) {
-			return protocol.FileEvent{}, true
-		}
-	} else {
-		switch strings.TrimPrefix(filepath.Ext(event.Name), ".") {
-		case "go", "mod", "sum", "work", "s":
-		default:
-			return protocol.FileEvent{}, false
-		}
+	if isDir && skipDir(filepath.Base(event.Name)) {
+		return protocol.FileEvent{}, true
+	}
+	if !isDir && skipFile(filepath.Base(event.Name)) {
+		return protocol.FileEvent{}, false
 	}
 
 	var t protocol.FileChangeType
diff --git a/gopls/internal/filewatcher/filewatcher_test.go b/gopls/internal/filewatcher/filewatcher_test.go
index d239868..8d768e9 100644
--- a/gopls/internal/filewatcher/filewatcher_test.go
+++ b/gopls/internal/filewatcher/filewatcher_test.go
@@ -11,6 +11,7 @@
 	"path/filepath"
 	"runtime"
 	"slices"
+	"strings"
 	"testing"
 	"time"
 
@@ -31,11 +32,11 @@
 	testCases := []struct {
 		name string
 		goos []string // if not empty, only run in these OS.
-		// If set, sends watch errors for this path to an error channel
-		// passed to the 'changes' func.
-		watchErrorPath string
+		// If set, sends watch errors for these path to error channels passed
+		// to the 'changes' func. After successful watch, channel will be closed.
+		watchErrorPath []string
 		initWorkspace  string
-		changes        func(root string, errs chan error) error
+		changes        func(root string, errs map[string]chan error) error
 		expectedEvents []protocol.FileEvent
 	}{
 		{
@@ -45,7 +46,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644)
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -59,7 +60,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644)
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -73,7 +74,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main // modified"), 0644)
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -88,7 +89,7 @@
 -- bar.go --
 package bar
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Remove(filepath.Join(root, "foo.go"))
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -102,7 +103,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go"))
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -117,7 +118,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go"))
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -131,7 +132,7 @@
 -- foo.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Mkdir(filepath.Join(root, "bar"), 0755)
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -144,7 +145,7 @@
 -- foo/bar.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.RemoveAll(filepath.Join(root, "foo"))
 			},
 			expectedEvents: []protocol.FileEvent{
@@ -166,12 +167,13 @@
 -- foo/bar.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz"))
 			},
 			expectedEvents: []protocol.FileEvent{
 				{URI: "foo", Type: protocol.Deleted},
 				{URI: "baz", Type: protocol.Created},
+				{URI: "baz/bar.go", Type: protocol.Created},
 			},
 		},
 		{
@@ -181,32 +183,50 @@
 -- foo/bar.go --
 package foo
 `,
-			changes: func(root string, errs chan error) error {
+			changes: func(root string, errs map[string]chan error) error {
 				return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz"))
 			},
 			expectedEvents: []protocol.FileEvent{
 				{URI: "baz", Type: protocol.Created},
+				{URI: "baz/bar.go", Type: protocol.Created},
 				{URI: "foo", Type: protocol.Deleted},
 			},
 		},
+		// TOOD(hxjiang): test for symlink to a dir.
 		{
 			name:           "broken symlink in darwin",
 			goos:           []string{"darwin"},
-			watchErrorPath: "foo",
-			changes: func(root string, errs chan error) error {
+			watchErrorPath: []string{"foo", "foo/b"},
+			changes: func(root string, errs map[string]chan error) error {
 				// Prepare a dir with with broken symbolic link.
-				// foo                        <- 1st
-				// └── from.go -> root/to.go  <- 1st
-				tmp := filepath.Join(t.TempDir(), "foo")
-				if err := os.Mkdir(tmp, 0755); err != nil {
-					return err
-				}
-				from := filepath.Join(tmp, "from.go")
+				// foo                       <- 1st
+				// ├── from.go -> root/to.go <- 1st
+				// ├── a.go                  <- 1st
+				// └── b                     <- 1st
+				//     └── b.go.             <- 1st
 
 				to := filepath.Join(root, "to.go")
+
+				archive := txtar.Parse([]byte(`
+-- a.go --
+package a
+-- b/b.go --
+package b
+`))
+				tmp := filepath.Join(t.TempDir(), "foo")
+				for _, f := range archive.Files {
+					path := filepath.Join(tmp, f.Name)
+					if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
+						return err
+					}
+					if err := os.WriteFile(path, f.Data, 0644); err != nil {
+						return err
+					}
+				}
+
 				// Create the symbolic link to a non-existing file. This would
-				// cause the watch registration to fail.
-				if err := os.Symlink(to, from); err != nil {
+				//  cause the watch registration for dir "foo" to fail.
+				if err := os.Symlink(to, filepath.Join(tmp, "from.go")); err != nil {
 					return err
 				}
 
@@ -218,54 +238,80 @@
 				}
 
 				// root
-				// ├── foo                       <- 2nd (Move)
-				// │   ├── from.go -> ../to.go   <- 2nd (Move)
-				// │   └── foo.go                <- 4th (Create)
-				// └── to.go                     <- 3rd (Create)
+				// ├── foo                          <- 2nd (Move)
+				// │   ├── from.go -> ../../to.go   <- 2nd (Move)
+				// │   ├── a.go                     <- 2nd (Move)
+				// │   ├── new.go                   <- 4th (Create)
+				// │   └── b                        <- 2nd (Move)
+				// │       ├── b.go                 <- 2nd (Move)
+				// │       └── new.go               <- 4th (Create)
+				// └── to.go                        <- 3rd (Create)
 
-				// Should be able to capture an error from [fsnotify.Watcher.Add].
-				err := <-errs
+				// Should be able to capture watch error while trying to watch
+				// dir "foo".
+				err := <-errs["foo"]
 				if err == nil {
-					return fmt.Errorf("did not capture watch registration failure")
+					return fmt.Errorf("did not capture watch registration failure for dir foo")
 				}
 
 				// The file watcher should retry watch registration and
-				// eventually succeed after the file got created.
-				if err := os.WriteFile(to, []byte("package main"), 0644); err != nil {
-					return err
-				}
-
-				timer := time.NewTimer(30 * time.Second)
-				for {
-					var (
-						err error
-						ok  bool
-					)
-					select {
-					case err, ok = <-errs:
-						if !ok {
-							return fmt.Errorf("can not register watch for foo")
-						}
-					case <-timer.C:
-						return fmt.Errorf("can not register watch for foo after 30 seconds")
+				// eventually succeed watching for all dir under 'foo' after the
+				// file got created.
+				{
+					if err := os.WriteFile(to, []byte("package main"), 0644); err != nil {
+						return err
 					}
 
-					if err == nil {
-						break // watch registration success
+					timer := time.NewTimer(30 * time.Second)
+					defer timer.Stop()
+
+					var watchingFoo, watchingFooB bool
+					for {
+						if watchingFoo && watchingFooB {
+							break
+						}
+
+						select {
+						case _, ok := <-errs["foo"]:
+							if !ok {
+								watchingFoo = true
+							}
+						case _, ok := <-errs["foo/b"]:
+							if !ok {
+								watchingFooB = true
+							}
+						case <-timer.C:
+							var missing []string
+							if !watchingFoo {
+								missing = append(missing, "'foo'")
+							}
+							if !watchingFooB {
+								missing = append(missing, "'foo/b'")
+							}
+							return fmt.Errorf("timed out after 30s waiting for watches on %s to be established", strings.Join(missing, " and "))
+						}
 					}
 				}
 
 				// Once the watch registration is done, file events under the
-				// dir should be captured.
-				return os.WriteFile(filepath.Join(root, "foo", "foo.go"), []byte("package main"), 0644)
+				// dir "foo" and "foo/b" should be captured
+				if err := os.WriteFile(filepath.Join(root, "foo", "new.go"), []byte("package main"), 0644); err != nil {
+					return err
+				}
+				return os.WriteFile(filepath.Join(root, "foo", "b", "new.go"), []byte("package main"), 0644)
 			},
 			expectedEvents: []protocol.FileEvent{
+				// "foo" create event from fsnotify and synthesized create event
+				// for all entries under foo.
 				{URI: "foo", Type: protocol.Created},
-				// TODO(hxjiang): enable this after implementing retrospectively
-				// generate create events.
-				// {URI: "foo/from.go", Type: protocol.Created},
+				{URI: "foo/a.go", Type: protocol.Created},
+				{URI: "foo/b", Type: protocol.Created},
+				{URI: "foo/b/b.go", Type: protocol.Created},
+				// "to.go" creation from fsnotify.
 				{URI: "to.go", Type: protocol.Created},
-				{URI: "foo/foo.go", Type: protocol.Created},
+				// file creation event after watch retry succeeded.
+				{URI: "foo/new.go", Type: protocol.Created},
+				{URI: "foo/b/new.go", Type: protocol.Created},
 			},
 		},
 	}
@@ -278,14 +324,22 @@
 
 			root := t.TempDir()
 
-			var errs chan error
-			if tt.watchErrorPath != "" {
-				errs = make(chan error, 10)
-				filewatcher.SetAfterAddHook(func(path string, err error) {
-					if path == filepath.Join(root, tt.watchErrorPath) {
-						errs <- err
-						if err == nil {
-							close(errs)
+			var errs map[string]chan error
+			if len(tt.watchErrorPath) != 0 {
+				errs = make(map[string]chan error, len(tt.watchErrorPath))
+				for _, path := range tt.watchErrorPath {
+					errs[path] = make(chan error, 10)
+				}
+				filewatcher.SetAfterAddHook(func(path string, watchErr error) {
+					rel, err := filepath.Rel(root, path)
+					if err != nil {
+						return
+					}
+					if ch, ok := errs[filepath.ToSlash(rel)]; ok {
+						if watchErr == nil {
+							close(ch)
+						} else {
+							ch <- watchErr
 						}
 					}
 				})
@@ -303,38 +357,38 @@
 				}
 			}
 
-			matched := 0
 			foundAll := make(chan struct{})
 			var gots []protocol.FileEvent
-			eventHandler := func(events []protocol.FileEvent) {
+
+			index := 0
+			eventsHandler := func(events []protocol.FileEvent) {
+
 				gots = append(gots, events...)
+
 				// This verifies that the list of wanted events is a subsequence of
 				// the received events. It confirms not only that all wanted events
 				// are present, but also that their relative order is preserved.
 				for _, got := range events {
-					if matched == len(tt.expectedEvents) {
-						break
-					}
 					want := protocol.FileEvent{
-						URI:  protocol.URIFromPath(filepath.Join(root, string(tt.expectedEvents[matched].URI))),
-						Type: tt.expectedEvents[matched].Type,
+						URI:  protocol.URIFromPath(filepath.Join(root, string(tt.expectedEvents[index].URI))),
+						Type: tt.expectedEvents[index].Type,
 					}
 					if want == got {
-						matched++
+						index++
+					}
+					if index == len(tt.expectedEvents) {
+						close(foundAll)
 					}
 				}
-				if matched == len(tt.expectedEvents) {
-					close(foundAll)
-				}
+
 			}
 			errHandler := func(err error) {
 				t.Errorf("error from watcher: %v", err)
 			}
-			w, err := filewatcher.New(50*time.Millisecond, nil, eventHandler, errHandler)
+			w, err := filewatcher.New(50*time.Millisecond, nil, eventsHandler, errHandler)
 			if err != nil {
 				t.Fatal(err)
 			}
-
 			defer func() {
 				if err := w.Close(); err != nil {
 					t.Errorf("failed to close the file watcher: %v", err)
@@ -354,9 +408,18 @@
 			select {
 			case <-foundAll:
 			case <-time.After(30 * time.Second):
-				if matched < len(tt.expectedEvents) {
-					t.Errorf("found %v matching events\nall want: %#v\nall got: %#v", matched, tt.expectedEvents, gots)
+				if index != len(tt.expectedEvents) {
+					var want strings.Builder
+					for _, e := range tt.expectedEvents {
+						want.WriteString(fmt.Sprintf("URI: %s type: %v\n", e.URI, e.Type))
+					}
+					var got strings.Builder
+					for _, e := range gots {
+						got.WriteString(fmt.Sprintf("URI: %s type: %v\n", strings.TrimPrefix(e.URI.Path(), root+"/"), e.Type))
+					}
+					t.Errorf("found %v matching events slice\nmissing sequences:\n%s\nall got:\n%s", index, want.String(), got.String())
 				}
+
 			}
 		})
 	}
@@ -427,6 +490,9 @@
 	foundAll := make(chan struct{})
 
 	eventsHandler := func(events []protocol.FileEvent) {
+		if len(wants) == 0 { // avoid closing twice
+			return
+		}
 		for _, e := range events {
 			delete(wants, e)
 		}

Change information

Files:
  • M gopls/internal/filewatcher/filewatcher.go
  • M gopls/internal/filewatcher/filewatcher_test.go
Change size: L
Delta: 2 files changed, 204 insertions(+), 94 deletions(-)
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newchange
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 1
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Oct 29, 2025, 5:35:37 PM (8 days ago) Oct 29
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang uploaded new patchset

Hongxiang Jiang uploaded patch set #2 to this change.
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 2
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Oct 29, 2025, 5:39:37 PM (8 days ago) Oct 29
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang uploaded new patchset

Hongxiang Jiang uploaded patch set #3 to this change.
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 3
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Oct 29, 2025, 5:45:32 PM (8 days ago) Oct 29
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang uploaded new patchset

Hongxiang Jiang uploaded patch set #4 to this change.
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 4
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Nov 3, 2025, 4:59:46 PM (3 days ago) Nov 3
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang uploaded new patchset

Hongxiang Jiang uploaded patch set #6 to this change.
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 6
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Nov 3, 2025, 5:01:08 PM (3 days ago) Nov 3
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang uploaded new patchset

Hongxiang Jiang uploaded patch set #7 to this change.
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: newpatchset
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 7
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
unsatisfied_requirement
satisfied_requirement
open
diffy

Hongxiang Jiang (Gerrit)

unread,
Nov 3, 2025, 5:02:20 PM (3 days ago) Nov 3
to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com

Hongxiang Jiang voted Commit-Queue+1

Commit-Queue+1
Open in Gerrit

Related details

Attention set is empty
Submit Requirements:
  • requirement is not satisfiedCode-Review
  • requirement satisfiedNo-Unresolved-Comments
  • requirement is not satisfiedReview-Enforcement
  • requirement is not satisfiedTryBots-Pass
Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
Gerrit-MessageType: comment
Gerrit-Project: tools
Gerrit-Branch: master
Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
Gerrit-Change-Number: 716260
Gerrit-PatchSet: 7
Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
Gerrit-Reviewer: Hongxiang Jiang <hxj...@golang.org>
Gerrit-Comment-Date: Mon, 03 Nov 2025 22:02:15 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
unsatisfied_requirement
satisfied_requirement
open
diffy

Robert Findley (Gerrit)

unread,
Nov 4, 2025, 9:21:34 AM (2 days ago) Nov 4
to Hongxiang Jiang, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com
Attention needed from Hongxiang Jiang

Robert Findley added 7 comments

Patchset-level comments
File-level comment, Patchset 7 (Latest):
Robert Findley . resolved

So much easier to review!

File gopls/internal/filewatcher/filewatcher.go
Line 184, Patchset 7 (Latest): synthesized := []protocol.FileEvent{} // synthesized create events
Robert Findley . unresolved

var synthesized []protocol.FileEvent

Line 226, Patchset 7 (Latest): if len(synthesized) > 1 {
synthesized = synthesized[1:]
}
Robert Findley . unresolved

Can you just skip path == event.Name above? Might be easier to reason about.

File gopls/internal/filewatcher/filewatcher_test.go
Line 35, Patchset 7 (Latest): // If set, sends watch errors for these path to error channels passed

// to the 'changes' func. After successful watch, channel will be closed.
Robert Findley . unresolved

The most important thing for comments to explain is 'why'.

What purpose is this machinery serving? What are we trying to verify?

Line 39, Patchset 7 (Latest): changes func(root string, errs map[string]chan error) error
Robert Findley . unresolved

'setup'?

Line 200, Patchset 7 (Latest): changes: func(root string, errs map[string]chan error) error {
Robert Findley . unresolved

As far as I can tell, this is the only test that uses the errs argument, whose type is rather complicated, and whose usage is hard to understand.

It feels to me like we're forcing what's really a separate test into this framework. If we want to verify that the watcher reports an error for a broken symlink, just write a `TestBrokenSymlink` that does the minimum amount of work to verify that an error is reported. I think that test doesn't need all the machinery of this one, and separating this complicated error synchronization out of this test should make it much simpler and more maintainable--it's really hard to reason that the test won't hang when we have a map of channels keyed by file names!

Line 508, Patchset 7 (Latest): if len(wants) == 0 { // avoid closing twice
return
}
Robert Findley . unresolved

? I don't understand this. How does closing twice depend on len(wants)?

Open in Gerrit

Related details

Attention is currently required from:
  • Hongxiang Jiang
Submit Requirements:
    • requirement is not satisfiedCode-Review
    • requirement is not satisfiedNo-Unresolved-Comments
    • requirement is not satisfiedReview-Enforcement
    • requirement satisfiedTryBots-Pass
    Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
    Gerrit-MessageType: comment
    Gerrit-Project: tools
    Gerrit-Branch: master
    Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
    Gerrit-Change-Number: 716260
    Gerrit-PatchSet: 7
    Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
    Gerrit-Reviewer: Hongxiang Jiang <hxj...@golang.org>
    Gerrit-Reviewer: Robert Findley <rfin...@google.com>
    Gerrit-Attention: Hongxiang Jiang <hxj...@golang.org>
    Gerrit-Comment-Date: Tue, 04 Nov 2025 14:21:31 +0000
    Gerrit-HasComments: Yes
    Gerrit-Has-Labels: No
    unsatisfied_requirement
    satisfied_requirement
    open
    diffy

    Hongxiang Jiang (Gerrit)

    unread,
    Nov 4, 2025, 3:13:29 PM (2 days ago) Nov 4
    to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
    Attention needed from Hongxiang Jiang

    Hongxiang Jiang uploaded new patchset

    Hongxiang Jiang uploaded patch set #8 to this change.
    Following approvals got outdated and were removed:
    • TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
    Open in Gerrit

    Related details

    Attention is currently required from:
    • Hongxiang Jiang
    Submit Requirements:
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement is not satisfiedReview-Enforcement
      • requirement is not satisfiedTryBots-Pass
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: newpatchset
      Gerrit-Project: tools
      Gerrit-Branch: master
      Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
      Gerrit-Change-Number: 716260
      Gerrit-PatchSet: 8
      unsatisfied_requirement
      open
      diffy

      Hongxiang Jiang (Gerrit)

      unread,
      Nov 4, 2025, 3:17:32 PM (2 days ago) Nov 4
      to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
      Attention needed from Hongxiang Jiang

      Hongxiang Jiang uploaded new patchset

      Hongxiang Jiang uploaded patch set #9 to this change.
      Open in Gerrit

      Related details

      Attention is currently required from:
      • Hongxiang Jiang
      Submit Requirements:
      • requirement is not satisfiedCode-Review
      • requirement is not satisfiedNo-Unresolved-Comments
      • requirement is not satisfiedReview-Enforcement
      • requirement is not satisfiedTryBots-Pass
      Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
      Gerrit-MessageType: newpatchset
      Gerrit-Project: tools
      Gerrit-Branch: master
      Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
      Gerrit-Change-Number: 716260
      Gerrit-PatchSet: 9
      unsatisfied_requirement
      open
      diffy

      Hongxiang Jiang (Gerrit)

      unread,
      Nov 4, 2025, 3:20:09 PM (2 days ago) Nov 4
      to goph...@pubsubhelper.golang.org, Robert Findley, Go LUCI, golang-co...@googlegroups.com
      Attention needed from Robert Findley

      Hongxiang Jiang voted and added 6 comments

      Votes added by Hongxiang Jiang

      Commit-Queue+1

      6 comments

      File gopls/internal/filewatcher/filewatcher.go
      Line 184, Patchset 7: synthesized := []protocol.FileEvent{} // synthesized create events
      Robert Findley . resolved

      var synthesized []protocol.FileEvent

      Hongxiang Jiang

      Done

      Line 226, Patchset 7: if len(synthesized) > 1 {
      synthesized = synthesized[1:]
      }
      Robert Findley . resolved

      Can you just skip path == event.Name above? Might be easier to reason about.

      Hongxiang Jiang

      Done

      File gopls/internal/filewatcher/filewatcher_test.go
      Line 35, Patchset 7: // If set, sends watch errors for these path to error channels passed

      // to the 'changes' func. After successful watch, channel will be closed.
      Robert Findley . resolved

      The most important thing for comments to explain is 'why'.

      What purpose is this machinery serving? What are we trying to verify?

      Hongxiang Jiang

      Done. Explanation added to the TestBrokenSymlink test case.

      Line 39, Patchset 7: changes func(root string, errs map[string]chan error) error
      Robert Findley . resolved

      'setup'?

      Hongxiang Jiang

      Done

      Line 200, Patchset 7: changes: func(root string, errs map[string]chan error) error {
      Robert Findley . resolved

      As far as I can tell, this is the only test that uses the errs argument, whose type is rather complicated, and whose usage is hard to understand.

      It feels to me like we're forcing what's really a separate test into this framework. If we want to verify that the watcher reports an error for a broken symlink, just write a `TestBrokenSymlink` that does the minimum amount of work to verify that an error is reported. I think that test doesn't need all the machinery of this one, and separating this complicated error synchronization out of this test should make it much simpler and more maintainable--it's really hard to reason that the test won't hang when we have a map of channels keyed by file names!

      Hongxiang Jiang

      SG. I have moved that as a separate test.

      Line 508, Patchset 7: if len(wants) == 0 { // avoid closing twice
      return
      }
      Robert Findley . resolved

      ? I don't understand this. How does closing twice depend on len(wants)?

      Hongxiang Jiang

      events handler can be called multiple times. It's possible we receiver more events after we found all the expected events.

      We delet entries from the map when we see events we want to see.

      The first time we found all the events, the map's length is 0, we close the foundAll. But if later, more events arrived, we need to avoid closing foundAll.

      Open in Gerrit

      Related details

      Attention is currently required from:
      • Robert Findley
      Submit Requirements:
        • requirement is not satisfiedCode-Review
        • requirement satisfiedNo-Unresolved-Comments
        • requirement is not satisfiedReview-Enforcement
        • requirement is not satisfiedTryBots-Pass
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: tools
        Gerrit-Branch: master
        Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
        Gerrit-Change-Number: 716260
        Gerrit-PatchSet: 9
        Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Robert Findley <rfin...@google.com>
        Gerrit-Attention: Robert Findley <rfin...@google.com>
        Gerrit-Comment-Date: Tue, 04 Nov 2025 20:20:04 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        Comment-In-Reply-To: Robert Findley <rfin...@google.com>
        unsatisfied_requirement
        satisfied_requirement
        open
        diffy

        Robert Findley (Gerrit)

        unread,
        Nov 4, 2025, 3:22:58 PM (2 days ago) Nov 4
        to Hongxiang Jiang, goph...@pubsubhelper.golang.org, Go LUCI, golang-co...@googlegroups.com
        Attention needed from Hongxiang Jiang

        Robert Findley voted and added 3 comments

        Votes added by Robert Findley

        Code-Review+2

        3 comments

        Patchset-level comments
        File-level comment, Patchset 9 (Latest):
        Robert Findley . resolved

        Very nice!

        File gopls/internal/filewatcher/filewatcher_test.go
        Line 39, Patchset 7: changes func(root string, errs map[string]chan error) error
        Robert Findley . unresolved

        'setup'?

        Hongxiang Jiang

        Done

        Robert Findley

        Oh! This was a bad suggestion! I thought we were setting it up beforehand, but I see that we're actually making changes to expect events.

        Feel free to revert, and to tell me when my suggestions are bad 😊

        Line 286, Patchset 9 (Latest):func TestBrokenSymlink(t *testing.T) {
        Robert Findley . unresolved

        If we're just testing one thing, this test seems awfully large. Can't we just have a single broken symlink? The test for traversal is really implemented above.

        Open in Gerrit

        Related details

        Attention is currently required from:
        • Hongxiang Jiang
        Submit Requirements:
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        • requirement is not satisfiedTryBots-Pass
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: tools
        Gerrit-Branch: master
        Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
        Gerrit-Change-Number: 716260
        Gerrit-PatchSet: 9
        Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Robert Findley <rfin...@google.com>
        Gerrit-Attention: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Comment-Date: Tue, 04 Nov 2025 20:22:54 +0000
        Gerrit-HasComments: Yes
        Gerrit-Has-Labels: Yes
        Comment-In-Reply-To: Hongxiang Jiang <hxj...@golang.org>
        Comment-In-Reply-To: Robert Findley <rfin...@google.com>
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Hongxiang Jiang (Gerrit)

        unread,
        Nov 4, 2025, 3:34:29 PM (2 days ago) Nov 4
        to goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
        Attention needed from Hongxiang Jiang

        Hongxiang Jiang uploaded new patchset

        Hongxiang Jiang uploaded patch set #10 to this change.
        Open in Gerrit

        Related details

        Attention is currently required from:
        • Hongxiang Jiang
        Submit Requirements:
        • requirement satisfiedCode-Review
        • requirement is not satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        • requirement is not satisfiedTryBots-Pass
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: newpatchset
        Gerrit-Project: tools
        Gerrit-Branch: master
        Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
        Gerrit-Change-Number: 716260
        Gerrit-PatchSet: 10
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Hongxiang Jiang (Gerrit)

        unread,
        Nov 4, 2025, 3:35:56 PM (2 days ago) Nov 4
        to goph...@pubsubhelper.golang.org, Go LUCI, Robert Findley, golang-co...@googlegroups.com

        Hongxiang Jiang voted and added 2 comments

        Votes added by Hongxiang Jiang

        Auto-Submit+1
        Commit-Queue+1

        2 comments

        File gopls/internal/filewatcher/filewatcher_test.go
        Line 39, Patchset 7: changes func(root string, errs map[string]chan error) error
        Robert Findley . resolved

        'setup'?

        Hongxiang Jiang

        Done

        Robert Findley

        Oh! This was a bad suggestion! I thought we were setting it up beforehand, but I see that we're actually making changes to expect events.

        Feel free to revert, and to tell me when my suggestions are bad 😊

        Hongxiang Jiang

        Done

        Line 286, Patchset 9:func TestBrokenSymlink(t *testing.T) {
        Robert Findley . resolved

        If we're just testing one thing, this test seems awfully large. Can't we just have a single broken symlink? The test for traversal is really implemented above.

        Hongxiang Jiang

        SG. I have remove the sub dir foo/a and foo/b. So we only wait for foo's watch error and wait for foo's watch to succeed.

        Open in Gerrit

        Related details

        Attention set is empty
        Submit Requirements:
        • requirement satisfiedCode-Review
        • requirement satisfiedNo-Unresolved-Comments
        • requirement satisfiedReview-Enforcement
        • requirement is not satisfiedTryBots-Pass
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        Gerrit-MessageType: comment
        Gerrit-Project: tools
        Gerrit-Branch: master
        Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
        Gerrit-Change-Number: 716260
        Gerrit-PatchSet: 10
        Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Hongxiang Jiang <hxj...@golang.org>
        Gerrit-Reviewer: Robert Findley <rfin...@google.com>
        Gerrit-Comment-Date: Tue, 04 Nov 2025 20:35:52 +0000
        satisfied_requirement
        unsatisfied_requirement
        open
        diffy

        Gopher Robot (Gerrit)

        unread,
        Nov 4, 2025, 3:55:39 PM (2 days ago) Nov 4
        to Hongxiang Jiang, goph...@pubsubhelper.golang.org, golang-...@googlegroups.com, Go LUCI, Robert Findley, golang-co...@googlegroups.com

        Gopher Robot submitted the change with unreviewed changes

        Unreviewed changes

        9 is the latest approved patch-set.
        The change was submitted with unreviewed changes in the following files:

        ```
        The name of the file: gopls/internal/filewatcher/filewatcher_test.go
        Insertions: 31, Deletions: 68.

        @@ -33,7 +33,7 @@

         		name           string
         		goos           []string // if not empty, only run in these OS.
         		initWorkspace  string
        -		setup          func(root string) error
        +		changes        func(root string) error
         		expectedEvents []protocol.FileEvent
         	}{
         		{
        @@ -43,7 +43,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644)
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -57,7 +57,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.WriteFile(filepath.Join(root, "bar.go"), []byte("package main"), 0644)
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -71,7 +71,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.WriteFile(filepath.Join(root, "foo.go"), []byte("package main // modified"), 0644)
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -86,7 +86,7 @@

         -- bar.go --
         package bar
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Remove(filepath.Join(root, "foo.go"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -100,7 +100,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -115,7 +115,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Rename(filepath.Join(root, "foo.go"), filepath.Join(root, "bar.go"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -129,7 +129,7 @@

         -- foo.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Mkdir(filepath.Join(root, "bar"), 0755)
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -142,7 +142,7 @@

         -- foo/bar.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.RemoveAll(filepath.Join(root, "foo"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -164,7 +164,7 @@

         -- foo/bar.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -180,7 +180,7 @@

         -- foo/bar.go --
         package foo
         `,
        -			setup: func(root string) error {
        +			changes: func(root string) error {

         				return os.Rename(filepath.Join(root, "foo"), filepath.Join(root, "baz"))
         			},
         			expectedEvents: []protocol.FileEvent{
        @@ -257,8 +257,8 @@
         				t.Fatal(err)
         			}
         
        -			if tt.setup != nil {
        -				if err := tt.setup(root); err != nil {
        +			if tt.changes != nil {
        +				if err := tt.changes(root); err != nil {
         					t.Fatal(err)
         				}
         			}
        @@ -293,21 +293,17 @@
         	// watchErrs is used to capture watch errors during directory monitoring.
         	// This mechanism allows the test to assert that specific directory watches
         	// initially fail and subsequently recover upon fixing the broken symlink.
        -	watchErrs := map[string]chan error{
        -		"foo":   make(chan error, 10),
        -		"foo/a": make(chan error, 10),
        -		"foo/b": make(chan error, 10),
        -	}
        +	watchErrs := make(chan error, 10)
         	filewatcher.SetAfterAddHook(func(path string, watchErr error) {

         		rel, err := filepath.Rel(root, path)
         		if err != nil {
         			return
         		}
        -		if ch, ok := watchErrs[filepath.ToSlash(rel)]; ok {
        +		if rel == "foo" {
         			if watchErr == nil {
        -				close(ch)
        +				close(watchErrs)
         			} else {
        -				ch <- watchErr
        +				watchErrs <- watchErr
         			}
         		}
         	})
        @@ -322,16 +318,13 @@

         		// "foo" create event from fsnotify and synthesized create event
         		// for all entries under foo.
         		{URI: "foo", Type: protocol.Created},
        -		{URI: "foo/a", Type: protocol.Created},
        -		{URI: "foo/a/a.go", Type: protocol.Created},
        -		{URI: "foo/b", Type: protocol.Created},
        -		{URI: "foo/b/b.go", Type: protocol.Created},

        +		{URI: "foo/a.go", Type: protocol.Created},
        +		{URI: "foo/b.go", Type: protocol.Created},

         		{URI: "foo/from.go", Type: protocol.Created},
         		// "to.go" creation from fsnotify.
         		{URI: "to.go", Type: protocol.Created},
         		// file creation event after watch retry succeeded.
         		{URI: "foo/new.go", Type: protocol.Created},
        -		{URI: "foo/b/new.go", Type: protocol.Created},
         	}
         	eventsHandler := func(events []protocol.FileEvent) {
         		gots = append(gots, events...)
        @@ -376,17 +369,15 @@

         		// Prepare a dir with with broken symbolic link.
         		// foo                       <- 1st

         		// ├── from.go -> root/to.go <- 1st
        -		// ├── a                     <- 1st
        -		// │   └── a.go.             <- 1st
        -		// └── b                     <- 1st
        -		//     └── b.go.             <- 1st

        +		// ├── a.go                  <- 1st
        +		// └── b.go                  <- 1st

         
         		to := filepath.Join(root, "to.go")
         
         		archive := txtar.Parse([]byte(`
        --- a/a.go --
        +-- a.go --
         package a
        --- b/b.go --
        +-- b.go --
         package b
         `))

         		tmp := filepath.Join(t.TempDir(), "foo")
        @@ -415,17 +406,14 @@
         
         		// root

         		// ├── foo                          <- 2nd (Move)
        -		// │   ├── a                        <- 2nd (Move)
        -		// │   │   └── a.go                 <- 2nd (Move)
        -		// │   ├── b                        <- 2nd (Move)
        -		// │   │   ├── b.go                 <- 2nd (Move)
        -		// │   │   └── new.go               <- 4th (Create)

        +		// │   ├── a.go                     <- 2nd (Move)
        +		// │   ├── b.go                     <- 2nd (Move)

         		// │   ├── from.go -> ../../to.go   <- 2nd (Move)
         		// │   └── new.go                   <- 4th (Create)
         		// └── to.go                        <- 3rd (Create)
         
         		// Should be able to capture watch error while trying to watch dir "foo".
        -		if err := <-watchErrs["foo"]; err == nil {
        +		if err := <-watchErrs; err == nil {

         			t.Errorf("did not capture watch registration failure for dir foo")
         		}
         
        @@ -439,49 +427,24 @@

         			timer := time.NewTimer(30 * time.Second)
         			defer timer.Stop()
         
        -			var watchingFoo, watchingFooA, watchingFooB bool
        +		outer:
         			for {
        -				if watchingFoo && watchingFooA && watchingFooB {
        -					break
        -				}
        -
         				select {
        -				case _, ok := <-watchErrs["foo"]:
        +				case _, ok := <-watchErrs:
         					if !ok {
        -						watchingFoo = true
        -					}
        -				case _, ok := <-watchErrs["foo/a"]:
        -					if !ok {
        -						watchingFooA = true
        -					}
        -				case _, ok := <-watchErrs["foo/b"]:
        -					if !ok {
        -						watchingFooB = true
        +						break outer
         					}
         				case <-timer.C:
        -					var missing []string
        -					if !watchingFoo {
        -						missing = append(missing, "'foo'")
        -					}
        -					if !watchingFooA {
        -						missing = append(missing, "'foo/a'")
        -					}
        -					if !watchingFooB {
        -						missing = append(missing, "'foo/b'")
        -					}
        -					t.Errorf("timed out after 30s waiting for watches on %s to be established", strings.Join(missing, " , "))
        +					t.Errorf("timed out after 30s waiting for watches on foo to be established")

         				}
         			}
         		}
         
         		// Once the watch registration is done, file events under the
        -		// dir "foo" and "foo/b" should be captured
        +		// dir "foo" should be captured

         		if err := os.WriteFile(filepath.Join(root, "foo", "new.go"), []byte("package main"), 0644); err != nil {
         			t.Fatalf("fail to write file %v", err)
         		}
        -		if err := os.WriteFile(filepath.Join(root, "foo", "b", "new.go"), []byte("package main"), 0644); err != nil {
        -			t.Fatalf("fail to write file %v", err)
        -		}
         	}
         
         	select {
        ```

        Change information

        Commit message:
        gopls/internal/filewatcher: synthesize events after watching a dir

        Previously, file or subdirectory creations could be missed if they
        occurred after a directory was created but before the file watcher was
        registered for it. This resulted in missed events for all subsequent
        operations within that subdirectory.

        This change addresses this gap by performing a breadth-first scan of a
        new directory immediately after the watch is established. The scan
        iterates over the directory's entries, synthesizes `Create` events for
        each one.

        The BFS(pre-order) make sure the synthesized events follow logical
        order: the parent dir creation events always preceds those of their
        children.


        The root dir creation event and all synthesized `Create` are added to
        the out atomically to make sure the out slice is always holding a
        logically correct file events and ready to flush anytime.

        Note: file/dir creation events maybe duplicated, one captured by the
        scan, one captured by fsnotify but it is promised there is no miss.

        For golang/go#74292
        Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160
        Reviewed-by: Robert Findley <rfin...@google.com>
        Auto-Submit: Hongxiang Jiang <hxj...@golang.org>
        Files:
        • M gopls/internal/filewatcher/filewatcher.go
        • M gopls/internal/filewatcher/filewatcher_test.go
        Change size: L
        Delta: 2 files changed, 289 insertions(+), 145 deletions(-)
        Branch: refs/heads/master
        Submit Requirements:
        • requirement satisfiedCode-Review: +2 by Robert Findley
        • requirement satisfiedTryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
         Open in Gerrit
        Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. DiffyGerrit
        


         Gerrit-MessageType: merged 
        

         Gerrit-Project: tools 
        

         Gerrit-Branch: master 
        

         Gerrit-Change-Id: Id9b63887d9a7fc83e608bf8390b2f7c198cfc160 
        

         Gerrit-Change-Number: 716260 
        

         Gerrit-PatchSet: 11 
        

         Gerrit-Owner: Hongxiang Jiang <hxj...@golang.org> 
        


         Gerrit-Reviewer: Gopher Robot <go...@golang.org> 
        open
        diffy
        satisfied_requirement
        Reply all
        Reply to author
        Forward
        0 new messages