code review 5783079: cmd/godoc: fix directory read

cmd/godoc: fix directory read

Undo CL 5783076 and apply correct fix.

The /doc hack is wrong.  The code to handle this case was
already there and just needs a simple fix:

 	// We didn't find any directories containing Go files.
 	// If some directory returned successfully, use that.
-	if len(all) == 0 && first != nil {
+	if !haveGo {
 		for _, d := range first {
 			haveName[d.Name()] = true
 			all = append(all, d)

[rsc, 2012-03-12 16:37:14]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2012-03-12 16:44:50]

LGTM

Testing this, I notice what a mess /pkg is once you have third-party code
installed.

I'd really like to see the standard library by itself, or at least broken
up by itself at the top of the page, with third-party GOPATH stuff at the
bottom.


On Mon, Mar 12, 2012 at 9:37 AM, <rsc@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/godoc: fix directory read
>
> Undo CL 5783076 and apply correct fix.
>
> The /doc hack is wrong.  The code to handle this case was
> already there and just needs a simple fix:
>
>        // We didn't find any directories containing Go files.
>        // If some directory returned successfully, use that.
> -       if len(all) == 0 && first != nil {
> +       if !haveGo {
>                for _, d := range first {
>                        haveName[d.Name()] = true
>                        all = append(all, d)
>
> Please review this at
http://codereview.appspot.com/**5783079/<http://codereview.appspot.com/5783079/>
>
> Affected files:
>  M src/cmd/godoc/filesystem.go
>
>
> Index: src/cmd/godoc/filesystem.go
> ==============================**==============================**=======
> --- a/src/cmd/godoc/filesystem.go
> +++ b/src/cmd/godoc/filesystem.go
> @@ -420,17 +420,11 @@
>                        first = dir
>                }
>
> -               useFiles := false
> -
> -               // Always include all files under /doc.
> -               if path == "/doc" || strings.HasPrefix(path, "/doc/") {
> -                       useFiles = true // always include docs
> -               }
> -
>                // 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.
> -               if !useFiles && !haveGo {
> +               useFiles := false
> +               if !haveGo {
>                        for _, d := range dir {
>                                if strings.HasSuffix(d.Name(), ".go") {
>                                        useFiles = true
> @@ -451,7 +445,7 @@
>
>        // We didn't find any directories containing Go files.
>        // If some directory returned successfully, use that.
> -       if len(all) == 0 && first != nil {
> +       if !haveGo {
>                for _, d := range first {
>                        haveName[d.Name()] = true
>                        all = append(all, d)
>
>
>

[rsc, 2012-03-12 17:10:40]

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

cmd/godoc: fix directory read

Undo CL 5783076 and apply correct fix.

The /doc hack is wrong.  The code to handle this case was
already there and just needs a simple fix:

 	// We didn't find any directories containing Go files.
 	// If some directory returned successfully, use that.
-	if len(all) == 0 && first != nil {
+	if !haveGo {
 		for _, d := range first {
 			haveName[d.Name()] = true
 			all = append(all, d)

R=golang-dev, bradfitz
CC=golang-dev
http://codereview.appspot.com/5783079

[rsc, 2012-03-12 17:25:37]

If you really wanted to, you could do something like 5797067.
But I am not sure it's such a good idea.

[bradfitz, 2012-03-12 17:35:04]

On Mon, Mar 12, 2012 at 10:25 AM, Russ Cox <rsc@golang.org> wrote:

> If you really wanted to, you could do something like 5797067.
> But I am not sure it's such a good idea.
>

Sounds like a great idea. Much prettier. What's the downside?

[rsc, 2012-03-12 17:36:38]

On Mon, Mar 12, 2012 at 13:35, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Sounds like a great idea. Much prettier. What's the downside?

I have used systems with 'mostly sorted' lists before, and
the convenience always seems to give way to confusion,
because people think they _are_ sorted and then can't find things.

Russ

[gri, 2012-03-12 17:44:13]

Can we have different modes for looking at it?
- Robert

On Mon, Mar 12, 2012 at 10:36 AM, Russ Cox <rsc@golang.org> wrote:
> On Mon, Mar 12, 2012 at 13:35, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> Sounds like a great idea. Much prettier. What's the downside?
>
> I have used systems with 'mostly sorted' lists before, and
> the convenience always seems to give way to confusion,
> because people think they _are_ sorted and then can't find things.
>
> Russ

[bradfitz, 2012-03-12 17:51:10]

On Mon, Mar 12, 2012 at 10:44 AM, Robert Griesemer <gri@golang.org> wrote:

> Can we have different modes for looking at it?
> - Robert
>

The default is all that matters.

There's already a flat mode (which I prefer), but I can never remember how
to get to it without looking up the source, which I'm always too lazy to do.

You could argue for mode-switching links, but therein lies craziness.

[rsc, 2012-03-12 17:55:04]

Let's leave this for after Go 1.