godoc/vfs/zipfs: add tests; fix handling of "/"

- add tests for Open, ReadDir, and Stat funcs
- add tests for Seek of Open()
- simplify internal handling of absolute vs. "zip" paths
- fix handling of "/"

The fix special cases this scenario, leaving the codepath for all
other file paths the same. Specifically,

- Exported funcs call stat(), so stat("/") is handled by simply
returning 0 to indicate all entries are (effectively) prefixed
by "/" and zipFI{"", nil} because "/" has no name and nil
indicates it is a directory.

- ReadDir("/") is further handled by seeding the existing lookup
logic with "" instead of what would have been "/". This is
necessary because, per the zipfs spec, the zip file entries MUST
NOT start with "/", so using "/" would incorrectly match nothing.
This works because seeding lookup with "" (correctly) matches
all files and then the following, existing logic (correctly) pares
things down to just the files in the root directory; not in any
subdirectories.

Verified that godoc -zip still works.

Fixes #12743

Change-Id: Icb5f01b8a29cefa4e2820135f318894042970301
Reviewed-on: https://go-review.googlesource.com/16925
Reviewed-by: Andrew Gerrand <[email protected]>

Author: jdhenke

godoc/vfs/zipfs/zipfs.go

@@ -86,21 +86,35 @@ func (fs *zipFS) Close() error {
 	return fs.ReadCloser.Close()
 }
 
-func zipPath(name string) string {
+func zipPath(name string) (string, error) {
 	name = path.Clean(name)
 	if !path.IsAbs(name) {
-		panic(fmt.Sprintf("stat: not an absolute path: %s", name))
+		return "", fmt.Errorf("stat: not an absolute path: %s", name)
 	}
-	return name[1:] // strip leading '/'
+	return name[1:], nil // strip leading '/'
+}
+
+func isRoot(abspath string) bool {
+	return path.Clean(abspath) == "/"
 }
 
 func (fs *zipFS) stat(abspath string) (int, zipFI, error) {
-	i, exact := fs.list.lookup(abspath)
+	if isRoot(abspath) {
+		return 0, zipFI{
+			name: "",
+			file: nil,
+		}, nil
+	}
+	zippath, err := zipPath(abspath)
+	if err != nil {
+		return 0, zipFI{}, err
+	}
+	i, exact := fs.list.lookup(zippath)
 	if i < 0 {
-		// abspath has leading '/' stripped - print it explicitly
-		return -1, zipFI{}, fmt.Errorf("file not found: /%s", abspath)
+		// zippath has leading '/' stripped - print it explicitly
+		return -1, zipFI{}, fmt.Errorf("file not found: /%s", zippath)
 	}
-	_, name := path.Split(abspath)
+	_, name := path.Split(zippath)
 	var file *zip.File
 	if exact {
 		file = fs.list[i] // exact match found - must be a file
@@ -109,7 +123,7 @@ func (fs *zipFS) stat(abspath string) (int, zipFI, error) {
 }
 
 func (fs *zipFS) Open(abspath string) (vfs.ReadSeekCloser, error) {
-	_, fi, err := fs.stat(zipPath(abspath))
+	_, fi, err := fs.stat(abspath)
 	if err != nil {
 		return nil, err
 	}
@@ -142,18 +156,17 @@ func (f *zipSeek) Seek(offset int64, whence int) (int64, error) {
 }
 
 func (fs *zipFS) Lstat(abspath string) (os.FileInfo, error) {
-	_, fi, err := fs.stat(zipPath(abspath))
+	_, fi, err := fs.stat(abspath)
 	return fi, err
 }
 
 func (fs *zipFS) Stat(abspath string) (os.FileInfo, error) {
-	_, fi, err := fs.stat(zipPath(abspath))
+	_, fi, err := fs.stat(abspath)
 	return fi, err
 }
 
 func (fs *zipFS) ReadDir(abspath string) ([]os.FileInfo, error) {
-	path := zipPath(abspath)
-	i, fi, err := fs.stat(path)
+	i, fi, err := fs.stat(abspath)
 	if err != nil {
 		return nil, err
 	}
@@ -162,7 +175,21 @@ func (fs *zipFS) ReadDir(abspath string) ([]os.FileInfo, error) {
 	}
 
 	var list []os.FileInfo
-	dirname := path + "/"
+
+	// make dirname the prefix that file names must start with to be considered
+	// in this directory. we must special case the root directory because, per
+	// the spec of this package, zip file entries MUST NOT start with /, so we
+	// should not append /, as we would in every other case.
+	var dirname string
+	if isRoot(abspath) {
+		dirname = ""
+	} else {
+		zippath, err := zipPath(abspath)
+		if err != nil {
+			return nil, err
+		}
+		dirname = zippath + "/"
+	}
 	prevname := ""
 	for _, e := range fs.list[i:] {
 		if !strings.HasPrefix(e.Name, dirname) {

godoc/vfs/zipfs/zipfs_test.go

@@ -0,0 +1,179 @@
+// Copyright 2015 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 zipfs
+package zipfs
+
+import (
+	"archive/zip"
+	"bytes"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+	"reflect"
+	"testing"
+
+	"golang.org/x/tools/godoc/vfs"
+)
+
+var (
+
+	// files to use to build zip used by zipfs in testing; maps path : contents
+	files = map[string]string{"foo": "foo", "bar/baz": "baz", "a/b/c": "c"}
+
+	// expected info for each entry in a file system described by files
+	tests = []struct {
+		Path      string
+		IsDir     bool
+		IsRegular bool
+		Name      string
+		Contents  string
+		Files     map[string]bool
+	}{
+		{"/", true, false, "", "", map[string]bool{"foo": true, "bar": true, "a": true}},
+		{"//", true, false, "", "", map[string]bool{"foo": true, "bar": true, "a": true}},
+		{"/foo", false, true, "foo", "foo", nil},
+		{"/foo/", false, true, "foo", "foo", nil},
+		{"/foo//", false, true, "foo", "foo", nil},
+		{"/bar", true, false, "bar", "", map[string]bool{"baz": true}},
+		{"/bar/", true, false, "bar", "", map[string]bool{"baz": true}},
+		{"/bar/baz", false, true, "baz", "baz", nil},
+		{"//bar//baz", false, true, "baz", "baz", nil},
+		{"/a/b", true, false, "b", "", map[string]bool{"c": true}},
+	}
+
+	// to be initialized in setup()
+	fs        vfs.FileSystem
+	statFuncs []statFunc
+)
+
+type statFunc struct {
+	Name string
+	Func func(string) (os.FileInfo, error)
+}
+
+func TestMain(t *testing.M) {
+	if err := setup(); err != nil {
+		fmt.Fprintf(os.Stderr, "Error setting up zipfs testing state: %v.\n", err)
+		os.Exit(1)
+	}
+	os.Exit(t.Run())
+}
+
+// setups state each of the tests uses
+func setup() error {
+	// create zipfs
+	b := new(bytes.Buffer)
+	zw := zip.NewWriter(b)
+	for file, contents := range files {
+		w, err := zw.Create(file)
+		if err != nil {
+			return err
+		}
+		_, err = io.WriteString(w, contents)
+		if err != nil {
+			return err
+		}
+	}
+	zw.Close()
+	zr, err := zip.NewReader(bytes.NewReader(b.Bytes()), int64(b.Len()))
+	if err != nil {
+		return err
+	}
+	rc := &zip.ReadCloser{
+		Reader: *zr,
+	}
+	fs = New(rc, "foo")
+
+	// pull out different stat functions
+	statFuncs = []statFunc{
+		{"Stat", fs.Stat},
+		{"Lstat", fs.Lstat},
+	}
+
+	return nil
+}
+
+func TestZipFSReadDir(t *testing.T) {
+	for _, test := range tests {
+		if test.IsDir {
+			infos, err := fs.ReadDir(test.Path)
+			if err != nil {
+				t.Errorf("Failed to read directory %v\n", test.Path)
+				continue
+			}
+			got := make(map[string]bool)
+			for _, info := range infos {
+				got[info.Name()] = true
+			}
+			if want := test.Files; !reflect.DeepEqual(got, want) {
+				t.Errorf("ReadDir %v got %v\nwanted %v\n", test.Path, got, want)
+			}
+		}
+	}
+}
+
+func TestZipFSStatFuncs(t *testing.T) {
+	for _, test := range tests {
+		for _, statFunc := range statFuncs {
+
+			// test can stat
+			info, err := statFunc.Func(test.Path)
+			if err != nil {
+				t.Errorf("Unexpected error using %v for %v: %v\n", statFunc.Name, test.Path, err)
+				continue
+			}
+
+			// test info.Name()
+			if got, want := info.Name(), test.Name; got != want {
+				t.Errorf("Using %v for %v info.Name() got %v wanted %v\n", statFunc.Name, test.Path, got, want)
+			}
+			// test info.IsDir()
+			if got, want := info.IsDir(), test.IsDir; got != want {
+				t.Errorf("Using %v for %v info.IsDir() got %v wanted %v\n", statFunc.Name, test.Path, got, want)
+			}
+			// test info.Mode().IsDir()
+			if got, want := info.Mode().IsDir(), test.IsDir; got != want {
+				t.Errorf("Using %v for %v info.Mode().IsDir() got %v wanted %v\n", statFunc.Name, test.Path, got, want)
+			}
+			// test info.Mode().IsRegular()
+			if got, want := info.Mode().IsRegular(), test.IsRegular; got != want {
+				t.Errorf("Using %v for %v info.Mode().IsRegular() got %v wanted %v\n", statFunc.Name, test.Path, got, want)
+			}
+			// test info.Size()
+			if test.IsRegular {
+				if got, want := info.Size(), int64(len(test.Contents)); got != want {
+					t.Errorf("Using %v for %v inf.Size() got %v wanted %v", statFunc.Name, test.Path, got, want)
+				}
+			}
+		}
+	}
+}
+
+func TestZipFSOpenSeek(t *testing.T) {
+	for _, test := range tests {
+		if test.IsRegular {
+
+			// test Open()
+			f, err := fs.Open(test.Path)
+			if err != nil {
+				t.Error(err)
+				return
+			}
+			defer f.Close()
+
+			// test Seek() multiple times
+			for i := 0; i < 3; i++ {
+				all, err := ioutil.ReadAll(f)
+				if err != nil {
+					t.Error(err)
+					return
+				}
+				if got, want := string(all), test.Contents; got != want {
+					t.Errorf("File contents for %v got %v wanted %v\n", test.Path, got, want)
+				}
+				f.Seek(0, 0)
+			}
+		}
+	}
+}