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
kevinburke1 opened this issue Aug 6, 2019 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@kevinburke1
Copy link

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
Copy link
Contributor

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
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 6, 2019
@gopherbot
Copy link

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

@kevinburke
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

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 This label describes issues relating to any tools in the x/tools repository. 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
Copy link

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

@dmitshur dmitshur removed their assignment Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

5 participants