Go: x/tools/godoc/vfs: ReadDir reports files as directories after Bind was called on a file

[kevinburkemeter]

Running golang.googlesource.com/go, x/website and x/tools at tip, I run the golangorg --index=false binary and immediately get this log message:

2019/08/06 10:34:12 updateMetadata: file does not exist
2019/08/06 10:34:12 updateMetadata: file does not exist

It's not clear where this is coming from, which file does not exist, or what action I am supposed to take. (The website loads fine). It might be good to make the error message clearer in this case.

[agnivade]

I have seen it too. I had to modify the log line to see which file. It is /doc/copyright.html. That file is in content/static/doc/. But the vfs.ReadFile call is failing. I will take a look when I have some time.

[gopherbot]

Change https://golang.org/cl/189560 mentions this issue: godoc: better error message on error

[kevinburke]

Agreed, it is doc/copyright.html. godoc/meta.go is attempting to read it as a directory, but failing because it is a plain file.

    scan = func(dir string) {
        fis, err := c.fs.ReadDir(dir)
        if err != nil {
            log.Printf("updateMetadata: error reading directory %q: %v\n", dir, err)
            return
        }
        for _, fi := range fis {
            name := pathpkg.Join(dir, fi.Name())
            if fi.IsDir() {
                fmt.Println("fi", fi.Name(), "is dir", "scan", name)
                scan(name) // recurse
                continue
            }

prints:

fi wiki is dir scan /doc/articles/wiki
fi codewalk is dir scan /doc/codewalk
fi copyright.html is dir scan /doc/copyright.html
2019/08/09 07:40:03 updateMetadata: error reading directory "/doc/copyright.html": file does not exist

Seems odd!!!

[kevinburke]

Hmmm... c.fs.ReadDir returns a value that says "/doc/copyright.html" is a directory, but c.fs.Stat returns a value that says it is not. I think it is related to the fact that /doc/copyright.html is mounted with fs.Bind, here:

        fs.Bind("/doc/copyright.html", mapfs.New(static.Files), "/doc/copyright.html", vfs.BindReplace)

This logic in vfs/namespace.go:ReadDir assumes that any mount point root must be a directory, which is incorrect.

    for old := range ns {
        if hasPathPrefix(old, path) && old != path {
            // Find next element after path in old.
            elem := old[len(path):]
            elem = strings.TrimPrefix(elem, "/")
            if i := strings.Index(elem, "/"); i >= 0 {
                elem = elem[:i]
            }
            if !haveName[elem] {
                haveName[elem] = true
                all = append(all, dirInfo(elem))
            }
        }
    }

[kevinburke]

Here's a patch that fixes the issue, I believe.

$ git diff
diff --git a/godoc/vfs/namespace.go b/godoc/vfs/namespace.go
index b8a1122d..4e495750 100644
--- a/godoc/vfs/namespace.go
+++ b/godoc/vfs/namespace.go
@@ -366,6 +366,10 @@ func (ns NameSpace) ReadDir(path string) ([]os.FileInfo, error) {
            if i := strings.Index(elem, "/"); i >= 0 {
                elem = elem[:i]
            }
+           stat, _ := ns.Stat(old)
+           if stat != nil && !stat.IsDir() {
+               continue
+           }
            if !haveName[elem] {
                haveName[elem] = true
                all = append(all, dirInfo(elem))

[gopherbot]

Change https://golang.org/cl/189657 mentions this issue: godoc/vfs: files bound as root are treated as files

[dmitshur]

I've investigated this a bit.

That fix above is a good start and helpful, but it's incomplete. I've identified these two problems.

First, it makes it so that directories are skipped altogether if the bound file is inside a more-nested directory. E.g., if there was:

ns.Bind("/doc/inner/file.html", newMount, "/doc/inner/file.html", vfs.BindReplace)

Doing ns.ReadDir("/doc") should include the "inner" directory, but it gets skipped because ns.Stat(old) will stat "/doc/inner/file.html", which is a file, rather than "/doc/inner", a directory.

This can be fixed in a computationally inexpensive way: if old[len(path):] has more than 2 elements (e.g., "/inner/file.html"), then we can be pretty safe in assuming the first one must be a directory.

Second, it doesn't include the file itself as part of the ReadDir output. We want to fix that (see also issue #34571, /cc @jayconrod). Since Stat was already called, it's as easy as using its output and adding that to all. However, I worry a bit about how expensive calling Stat is; we should see if it can be done more directly.

[gopherbot]

Change https://golang.org/cl/205661 mentions this issue: godoc/vfs: include dirs needed to reach mount points in Stat