godoc/vfs: fix union logic in NameSpace.ReadDir

ReadDir now returns files from directories in all matching mount
points if no Go files are present in any of them. The behavior now
matches the documentation.

Fixes golang/go#34571

Change-Id: I3a0c8d49a5906ec33ebe9e3efea9d2b9d267506c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/197801
Run-TryBot: Jay Conrod <jayconrod@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>

3 files changed, 165 insertions, 100 deletions

diff --git a/godoc/vfs/emptyvfs_test.go b/godoc/vfs/emptyvfs_test.go
deleted file mode 100644
index 8ea9b0164..000000000
--- a/godoc/vfs/emptyvfs_test.go
+++ /dev/null
@@ -1,58 +0,0 @@
-// Copyright 2016 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package vfs_test
-
-import (
-	"testing"
-	"time"
-
-	"golang.org/x/tools/godoc/vfs"
-	"golang.org/x/tools/godoc/vfs/mapfs"
-)
-
-func TestNewNameSpace(t *testing.T) {
-
-	// We will mount this filesystem under /fs1
-	mount := mapfs.New(map[string]string{"fs1file": "abcdefgh"})
-
-	// Existing process. This should give error on Stat("/")
-	t1 := vfs.NameSpace{}
-	t1.Bind("/fs1", mount, "/", vfs.BindReplace)
-
-	// using NewNameSpace. This should work fine.
-	t2 := vfs.NewNameSpace()
-	t2.Bind("/fs1", mount, "/", vfs.BindReplace)
-
-	testcases := map[string][]bool{
-		"/":            {false, true},
-		"/fs1":         {true, true},
-		"/fs1/fs1file": {true, true},
-	}
-
-	fss := []vfs.FileSystem{t1, t2}
-
-	for j, fs := range fss {
-		for k, v := range testcases {
-			_, err := fs.Stat(k)
-			result := err == nil
-			if result != v[j] {
-				t.Errorf("fs: %d, testcase: %s, want: %v, got: %v, err: %s", j, k, v[j], result, err)
-			}
-		}
-	}
-
-	fi, err := t2.Stat("/")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if fi.Name() != "/" {
-		t.Errorf("t2.Name() : want:%s got:%s", "/", fi.Name())
-	}
-
-	if !fi.ModTime().IsZero() {
-		t.Errorf("t2.Modime() : want:%v got:%v", time.Time{}, fi.ModTime())
-	}
-}
diff --git a/godoc/vfs/namespace.go b/godoc/vfs/namespace.go
index 4679bd45b..227be571b 100644
--- a/godoc/vfs/namespace.go
+++ b/godoc/vfs/namespace.go
@@ -298,66 +298,52 @@ var startTime = time.Now()
 func (ns NameSpace) ReadDir(path string) ([]os.FileInfo, error) {
 	path = ns.clean(path)
 
+	// List matching directories and determine whether any of them contain
+	// Go files.
 	var (
-		haveGo   = false
-		haveName = map[string]bool{}
-		all      []os.FileInfo
-		err      error
-		first    []os.FileInfo
+		dirs       [][]os.FileInfo
+		goDirIndex = -1
+		readDirErr error
 	)
 
 	for _, m := range ns.resolve(path) {
-		dir, err1 := m.fs.ReadDir(m.translate(path))
-		if err1 != nil {
-			if err == nil {
-				err = err1
+		dir, err := m.fs.ReadDir(m.translate(path))
+		if err != nil {
+			if readDirErr == nil {
+				readDirErr = err
 			}
 			continue
 		}
 
-		if dir == nil {
-			dir = []os.FileInfo{}
-		}
-
-		if first == nil {
-			first = dir
-		}
+		dirs = append(dirs, dir)
 
-		// If we don't yet have Go files in 'all' and this directory
-		// has some, add all the files from this directory.
-		// Otherwise, only add subdirectories.
-		useFiles := false
-		if !haveGo {
-			for _, d := range dir {
-				if strings.HasSuffix(d.Name(), ".go") {
-					useFiles = true
-					haveGo = true
+		if goDirIndex < 0 {
+			for _, f := range dir {
+				if !f.IsDir() && strings.HasSuffix(f.Name(), ".go") {
+					goDirIndex = len(dirs) - 1
 					break
 				}
 			}
 		}
-
-		for _, d := range dir {
-			name := d.Name()
-			if (d.IsDir() || useFiles) && !haveName[name] {
-				haveName[name] = true
-				all = append(all, d)
-			}
-		}
 	}
 
-	// We didn't find any directories containing Go files.
-	// If some directory returned successfully, use that.
-	if !haveGo {
-		for _, d := range first {
-			if !haveName[d.Name()] {
-				haveName[d.Name()] = true
-				all = append(all, d)
+	// Build a list of files and subdirectories. If a directory contains Go files,
+	// only include files from that directory. Otherwise, include files from
+	// all directories. Include subdirectories from all directories regardless
+	// of whether Go files are present.
+	haveName := make(map[string]bool)
+	var all []os.FileInfo
+	for i, dir := range dirs {
+		for _, f := range dir {
+			name := f.Name()
+			if !haveName[name] && (f.IsDir() || goDirIndex < 0 || goDirIndex == i) {
+				all = append(all, f)
+				haveName[name] = true
 			}
 		}
 	}
 
-	// Built union.  Add any missing directories needed to reach mount points.
+	// Add any missing directories needed to reach mount points.
 	for old := range ns {
 		if hasPathPrefix(old, path) && old != path {
 			// Find next element after path in old.
@@ -374,7 +360,7 @@ func (ns NameSpace) ReadDir(path string) ([]os.FileInfo, error) {
 	}
 
 	if len(all) == 0 {
-		return nil, err
+		return nil, readDirErr
 	}
 
 	sort.Sort(byName(all))
diff --git a/godoc/vfs/namespace_test.go b/godoc/vfs/namespace_test.go
new file mode 100644
index 000000000..edf3bc750
--- /dev/null
+++ b/godoc/vfs/namespace_test.go
@@ -0,0 +1,137 @@
+// Copyright 2016 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package vfs_test
+
+import (
+	"fmt"
+	"strings"
+	"testing"
+	"time"
+
+	"golang.org/x/tools/godoc/vfs"
+	"golang.org/x/tools/godoc/vfs/mapfs"
+)
+
+func TestNewNameSpace(t *testing.T) {
+
+	// We will mount this filesystem under /fs1
+	mount := mapfs.New(map[string]string{"fs1file": "abcdefgh"})
+
+	// Existing process. This should give error on Stat("/")
+	t1 := vfs.NameSpace{}
+	t1.Bind("/fs1", mount, "/", vfs.BindReplace)
+
+	// using NewNameSpace. This should work fine.
+	t2 := vfs.NewNameSpace()
+	t2.Bind("/fs1", mount, "/", vfs.BindReplace)
+
+	testcases := map[string][]bool{
+		"/":            {false, true},
+		"/fs1":         {true, true},
+		"/fs1/fs1file": {true, true},
+	}
+
+	fss := []vfs.FileSystem{t1, t2}
+
+	for j, fs := range fss {
+		for k, v := range testcases {
+			_, err := fs.Stat(k)
+			result := err == nil
+			if result != v[j] {
+				t.Errorf("fs: %d, testcase: %s, want: %v, got: %v, err: %s", j, k, v[j], result, err)
+			}
+		}
+	}
+
+	fi, err := t2.Stat("/")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if fi.Name() != "/" {
+		t.Errorf("t2.Name() : want:%s got:%s", "/", fi.Name())
+	}
+
+	if !fi.ModTime().IsZero() {
+		t.Errorf("t2.ModTime() : want:%v got:%v", time.Time{}, fi.ModTime())
+	}
+}
+
+func TestReadDirUnion(t *testing.T) {
+	for _, tc := range []struct {
+		desc       string
+		ns         vfs.NameSpace
+		path, want string
+	}{
+		{
+			desc: "no_go_files",
+			ns: func() vfs.NameSpace {
+				rootFs := mapfs.New(map[string]string{
+					"doc/a.txt":       "1",
+					"doc/b.txt":       "1",
+					"doc/dir1/d1.txt": "",
+				})
+				docFs := mapfs.New(map[string]string{
+					"doc/a.txt":       "22",
+					"doc/dir2/d2.txt": "",
+				})
+				ns := vfs.NameSpace{}
+				ns.Bind("/", rootFs, "/", vfs.BindReplace)
+				ns.Bind("/doc", docFs, "/doc", vfs.BindBefore)
+				return ns
+			}(),
+			path: "/doc",
+			want: "a.txt:2,b.txt:1,dir1:0,dir2:0",
+		}, {
+			desc: "have_go_files",
+			ns: func() vfs.NameSpace {
+				a := mapfs.New(map[string]string{
+					"src/x/a.txt":        "",
+					"src/x/suba/sub.txt": "",
+				})
+				b := mapfs.New(map[string]string{
+					"src/x/b.go":         "package b",
+					"src/x/subb/sub.txt": "",
+				})
+				c := mapfs.New(map[string]string{
+					"src/x/c.txt":        "",
+					"src/x/subc/sub.txt": "",
+				})
+				ns := vfs.NameSpace{}
+				ns.Bind("/", a, "/", vfs.BindReplace)
+				ns.Bind("/", b, "/", vfs.BindAfter)
+				ns.Bind("/", c, "/", vfs.BindAfter)
+				return ns
+			}(),
+			path: "/src/x",
+			want: "b.go:9,suba:0,subb:0,subc:0",
+		}, {
+			desc: "empty_mount",
+			ns: func() vfs.NameSpace {
+				ns := vfs.NameSpace{}
+				ns.Bind("/empty", mapfs.New(nil), "/empty", vfs.BindReplace)
+				return ns
+			}(),
+			path: "/",
+			want: "empty:0",
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			fis, err := tc.ns.ReadDir(tc.path)
+			if err != nil {
+				t.Fatal(err)
+			}
+			buf := &strings.Builder{}
+			sep := ""
+			for _, fi := range fis {
+				fmt.Fprintf(buf, "%s%s:%d", sep, fi.Name(), fi.Size())
+				sep = ","
+			}
+			if got := buf.String(); got != tc.want {
+				t.Errorf("got %q; want %q", got, tc.want)
+			}
+		})
+	}
+}