code review 6257059: path/filepath: implement documented SkipDir behavior

path/filepath: implement documented SkipDir behavior

Currently walk() doesn't check for err == SkipDir when iterating
a directory list, but such promise is made in the docs for WalkFunc.

Fixes issue 3486.

[jan_mercl, 2012-05-26 11:25:20]

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

I'd like you to review this change to
https://code.google.com/p/go

[r, 2012-05-26 17:24:02]

the issue documents a test case. it would be nice to add that validation to the
test for this function.

[jan_mercl, 2012-05-27 19:44:02]

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

Please take another look.

[jan_mercl, 2012-05-27 19:44:21]

2012/5/26  <r@golang.org>:
> the issue documents a test case. it would be nice to add that validation
> to the test for this function.

Done. I'm guess it's much more lines than you are going to like.

Updated CL follows.

-j

[jan_mercl, 2012-05-27 19:59:52]

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

Please take another look.

[jan_mercl, 2012-05-27 20:00:22]

2012/5/27 Honza <befelemepeseveze@gmail.com>:
> Done. I'm guess it's much more lines than you are going to like.

s/I'm/I/

+ found 2 times required s/Error/Errorf/ in the CL, so another updated
CL follows.

-j

[r, 2012-05-29 16:49:22]

http://codereview.appspot.com/6257059/diff/1003/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/6257059/diff/1003/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:879: var tempDir string
i can't believe it's this hard to test.

[jan_mercl, 2012-05-29 17:09:15]

http://codereview.appspot.com/6257059/diff/1003/src/pkg/path/filepath/path_te...
File src/pkg/path/filepath/path_test.go (right):

http://codereview.appspot.com/6257059/diff/1003/src/pkg/path/filepath/path_te...
src/pkg/path/filepath/path_test.go:879: var tempDir string
On 2012/05/29 16:49:22, r wrote:
> i can't believe it's this hard to test.

Apologies. I don't like the code either, but this first attempt somehow inflated
a lot to become, I think, complete. Will try some other, probably completely
different and/or less thorough, approach. Any hints are really more than
welcome, TIA. (I'll dive into it not before tomorrow.)

-j

[rsc, 2012-05-29 17:11:25]

It seems like you can Walk($GOROOT), make the function return SkipDir
for "lib", and make sure that "src" is still considered.

Russ

[jan_mercl, 2012-05-29 17:19:31]

2012/5/29 Russ Cox <rsc@golang.org>:
> It seems like you can Walk($GOROOT), make the function return SkipDir
> for "lib", and make sure that "src" is still considered.

Very good idea, thank you. I failed to realize that the issue is not
about the real permission denied thing but only about respecting the
returned SkipDir value in walk() as documented. Your approach will be
*much* simpler for sure.

-j

[jan_mercl, 2012-05-30 11:02:09]

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

Please take another look.

[jan_mercl, 2012-05-30 11:06:17]

2012/5/30  <befelemepeseveze@gmail.com>:
> Hello rsc@golang.org, r@golang.org (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
> http://codereview.appspot.com/6257059/

Please note also path.go was changed in this update wrt the initial
CL. Added another check for fileInfo.IsDir() because that's what the
WalkFunc docs say.

-j

[r, 2012-05-30 17:44:26]

LGTM
leaving for rsc.

http://codereview.appspot.com/6257059/diff/13001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/6257059/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:324: if err != nil && (err != SkipDir ||
!fileInfo.IsDir()) {
this is hard to read. break it up and rearrange
if err != nil {
  if !fileInfo.IsDir() || err != SkipDir {
    return err
  }
}

[jan_mercl, 2012-05-31 12:04:33]

http://codereview.appspot.com/6257059/diff/13001/src/pkg/path/filepath/path.go
File src/pkg/path/filepath/path.go (right):

http://codereview.appspot.com/6257059/diff/13001/src/pkg/path/filepath/path.g...
src/pkg/path/filepath/path.go:324: if err != nil && (err != SkipDir ||
!fileInfo.IsDir()) {
On 2012/05/30 17:44:27, r wrote:
> this is hard to read. break it up and rearrange
> if err != nil {
>   if !fileInfo.IsDir() || err != SkipDir {
>     return err
>   }
> }

Done.

[jan_mercl, 2012-05-31 12:05:11]

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

Please take another look.

[rsc, 2012-06-02 16:59:52]

LGTM

[rsc, 2012-06-02 17:00:13]

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

path/filepath: implement documented SkipDir behavior

Currently walk() doesn't check for err == SkipDir when iterating
a directory list, but such promise is made in the docs for WalkFunc.

Fixes issue 3486.

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

Committer: Russ Cox <rsc@golang.org>