code review 4381048: os: fix Readdir in Plan 9

os: fix Readdir in Plan 9

'TestReaddir.*' tests now passes.

[fhs, 2011-04-10 07:42:51]

Hello golang-dev@googlegroups.com,

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

[lucio, 2011-04-12 04:39:52]

On 2011/04/10 07:42:51, fhs wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I'm probably not much of a judge, but in your change to os_test.go, you're
checking len(small) against len(all) where the read explicitly asked for
len(all)+100 characters.  That seems self-defeating.

Lucio.

[fhs, 2011-04-12 17:41:09]

> I'm probably not much of a judge, but in your change to os_test.go, you're
> checking len(small) against len(all) where the read explicitly asked for
> len(all)+100 characters.  That seems self-defeating.
> 
> Lucio.

Thanks, I changed it to make sure len(small) >= len(all), so that that small[i]
doesn't crash with "index out of range" in the loop. I'm assuming the test
allows len(small) to be in the range [len(all)+1, 100+len(all)] because if few
files were added, the order of the first len(all) files would change. I'm not
sure if this is a correct assumption.

[lucio, 2011-04-12 18:01:30]

Glad to have been of help.  I'll bow to more knowledgeable persons to deal
with your follow-up question, I'm not clued up on that.

Lucio.

On Tue, Apr 12, 2011 at 7:41 PM, <fshahriar@gmail.com> wrote:

> I'm probably not much of a judge, but in your change to os_test.go,
>>
> you're
>
>> checking len(small) against len(all) where the read explicitly asked
>>
> for
>
>> len(all)+100 characters.  That seems self-defeating.
>>
>
>  Lucio.
>>
>
> Thanks, I changed it to make sure len(small) >= len(all), so that that
> small[i] doesn't crash with "index out of range" in the loop. I'm
> assuming the test allows len(small) to be in the range [len(all)+1,
> 100+len(all)] because if few files were added, the order of the first
> len(all) files would change. I'm not sure if this is a correct
> assumption.
>
>
>
> http://codereview.appspot.com/4381048/
>

[r, 2011-04-12 18:20:07]

http://codereview.appspot.com/4381048/diff/4/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4381048/diff/4/src/pkg/os/dir_plan9.go#newcode42
src/pkg/os/dir_plan9.go:42: return []FileInfo{}, &PathError{"readdir",
file.name, e}
return nil, &PathError{"readdir", file.name, e}

http://codereview.appspot.com/4381048/diff/4/src/pkg/os/dir_plan9.go#newcode48
src/pkg/os/dir_plan9.go:48: return []FileInfo{}, &PathError{"readdir",
file.name, Eshortstat}
ditto

http://codereview.appspot.com/4381048/diff/4/src/pkg/os/dir_plan9.go#newcode56
src/pkg/os/dir_plan9.go:56: return []FileInfo{}, &PathError{"readdir",
file.name, Eshortstat}
ditto

http://codereview.appspot.com/4381048/diff/4/src/pkg/os/dir_plan9.go#newcode60
src/pkg/os/dir_plan9.go:60: return []FileInfo{}, &PathError{"readdir",
file.name, e}
ditto

[fhs, 2011-04-12 18:57:08]

*** Abandoned ***

[fhs, 2011-04-12 19:09:30]

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

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

[fhs, 2011-04-12 19:17:29]

On Tue, Apr 12, 2011 at 3:09 PM,  <fshahriar@gmail.com> wrote:
> Hello golang-dev@googlegroups.com, lucio, r (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> http://codereview.appspot.com/4381048/
>

Sorry, hold off on the review. I'm having trouble with codereview.py.
I accidentally ran hg mail with the wrong hgpatch. I think I'll
abandon this and create a new CL.

fhs

[fhs, 2011-04-12 20:44:12]

PTAL

Ignore the Abandoned message.

[r, 2011-04-12 21:40:01]

http://codereview.appspot.com/4381048/diff/2002/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4381048/diff/2002/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:27: file.dirinfo.buf = make([]byte, syscall.STATMAX)
if you're not going to release this - and i don't think you can - it makes
little sense to allocate it as a slice. just embed it as an array in the
dirInfo.

however, i'm nervous about the size of this temporary data.

ok for now but maybe worth thinking about in future

[fhs, 2011-04-12 23:44:52]

PTAL

http://codereview.appspot.com/4381048/diff/2002/src/pkg/os/dir_plan9.go
File src/pkg/os/dir_plan9.go (right):

http://codereview.appspot.com/4381048/diff/2002/src/pkg/os/dir_plan9.go#newco...
src/pkg/os/dir_plan9.go:27: file.dirinfo.buf = make([]byte, syscall.STATMAX)
On 2011/04/12 21:40:01, r wrote:
> if you're not going to release this - and i don't think you can - it makes
> little sense to allocate it as a slice. just embed it as an array in the
> dirInfo.
> 
> however, i'm nervous about the size of this temporary data.
> 
> ok for now but maybe worth thinking about in future

Done.

I've moved the declaration of dirInfo into file_plan9.go because it's also in
file_unix.go and file_windows.go.

[r, 2011-04-12 23:58:31]

LGTM

[r, 2011-04-12 23:59:03]

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

os: fix Readdir in Plan 9

'TestReaddir.*' tests now passes.

R=golang-dev, lucio, r
CC=golang-dev
http://codereview.appspot.com/4381048

Committer: Rob Pike <r@golang.org>