code review 12261043: os: Made Readdir() work as documented

os: Made Readdir() work as documented

Readdir's result should never contain a nil.

Fixes issue 5960.

[peted, 2013-08-01 15:26:48]

Hello golang-dev@googlegroups.com,

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

[rsc, 2013-08-01 17:13:59]

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go#new...
src/pkg/os/file_unix.go:165: // When an error occurs, return the files read
The goal of the original code was to return an array with at least a name.

fi[i] = &fileStat{name: filename}
if err == nil {
    err = lerr
}

[peted, 2013-08-01 17:24:42]

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go#new...
src/pkg/os/file_unix.go:165: // When an error occurs, return the files read
On 2013/08/01 17:13:59, rsc wrote:
> The goal of the original code was to return an array with at least a name.
> 
> fi[i] = &fileStat{name: filename}
> if err == nil {
>     err = lerr
> }

In that case, the documentation should be updated. Right now it only mentions
"If it encounters an error before the end of the directory, Readdir returns the
FileInfo read until that point and a non-nil error."

[peted, 2013-08-01 19:09:03]

On Windows and Plan 9, when an error occurs only the files read up to that point
are returned, along with the error.

Looks like only on Unix are FileInfos returned with just the name.

If you ask me, the correct course of action is to fix Unix to be in line with
the other two implementations, and the documentation.

Also, it's strange that only Plan 9 puts its readdir implementation in
dir_plan9.go, while the other two are in file_unix.go and file_windows.go.

[bradfitz, 2013-08-01 21:40:40]

On Thu, Aug 1, 2013 at 8:26 AM, <pieter@binky.org.uk> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> os: Made Readdir() work as documented
>
> Readdir's result should never contain a nil and, on error, should return
> all files read so far.
> If anyone has a clever idea for a test that makes Lstat() fail on a
> filename coming from Readdirnames, I'm all ears.


Others might object to the extra noise, but I often do stuff like:

// testLstat is used instead of Lstat in tests if non-nil.
var testLstat func(string) (FileInfo, error)

func .... () {
     lstat := Lstat
     if testLstat != nil {
            lstat = testLstat
     }
     ....
    fip, lerr := lstat(dirname + filename)
    ....
}

Then in your foo_test.go, temporarily re-assign a lstat that injects
failures when you'd like.

[peted, 2013-08-01 22:29:27]

On 2013/08/01 21:40:40, bradfitz wrote:
> On Thu, Aug 1, 2013 at 8:26 AM, <mailto:pieter@binky.org.uk> wrote:
> 
> > Reviewers: golang-dev1,
> >
> > Message:
> > Hello mailto:golang-dev@googlegroups.com,
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go
> >
> >
> > Description:
> > os: Made Readdir() work as documented
> >
> > Readdir's result should never contain a nil and, on error, should return
> > all files read so far.
> > If anyone has a clever idea for a test that makes Lstat() fail on a
> > filename coming from Readdirnames, I'm all ears.
> 
> 
> Others might object to the extra noise, but I often do stuff like:
> 
> // testLstat is used instead of Lstat in tests if non-nil.
> var testLstat func(string) (FileInfo, error)
> 
> func .... () {
>      lstat := Lstat
>      if testLstat != nil {
>             lstat = testLstat
>      }
>      ....
>     fip, lerr := lstat(dirname + filename)
>     ....
> }
> 
> Then in your foo_test.go, temporarily re-assign a lstat that injects
> failures when you'd like.

Yeah, mocking Lstat was the way to go. Can't really put that in the tree though.

[bradfitz, 2013-08-01 22:38:43]

On Thu, Aug 1, 2013 at 3:29 PM, <pieter@binky.org.uk> wrote:

> On 2013/08/01 21:40:40, bradfitz wrote:
>
>  On Thu, Aug 1, 2013 at 8:26 AM, <mailto:pieter@binky.org.uk> wrote:
>>
>
>  > Reviewers: golang-dev1,
>> >
>> > Message:
>> > Hello mailto:golang-dev@**googlegroups.com<golang-dev@googlegroups.com>
>> ,
>>
>> >
>> > I'd like you to review this change to
>> > https://code.google.com/p/go
>> >
>> >
>> > Description:
>> > os: Made Readdir() work as documented
>> >
>> > Readdir's result should never contain a nil and, on error, should
>>
> return
>
>> > all files read so far.
>> > If anyone has a clever idea for a test that makes Lstat() fail on a
>> > filename coming from Readdirnames, I'm all ears.
>>
>
>
>  Others might object to the extra noise, but I often do stuff like:
>>
>
>  // testLstat is used instead of Lstat in tests if non-nil.
>> var testLstat func(string) (FileInfo, error)
>>
>
>  func .... () {
>>       lstat := Lstat
>>       if testLstat != nil {
>>              lstat = testLstat
>>       }
>>       ....
>>      fip, lerr := lstat(dirname + filename)
>>      ....
>> }
>>
>
>  Then in your foo_test.go, temporarily re-assign a lstat that injects
>> failures when you'd like.
>>
>
> Yeah, mocking Lstat was the way to go. Can't really put that in the tree
> though.


Why not?

We do elsewhere. Not too often within the Go standard library, but
e.g. net/dial_gen.go has var testingIssue5349 bool.  Probably a few other
places too.

It's more common within Google-internal Go libraries/tests.

[peted, 2013-08-01 22:43:16]

On 2013/08/01 22:38:43, bradfitz wrote:

> Why not?
> 
> We do elsewhere. Not too often within the Go standard library, but
> e.g. net/dial_gen.go has var testingIssue5349 bool.  Probably a few other
> places too.
> 
> It's more common within Google-internal Go libraries/tests.

Well, in that case I'll sort out some tests tomorrow :) Thanks Brad.

Any idea on the issue at hand, about what to do about Unix' readdir()?

[rsc, 2013-08-02 17:08:29]

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/12261043/diff/5001/src/pkg/os/file_unix.go#new...
src/pkg/os/file_unix.go:165: // When an error occurs, return the files read
On 2013/08/01 17:24:42, peted wrote:
> On 2013/08/01 17:13:59, rsc wrote:
> > The goal of the original code was to return an array with at least a name.
> > 
> > fi[i] = &fileStat{name: filename}
> > if err == nil {
> >     err = lerr
> > }
> 
> In that case, the documentation should be updated. Right now it only mentions
> "If it encounters an error before the end of the directory, Readdir returns
the
> FileInfo read until that point and a non-nil error."

The documentation is correct as it is. The reading of the directory has not
failed, just the Lstat of one of the things in the directory.

[peted, 2013-08-02 19:00:31]

On 2013/08/02 17:08:29, rsc wrote:
> The documentation is correct as it is. The reading of the directory has not
> failed, just the Lstat of one of the things in the directory.

Then so it shall remain. Tonight I'll see about adding some tests.

[rsc, 2013-08-02 19:38:24]

Thanks. The check made more sense when we used to call Stat on each
directory entry; Stat can fail if there is a bogus symlink. Lstat can only
fail if the file is removed between the Readdirnames and the Lstat.

Russ

[peted, 2013-08-07 22:27:55]

On 2013/08/02 19:38:24, rsc wrote:
> Thanks. The check made more sense when we used to call Stat on each
> directory entry; Stat can fail if there is a bogus symlink. Lstat can only
> fail if the file is removed between the Readdirnames and the Lstat.
> 
> Russ

Sorry for the delay, I kept forgetting it/putting it off. I've added a test.

[bradfitz, 2013-08-07 22:33:35]

https://codereview.appspot.com/12261043/diff/22001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/12261043/diff/22001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:152: var mockLstat = Lstat
I don't like this name.

This isn't a mock.  It's a variable.

The value you assign to this variable in your test code will be the fake, mock,
or stub.  But it isn't yet.

I would just name it "lstat", with a comment:

// lstat is overridden in tests.
var lstat = Lstat

[peted, 2013-08-07 22:38:58]

PTAL

https://codereview.appspot.com/12261043/diff/22001/src/pkg/os/file_unix.go
File src/pkg/os/file_unix.go (right):

https://codereview.appspot.com/12261043/diff/22001/src/pkg/os/file_unix.go#ne...
src/pkg/os/file_unix.go:152: var mockLstat = Lstat
On 2013/08/07 22:33:35, bradfitz wrote:
> I don't like this name.
> 
> This isn't a mock.  It's a variable.
> 
> The value you assign to this variable in your test code will be the fake,
mock,
> or stub.  But it isn't yet.
> 
> I would just name it "lstat", with a comment:
> 
> // lstat is overridden in tests.
> var lstat = Lstat

Done. I wasn't sure what to name is in export_test, I hope you like LstatP.

[bradfitz, 2013-08-07 23:39:20]

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go
File src/pkg/os/os_unix_test.go (right):

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:31: // Chown is not supported under Plan 9.
please send this in a separate CL.  It has nothing to do with this readdir or
issue 5960.

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:96: t.Fatalf("Expected Readdir to return ErrInvalid,
got %s", err)
convention nit: use %v for things of type error.

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:103: t.Fatalf("Expected Readdir for %s should not
contain Sys", failfile)
these two can probably be Errorf.  the test can proceed without crashing.

[peted, 2013-08-08 08:06:24]

I'll get on this when I get home from work.

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go
File src/pkg/os/os_unix_test.go (right):

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:31: // Chown is not supported under Plan 9.
On 2013/08/07 23:39:20, bradfitz wrote:
> please send this in a separate CL.  It has nothing to do with this readdir or
> issue 5960.

I'll do that when this one's committed.

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:96: t.Fatalf("Expected Readdir to return ErrInvalid,
got %s", err)
On 2013/08/07 23:39:20, bradfitz wrote:
> convention nit: use %v for things of type error.

Heh. I actually thought about this, and peeked at a different fmt to see how you
guys did it. I found line 53 of this very file, which uses %s.

https://codereview.appspot.com/12261043/diff/26001/src/pkg/os/os_unix_test.go...
src/pkg/os/os_unix_test.go:103: t.Fatalf("Expected Readdir for %s should not
contain Sys", failfile)
On 2013/08/07 23:39:20, bradfitz wrote:
> these two can probably be Errorf.  the test can proceed without crashing.

Will do.

[peted, 2013-08-08 14:05:55]

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

Please take another look.

[bradfitz, 2013-08-08 17:42:16]

LGTM



On Thu, Aug 8, 2013 at 7:05 AM, <pieter@binky.org.uk> wrote:

> Hello golang-dev@googlegroups.com, rsc@golang.org, bradfitz@golang.org
> (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/12261043/<https://codereview.appspot.com/122...
>

[bradfitz, 2013-08-08 17:44:05]

*** Submitted as https://code.google.com/p/go/source/detail?r=100a10512ea4 ***

os: make Readdir work as documented

Readdir's result should never contain a nil.

Fixes issue 5960.

R=golang-dev, rsc, bradfitz
CC=golang-dev
https://codereview.appspot.com/12261043

Committer: Brad Fitzpatrick <bradfitz@golang.org>