cmd/go: avoid spurious readdir during fsys.Walk

fsys.Walk is cloned from filepath.Walk, which has always handled
a walk of a directory by reading the full directory before calling the
callback on the directory itself. So if the callback returns fs.SkipDir,
those entries are thrown away, but the expense of reading them was
still incurred. (Worse, this is the expensive directory read that also
calls Stat on every entry.) On machines with slow file system I/O,
these reads are particularly annoying. For example, if I do

	go list m...

there is a call to filepath.Walk that is told about $GOROOT/src/archive
and responds by returning filepath.SkipDir because archive does not
start with m, but it only gets the chance to do that after the archive
directory has been read. (Same for all the other top-level directories.)
Even something like go list github.com/foo/bar/... reads every top-level
$GOPATH/src directory.

When we designed filepath.WalkDir, one of the changes we made was
to allow calling the callback twice for a directory: once before reading it,
and then possibly again if the read produces an error (uncommon).
This CL changes fsys.Walk to use that same model. None of the callbacks
need changing, but now the $GOROOT/src/archive and other top-level
directories won't be read when evaluating a pattern like 'm...'.

For #53577.
Fixes #53765.

Change-Id: Idfa3b9e2cc335417bfd9d66dd584cb16f92bd12e
Reviewed-on: https://go-review.googlesource.com/c/go/+/416179
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>

diff --git a/src/cmd/go/internal/fsys/fsys.go b/src/cmd/go/internal/fsys/fsys.go
index d96a290de5094771249ba96333a8f1eedec715ad..0d7bef911266b002255216c67dd9d11895a661e4 100644 (file)
--- a/src/cmd/go/internal/fsys/fsys.go
+++ b/src/cmd/go/internal/fsys/fsys.go
@@ -464,28 +464,20 @@ func IsDirWithGoFiles(dir string) (bool, error) {
 // walk recursively descends path, calling walkFn. Copied, with some
 // modifications from path/filepath.walk.
 func walk(path string, info fs.FileInfo, walkFn filepath.WalkFunc) error {
-       if !info.IsDir() {
-               return walkFn(path, info, nil)
+       if err := walkFn(path, info, nil); err != nil || !info.IsDir() {
+               return err
        }
 
-       fis, readErr := ReadDir(path)
-       walkErr := walkFn(path, info, readErr)
-       // If readErr != nil, walk can't walk into this directory.
-       // walkErr != nil means walkFn want walk to skip this directory or stop walking.
-       // Therefore, if one of readErr and walkErr isn't nil, walk will return.
-       if readErr != nil || walkErr != nil {
-               // The caller's behavior is controlled by the return value, which is decided
-               // by walkFn. walkFn may ignore readErr and return nil.
-               // If walkFn returns SkipDir, it will be handled by the caller.
-               // So walk should return whatever walkFn returns.
-               return walkErr
+       fis, err := ReadDir(path)
+       if err != nil {
+               return walkFn(path, info, err)
        }
 
        for _, fi := range fis {
                filename := filepath.Join(path, fi.Name())
-               if walkErr = walk(filename, fi, walkFn); walkErr != nil {
-                       if !fi.IsDir() || walkErr != filepath.SkipDir {
-                               return walkErr
+               if err := walk(filename, fi, walkFn); err != nil {
+                       if !fi.IsDir() || err != filepath.SkipDir {
+                               return err
                        }
                }
        }

diff --git a/src/cmd/go/testdata/script/fsys_walk.txt b/src/cmd/go/testdata/script/fsys_walk.txt
new file mode 100644 (file)
index 0000000..9d1a945
--- /dev/null
+++ b/src/cmd/go/testdata/script/fsys_walk.txt
@@ -0,0 +1,6 @@
+# Test that go list prefix... does not read directories not beginning with prefix.
+env GODEBUG=gofsystrace=1
+go list m...
+stderr mime
+stderr mime[\\/]multipart
+! stderr archive

diff --git a/src/cmd/go/testdata/script/list_permissions.txt b/src/cmd/go/testdata/script/list_perm.txt
similarity index 99%
rename from src/cmd/go/testdata/script/list_permissions.txt
rename to src/cmd/go/testdata/script/list_perm.txt
index f65896ca14ee62fdeaabe52dbb8648f8a99fe7f2..3b850ef3cc80740da01d4a9a8f3b1316f5a1549b 100644 (file)
--- a/src/cmd/go/testdata/script/list_permissions.txt
+++ b/src/cmd/go/testdata/script/list_perm.txt
@@ -11,12 +11,11 @@ stdout '^example.com/noread$'
 go list ./empty/...
 stderr 'matched no packages'
 
-[root] stop # Root typically ignores file permissions.
-
 # Make the directory ./noread unreadable, and verify that 'go list' reports an
 # explicit error for a pattern that should match it (rather than treating it as
 # equivalent to an empty directory).
 
+[root] stop # Root typically ignores file permissions.
 [windows] skip # Does not have Unix-style directory permissions.
 [plan9] skip   # Might not have Unix-style directory permissions.
 

diff --git a/src/cmd/go/testdata/script/mod_perm.txt b/src/cmd/go/testdata/script/mod_perm.txt
new file mode 100644 (file)
index 0000000..f5382ec
--- /dev/null
+++ b/src/cmd/go/testdata/script/mod_perm.txt
@@ -0,0 +1,23 @@
+# go list should work in ordinary conditions.
+go list ./...
+! stdout _data
+
+# skip in conditions where chmod 0 may not work.
+# plan9 should be fine, but copied from list_perm.txt unchanged.
+[root] skip
+[windows] skip
+[plan9] skip
+
+# go list should work with unreadable _data directory.
+chmod 0 _data
+go list ./...
+! stdout _data
+
+-- go.mod --
+module m
+
+-- x.go --
+package m
+
+-- _data/x.go --
+package p