code review 4572065: godoc: replace direct OS file system accesses in favor

godoc: replace direct OS file system accesses in favor
       of accesses via a FileSystem interface.

Preparation for appengine version which gets its files
via a snapshot or zip file and uses a corresponding
FileSystem implementation.

[gri, 2011-06-15 00:57:14]

Hello rsc@golang.org, r@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2011-06-15 02:46:47]

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go
File src/cmd/godoc/filesystem.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:17: type FileInfo interface {
I don't believe this makes sense.  os.FileInfo is already written to
be portable, meaning operating system-independent, and it is
pure exported data.  I suggest using *os.FileInfo below, and
then a level of wrapping and a bunch of changes all disappear.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:64: func (fs osFS) Open(path string) (io.ReadCloser,
os.Error) {
func (osFS)

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (left):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go#oldcod...
src/cmd/godoc/godoc.go:824: for _, d := range list {
It seems odd to redefine the interface to be different from os.
I would keep this code.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go
File src/cmd/godoc/index.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go#newcod...
src/cmd/godoc/index.go:909: err = os.ErrorString("all query parts must be
identifiers")
There are a bunch of these, and they are unrelated to this change.
Please revert them so that the diffs are clearer.

I pre-LGTM any CL containing just NewError -> ErrorString
in godoc if you want to convert them all.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go
File src/cmd/godoc/utils.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go#newcode47
src/cmd/godoc/utils.go:47: // TODO(gri) must use the underlying file system
here, not os
This could be clearer.  I think of 'os' as the underlying file system.

[gri, 2011-06-15 20:12:03]

PTAL

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go
File src/cmd/godoc/filesystem.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:17: type FileInfo interface {
On 2011/06/15 02:46:47, rsc wrote:
> I don't believe this makes sense.  os.FileInfo is already written to
> be portable, meaning operating system-independent, and it is
> pure exported data.  I suggest using *os.FileInfo below, and
> then a level of wrapping and a bunch of changes all disappear.
> 

I was thinking about doing that, but I believe that makes things more
complicated. os.FileInfo may be portable but there's a lot of information in
there that I don't care about (and may not even exist), and I don't want to
setup (or even just allocate the space for) an os.FileInfo when the "files" are
coming from elsewhere (say via a zip file). This is very light-weight, cheap,
and abstract. For osFS, there's very little code needed, and for another
implementation I am not bound to set up a (partial) os.FileInfo.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:64: func (fs osFS) Open(path string) (io.ReadCloser,
os.Error) {
On 2011/06/15 02:46:47, rsc wrote:
> func (osFS) 

Done.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (left):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/godoc.go#oldcod...
src/cmd/godoc/godoc.go:824: for _, d := range list {
On 2011/06/15 02:46:47, rsc wrote:
> It seems odd to redefine the interface to be different from os.
> I would keep this code.

Again, this requires os.FileInfo, or some settable size. The size returned from
os.Stat is really platform specific. With the new FileInfo, this size is already
0 and so this is not needed. Seems cleaner to me.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go
File src/cmd/godoc/index.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/index.go#newcod...
src/cmd/godoc/index.go:909: err = os.ErrorString("all query parts must be
identifiers")
On 2011/06/15 02:46:47, rsc wrote:
> There are a bunch of these, and they are unrelated to this change.
> Please revert them so that the diffs are clearer.
> 
> I pre-LGTM any CL containing just NewError -> ErrorString
> in godoc if you want to convert them all.
>  

Done.

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go
File src/cmd/godoc/utils.go (right):

http://codereview.appspot.com/4572065/diff/5001/src/cmd/godoc/utils.go#newcode47
src/cmd/godoc/utils.go:47: // TODO(gri) must use the underlying file system
here, not os
On 2011/06/15 02:46:47, rsc wrote:
> This could be clearer.  I think of 'os' as the underlying file system.

Done.

[rsc, 2011-06-15 20:31:26]

LGTM

[gri, 2011-06-15 21:06:44]

*** Submitted as http://code.google.com/p/go/source/detail?r=3a5f42c956e0 ***

godoc: replace direct OS file system accesses in favor
       of accesses via a FileSystem interface.

Preparation for appengine version which gets its files
via a snapshot or zip file and uses a corresponding
FileSystem implementation.

R=rsc, r
CC=golang-dev
http://codereview.appspot.com/4572065