code review 5711058: godoc: support $GOPATH, simplify file system code

godoc: support $GOPATH, simplify file system code

The motivation for this CL is to support $GOPATH well.
Since we already have a FileSystem interface, implement a
Plan 9-style name space.  Bind each of the $GOPATH src
directories onto the $GOROOT src/pkg directory: now
everything is laid out exactly like a normal $GOROOT and
needs very little special case code.

The filter files are no longer used (by us), so I think they
can just be deleted.  Similarly, the Mapping code and the
FileSystem interface were two different ways to accomplish
the same end, so delete the Mapping code.

Within the implementation, since FileSystem is defined to be
slash-separated, use package path consistently, leaving
path/filepath only for manipulating operating system paths.

I kept the -path flag, but I think it can be deleted too.

Fixes issue 2234.
Fixes issue 3046.

[rsc, 2012-03-01 22:08:36]

Hello golang-dev@googlegroups.com,

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

[gri, 2012-03-01 23:28:07]

Some preliminary comments. Still digesting this.

Please add better documentation to the new filesystem ops for those poor non
Plan-9 aficionados like my.

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

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:60: return filepath.Join(string(root),
pathpkg.Clean("/"+path))
why is the "/" needed? is it for "symmetry" with the code below? isn't Join
adding a separator? should have a comment

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:97: type nameSpace map[string][]mountedFS
this could use a more elaborate comment - for instance it's not clear why this
is not just map[string]mountedFS

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:141: const (
// mode values for Bind

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:193: // fmt.Printf("resolve %s: %v\n", path, m)
maybe:

if debug {
   ...
}

(could be useful down the line)

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/filesystem.go#n...
src/cmd/godoc/filesystem.go:208: // fmt.Printf("tx %s: %v\n", path,
m.translate(path))
same here

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/godoc.go
File src/cmd/godoc/godoc.go (right):

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:125: // isParentOf returns true if p is a parent of (or
the same as) q
is this still used? if in another file, it should be moved

http://codereview.appspot.com/5711058/diff/2001/src/cmd/godoc/godoc.go#newcod...
src/cmd/godoc/godoc.go:132: func initDirTrees() {
still needed? if so, comment please

[rsc, 2012-03-02 03:29:51]

PTAL

I made the suggested changes and added a few large comments
and worked examples explaining the name space.

[rsc1, 2012-03-02 03:38:09]

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

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:349: return ns.stat(path, FileSystem.Stat)
By the way, first time I've gotten to use an interface method function in real
code.

[r, 2012-03-02 04:16:09]

LVGTM

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

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:148: //			{old: "/src/pkg", fs: OS(`d:\Work2`), new:
"/src"},
all these examples would be easier to read if the first two elements were
swapped. that would put the two names together. i suggest making that change
throughout, even in the struct.
{fs: OS(`c:\Go`), old: "/src/pkg", new: "/src/pkg"},

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:241: bindReplace = iota
worth in-line comments defining them, or even just put most of the Bind comment
here.

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:261: mtpt = append(mtpt, ns.resolve(old)...)
wow

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:276: panic("invalid Bind")
panic(fmt.Sprintf(...))

http://codereview.appspot.com/5711058/diff/11001/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:289: const debugNS = false
move this to the top of the file

[rsc, 2012-03-02 04:20:15]

On Thu, Mar 1, 2012 at 23:16,  <r@golang.org> wrote:
> all these examples would be easier to read if the first two elements
> were swapped. that would put the two names together. i suggest making
> that change throughout, even in the struct.
> {fs: OS(`c:\Go`), old: "/src/pkg", new: "/src/pkg"},

I started to do this but I think it confuses things:
old is being evaluated in the name space,
but new is being evaluated in fs.  So the current
order:

{old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"}

should be read as (mentally merging fs+new):

"/src/pkg" -> d:\Work1\src

Russ

[r2, 2012-03-02 04:21:16]

On Mar 2, 2012, at 3:20 PM, Russ Cox wrote:

> On Thu, Mar 1, 2012 at 23:16,  <r@golang.org> wrote:
>> all these examples would be easier to read if the first two elements
>> were swapped. that would put the two names together. i suggest making
>> that change throughout, even in the struct.
>> {fs: OS(`c:\Go`), old: "/src/pkg", new: "/src/pkg"},
> 
> I started to do this but I think it confuses things:
> old is being evaluated in the name space,
> but new is being evaluated in fs.  So the current
> order:
> 
> {old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"}
> 
> should be read as (mentally merging fs+new):
> 
> "/src/pkg" -> d:\Work1\src

ok

[rsc, 2012-03-02 04:25:22]

I expanded the comment to show the fs+new reading:

// Given this name space, a ReadDir of /src/pkg/code will check each prefix
// of the path for a mount point (first /src/pkg/code, then /src/pkg, then /src,
// then /), stopping when it finds one.  For the above example, /src/pkg/code
// will find the mount point at /src/pkg:
//
//	{old: "/src/pkg", fs: OS(`c:\Go`), new: "/src/pkg"},
//	{old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"},
//	{old: "/src/pkg", fs: OS(`d:\Work2`), new: "/src"},
//
// ReadDir will when execute these three calls and merge the results:
//
//	OS(`c:\Go`).ReadDir("/src/pkg/code")
//	OS(`d:\Work1').ReadDir("/src/code")
//	OS(`d:\Work2').ReadDir("/src/code")
//
// Note that the "/src/pkg" in "/src/pkg/code" has been replaced by
// just "/src" in the final two calls.
//
// OS is itself an implementation of a file system: it implements
// OS(`c:\Go`).ReadDir("/src/pkg/code") as ioutil.ReadDir(`c:\Go\src\pkg\code`).
//
// Because the new path is evaluated by fs (here OS(root)), another way
// to read the mount table for is to mentally combine fs+new, so that
this table:
//
//	{old: "/src/pkg", fs: OS(`c:\Go`), new: "/src/pkg"},
//	{old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"},
//	{old: "/src/pkg", fs: OS(`d:\Work2`), new: "/src"},
//
// reads as:
//
//	"/src/pkg" -> c:\Go\src\pkg
//	"/src/pkg" -> d:\Work1\src
//	"/src/pkg" -> d:\Work2\src

[r2, 2012-03-02 04:28:53]

On Mar 2, 2012, at 3:25 PM, Russ Cox wrote:

> I expanded the comment to show the fs+new reading:
> 
> // Given this name space, a ReadDir of /src/pkg/code will check each prefix
> // of the path for a mount point (first /src/pkg/code, then /src/pkg, then
/src,
> // then /), stopping when it finds one.  For the above example, /src/pkg/code
> // will find the mount point at /src/pkg:
> //
> //	{old: "/src/pkg", fs: OS(`c:\Go`), new: "/src/pkg"},
> //	{old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"},
> //	{old: "/src/pkg", fs: OS(`d:\Work2`), new: "/src"},
> //
> // ReadDir will when execute these three calls and merge the results:
> //
> //	OS(`c:\Go`).ReadDir("/src/pkg/code")
> //	OS(`d:\Work1').ReadDir("/src/code")
> //	OS(`d:\Work2').ReadDir("/src/code")
> //
> // Note that the "/src/pkg" in "/src/pkg/code" has been replaced by
> // just "/src" in the final two calls.
> //
> // OS is itself an implementation of a file system: it implements
> // OS(`c:\Go`).ReadDir("/src/pkg/code") as
ioutil.ReadDir(`c:\Go\src\pkg\code`).
> //
> // Because the new path is evaluated by fs (here OS(root)), another way
> // to read the mount table for is to mentally combine fs+new, so that
> this table:
> //
> //	{old: "/src/pkg", fs: OS(`c:\Go`), new: "/src/pkg"},
> //	{old: "/src/pkg", fs: OS(`d:\Work1`), new: "/src"},
> //	{old: "/src/pkg", fs: OS(`d:\Work2`), new: "/src"},
> //
> // reads as:
> //
> //	"/src/pkg" -> c:\Go\src\pkg
> //	"/src/pkg" -> d:\Work1\src
> //	"/src/pkg" -> d:\Work2\src

lgtm

[gri, 2012-03-02 18:59:33]

New comments are great, but I still have some questions.

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

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:29: // system, which helps keep things simple.
s/keep/keeping/ ?

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:43: // even on Windows.
Can you say which functions use OS-specific paths? maybe as comment with the
function?

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:95: path = pathpkg.Clean("/" + path)
shouldn't this be filepath.Clean? Or is path not OS-specific? and if not, why is
is filepath below and not pathpkg?

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:159: //	ns.Bind("/", OS(`c:\Go`), "/", bindReplace)
is this causing two entries? or is a ns.Bind missing here?

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

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/godoc.go#oldco...
src/cmd/godoc/godoc.go:725: PkgRoots  []string
this means we lost the ability to show -path added filesystems directly

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go
File src/cmd/godoc/main.go (right):

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go#newcod...
src/cmd/godoc/main.go:377: fs.Bind("/target", OS(path), "/", bindReplace)
I assume OS is taking an OS-specific path

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go#newcod...
src/cmd/godoc/main.go:377: fs.Bind("/target", OS(path), "/", bindReplace)
const target = "/target" and a comment?

[rsc1, 2012-03-02 19:24:56]

PTAL

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

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:29: // system, which helps keep things simple.
On 2012/03/02 18:59:33, gri wrote:
> s/keep/keeping/ ?

I think keep is correct.

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:43: // even on Windows.
On 2012/03/02 18:59:33, gri wrote:
> Can you say which functions use OS-specific paths? maybe as comment with the
> function?

Rewrote:

// The use of this virtual file system means that almost all code processing
// paths can assume they are slash-separated and should be using
// package path (often imported as pathpkg) to manipulate them,
// even on Windows.  The only code that should use filepath is code
// that is bypassing the virtual file system and making direct calls
// to package os.

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:95: path = pathpkg.Clean("/" + path)
On 2012/03/02 18:59:33, gri wrote:
> shouldn't this be filepath.Clean? Or is path not OS-specific? and if not, why
is
> is filepath below and not pathpkg?

Added to type FileSystem comment:


// The FileSystem interface specifies the methods godoc is using
// to access the file system for which it serves documentation.
// The path argument to each method is always a slash-separated path.

Added comment here:

// resolve converts a slash-separated file system path
// (given to a FileSystem method) to the underlying
// operating system path that should be used with package os.

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/filesystem.go#...
src/cmd/godoc/filesystem.go:159: //	ns.Bind("/", OS(`c:\Go`), "/", bindReplace)
On 2012/03/02 18:59:33, gri wrote:
> is this causing two entries? or is a ns.Bind missing here?

It causes one entry at first, but then the first ns.Bind of /src/pkg
has to create a new mount point at /src/pkg, and because it says
bindAfter it makes a copy of the existing mapping for /src/pkg
(which is the mapping from /).

Rewrote:

// This is created by executing:
//
//	ns := nameSpace{}
//	ns.Bind("/", OS(`c:\Go`), "/", bindReplace)
//	ns.Bind("/src/pkg", OS(`d:\Work1`), "/src", bindAfter)
//	ns.Bind("/src/pkg", OS(`d:\Work2`), "/src", bindAfter)
//
// (There are three entries in the /src/pkg mount point above but
// only two ns.Bind calls because the first ns.Bind on /src/pkg
// uses bindAfter, which copies the existing effective mapping from /
// to create a new /src/pkg mount point before adding its own mapping.)

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

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/godoc.go#oldco...
src/cmd/godoc/godoc.go:725: PkgRoots  []string
On 2012/03/02 18:59:33, gri wrote:
> this means we lost the ability to show -path added filesystems directly

They appear as directories under /src/pkg.
I couldn't find anything in templates mentioning PkgRoots
but maybe I looked in the wrong place.

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go
File src/cmd/godoc/main.go (right):

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go#newcod...
src/cmd/godoc/main.go:377: fs.Bind("/target", OS(path), "/", bindReplace)
On 2012/03/02 18:59:33, gri wrote:
> I assume OS is taking an OS-specific path

Yes.

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/main.go#newcod...
src/cmd/godoc/main.go:377: fs.Bind("/target", OS(path), "/", bindReplace)
On 2012/03/02 18:59:33, gri wrote:
> const target = "/target" and a comment?

Done.

[gri, 2012-03-02 19:56:03]

LGTM

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

http://codereview.appspot.com/5711058/diff/12018/src/cmd/godoc/godoc.go#oldco...
src/cmd/godoc/godoc.go:725: PkgRoots  []string
On 2012/03/02 19:24:56, rsc1 wrote:
> On 2012/03/02 18:59:33, gri wrote:
> > this means we lost the ability to show -path added filesystems directly
> 
> They appear as directories under /src/pkg.
> I couldn't find anything in templates mentioning PkgRoots
> but maybe I looked in the wrong place.

They used to be there but got lost when adg revamped the godoc UI.

[r, 2012-03-02 21:10:52]

LGTM just in case

[rsc1, 2012-03-02 22:28:25]

Thanks.  My plan is to wait to submit this until
after Andrew cuts the weekly.

[rsc, 2012-03-05 15:02:51]

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

godoc: support $GOPATH, simplify file system code

The motivation for this CL is to support $GOPATH well.
Since we already have a FileSystem interface, implement a
Plan 9-style name space.  Bind each of the $GOPATH src
directories onto the $GOROOT src/pkg directory: now
everything is laid out exactly like a normal $GOROOT and
needs very little special case code.

The filter files are no longer used (by us), so I think they
can just be deleted.  Similarly, the Mapping code and the
FileSystem interface were two different ways to accomplish
the same end, so delete the Mapping code.

Within the implementation, since FileSystem is defined to be
slash-separated, use package path consistently, leaving
path/filepath only for manipulating operating system paths.

I kept the -path flag, but I think it can be deleted too.

Fixes issue 2234.
Fixes issue 3046.

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