Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kevinburkemeter opened this issue Aug 6, 2019 · 8 comments
Assignees
Milestone

Comments

@kevinburkemeter
Copy link

@kevinburkemeter kevinburkemeter commented Aug 6, 2019

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.

@gopherbot gopherbot added this to the Unreleased milestone Aug 6, 2019
@agnivade

This comment has been minimized.

Copy link
Contributor

@agnivade agnivade commented Aug 6, 2019

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.

@agnivade agnivade self-assigned this Aug 6, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

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

@kevinburke

This comment has been minimized.

Copy link
Contributor

@kevinburke kevinburke commented Aug 9, 2019

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

This comment has been minimized.

Copy link
Contributor

@kevinburke kevinburke commented Aug 9, 2019

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

This comment has been minimized.

Copy link
Contributor

@kevinburke kevinburke commented Aug 9, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Aug 9, 2019

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

@dmitshur dmitshur self-assigned this Sep 28, 2019
@dmitshur

This comment has been minimized.

Copy link
Member

@dmitshur dmitshur commented Sep 29, 2019

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.

@dmitshur dmitshur added the Tools label Sep 29, 2019
@dmitshur dmitshur changed the title x/website: unclear log messages on startup x/tools/godoc/vfs: ReadDir reports files as directories after Bind was called on a file Sep 29, 2019
@agnivade agnivade removed their assignment Sep 29, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 6, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.