code review 3559041: os/dir{darwin,freebsd,linux,nacl}.go: remove fstat TODO

os/dir{darwin,freebsd,linux,nacl}.go: remove fstat TODO

[cainetighe, 2010-12-09 21:37:32]

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

I'd like you to review this change.

[r, 2010-12-09 21:45:19]

http://codereview.appspot.com/3559041/diff/2001/src/pkg/os/dir_darwin.go
File src/pkg/os/dir_darwin.go (right):

http://codereview.appspot.com/3559041/diff/2001/src/pkg/os/dir_darwin.go#newc...
src/pkg/os/dir_darwin.go:22: fstatBuf := new(syscall.Statfs_t)
this adds an allocation and a system call to every call to Readdirnames.  TODO
notwithstanding, that bothers me. it also bothers me that this code is
duplicated in every dir_unix.go.

what happens if blockSize is too small? do we miss entries?

http://codereview.appspot.com/3559041/diff/2001/src/pkg/os/dir_darwin.go#newc...
src/pkg/os/dir_darwin.go:26: if errno != 0 {
these two lines can be folded together, constraining the scope of errno

[rsc1, 2010-12-09 21:47:03]

http://codereview.appspot.com/3559041/diff/2001/src/pkg/os/dir_darwin.go
File src/pkg/os/dir_darwin.go (right):

http://codereview.appspot.com/3559041/diff/2001/src/pkg/os/dir_darwin.go#newc...
src/pkg/os/dir_darwin.go:22: fstatBuf := new(syscall.Statfs_t)
On 2010/12/09 21:45:19, r wrote:
> this adds an allocation and a system call to every call to Readdirnames.  TODO
> notwithstanding, that bothers me. it also bothers me that this code is
> duplicated in every dir_unix.go.
> 
> what happens if blockSize is too small? do we miss entries?

at the least this should be

var st syscall.Statfs_t
syscall.Fstatfs(file.fd, &st)
blocksize := st.Bsize
if blocksize == 0 {
    blocksize = 4096
}

but I agree with Rob: the actual block size
here doesn't really matter much.
I'd rather just delete the TODO.

[cainetighe, 2010-12-09 21:53:14]

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

Please take another look.

[r, 2010-12-09 21:57:17]

rather than put you through the CLA dance for this, how about if i
just make these changes for you?

-rob

[cainetighe, 2010-12-09 23:17:12]

Sure if you want to; however,
3536041<http://codereview.appspot.com/3536041/>is still open as well.
I'd wait to do that one, since I'd like some more
feedback from the community on the change.  I'm planning on rolling issue
678 into this change too.

thanks for the help!

On Thu, Dec 9, 2010 at 4:57 PM, Rob 'Commander' Pike <r@golang.org> wrote:

> rather than put you through the CLA dance for this, how about if i
> just make these changes for you?
>
> -rob
>

[rsc, 2011-01-19 19:41:58]

removing reviewers

[cainetighe, 2011-03-10 04:13:03]

this change still exists and hasn't been committed -- should i rename it?
On 2011/01/19 19:41:58, rsc wrote:
> removing reviewers

[rsc, 2011-03-11 15:02:51]

LGTM

Sorry, I thought this had been taken care of in a separate CL.
It looks fine to me but please hg sync and hg mail so that
it is diffing against a more recent tree.

Russ

[cainetighe, 2011-03-11 19:40:28]

I no longer have that computer with that CL on it -- is there a way I
can import that change to my new machine?  hg download <change>
doesn't seem to pull it into hg p

thx :)

caine

On Fri, Mar 11, 2011 at 10:02 AM, Russ Cox <rsc@golang.org> wrote:
> LGTM
>
> Sorry, I thought this had been taken care of in a separate CL.
> It looks fine to me but please hg sync and hg mail so that
> it is diffing against a more recent tree.
>
> Russ
>

[cainetighe, 2011-03-11 20:08:42]

Russ,

   I think i've figured this out after all by running hg change / mail
it seemed to import it.  It has since been updated it says.

caine

On Fri, Mar 11, 2011 at 2:39 PM, caine tighe <arctanofyourface@gmail.com> wrote:
> I no longer have that computer with that CL on it -- is there a way I
> can import that change to my new machine?  hg download <change>
> doesn't seem to pull it into hg p
>
> thx :)
>
> caine
>
> On Fri, Mar 11, 2011 at 10:02 AM, Russ Cox <rsc@golang.org> wrote:
>> LGTM
>>
>> Sorry, I thought this had been taken care of in a separate CL.
>> It looks fine to me but please hg sync and hg mail so that
>> it is diffing against a more recent tree.
>>
>> Russ
>>
>

[cainetighe, 2011-03-11 20:15:14]

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

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

[cainetighe, 2011-03-14 20:17:54]

ping :)

On 2011/03/11 20:15:14, cainetighe wrote:
> Hello rsc (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

[rsc, 2011-03-17 17:57:22]

LGTM

[rsc, 2011-03-17 17:57:38]

*** Submitted as 822dc33fdd8c ***

os: remove fstat TODO

R=rsc
CC=golang-dev
http://codereview.appspot.com/3559041

Committer: Russ Cox <rsc@golang.org>