code review 4536058: os: make Readdir & Readdirnames return os.EOF at end

os: make Readdir & Readdirnames return os.EOF at end

Fixes issue 678

[bradfitz, 2011-05-13 18:22:20]

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

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

[bradfitz, 2011-05-13 18:25:26]

I'd like to add more tests for this but such tests would involve creating
directories & files.

The current readdir tests seem to go out of their way to avoid writing to
disk, but I'm not sure why.

Any objections?  Maybe it was just an early bootstrapping issue?


On Fri, May 13, 2011 at 11:22 AM, <bradfitz@golang.org> wrote:

> Reviewers: rsc,
>
> Message:
> Hello rsc@golang.org (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg
>
>
> Description:
> os: make Readdir & Readdirnames return os.EOF at end
>
> Fixes issue 678
>
> Please review this at http://codereview.appspot.com/4536058/
>
> Affected files:
>  M src/pkg/os/dir_unix.go
>  M src/pkg/os/dir_windows.go
>  M src/pkg/os/file_unix.go
>  M src/pkg/os/file_windows.go
>  M src/pkg/os/os_test.go
>  M src/pkg/os/path.go
>
>
>

[r, 2011-05-13 18:26:49]

LGTM but maybe wait for windows reviewer to check that part.

[bradfitz, 2011-05-13 18:34:18]

Yeah, the Windows stuff I just did blind.

We really need to get docs on how to do builds & tests in an EC2 VM.

On Fri, May 13, 2011 at 11:26 AM, <r@golang.org> wrote:

> LGTM but maybe wait for windows reviewer to check that part.
>
>
> http://codereview.appspot.com/4536058/
>

[brainman, 2011-05-13 23:46:03]

LGTM.

I couldn't test it properly, because build is broken at the moment:

test go/scanner
test go/typechecker
test go/types
TEST FAIL go/types
make[1]: Entering directory `/C/MinGW/go/src/pkg/go/types'
gotest -test.short -test.timeout=120
rm -f _test/go/types.a
8g  -o _gotest_.8 check.go const.go exportdata.go gcimporter.go types.go univers
e.go  check_test.go gcimporter_test.go
rm -f _test/go/types.a
gopack grc _test/go/types.a _gotest_.8
--- FAIL: types.TestCheck (0.08 seconds)
        testdata/test0.src:86:2: no (multiple?) error expected, but found:
illegal cycle in declaration of S7
        testdata/test0.src:98:2: no (multiple?) error expected, but found:
illegal cycle in declaration of A4
        testdata/test0.src:139:2: no (multiple?) error expected, but found:
illegal cycle in declaration of I11
        test0: 3 errors not reported:
        testdata/test0.src:95:2: illegal cycle
        testdata/test0.src:133:2: illegal cycle
        testdata/test0.src:83:2: illegal cycle
FAIL
gotest: "./8.out.exe -test.short=true -test.timeout=120" failed: exit status 1
make[1]: *** [testshort] Error 2
make[1]: Leaving directory `/C/MinGW/go/src/pkg/go/types'
make: *** [go/types.testshort] Error 1

But I did run test in pkg/os and it passes:

$ make clean test
rm -rf *.o *.a *.[568vq] [568vq].out *.so _obj _test _testmain.go *.exe _cgo* *.
cgo[12].* test.out build.out
gotest
rm -f _test/os.a
8g  -o _gotest_.8 error.go env.go exec.go file.go getwd.go path.go proc.go
stat_windows.go time.go types.go  dir_windows.go error_posix.go env_windows.go
file_posix.go file_windows.go sys_windows.go exec_posix.go exec_windows.go
rm -f _test/os.a
gopack grc _test/os.a _gotest_.8
PASS

Thank you.

Alex

http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/file_windows.go
File src/pkg/os/file_windows.go (right):

http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/file_windows.go#ne...
src/pkg/os/file_windows.go:167: return
I'm not sure if you need to change this bit of code. I suspect,
syscall.ERROR_NO_MORE_FILES covers EOF scenario that you're dancing around.
"Else" part of code deal with "other" errors. But, I don't think it is important
what fi set on return, when not EOF error occurs, so let's leave it.

[bradfitz, 2011-05-13 23:48:20]

On Fri, May 13, 2011 at 4:46 PM, <alex.brainman@gmail.com> wrote:

> LGTM.
>
...

>
>  But I did run test in pkg/os and it passes:
>
...

> Thank you.
>
> Alex
>

Cool!  I now have a Windows VM running on Amazon EC2.  How do I setup a
build tree?  I don't want just the pre-built weekly releases.  I want "hg"
and "./all.bash" and everything.  Are there docs somewhere?



> http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/file_windows.go
> File src/pkg/os/file_windows.go (right):
>
>
>
http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/file_windows.go#ne...
> src/pkg/os/file_windows.go:167: return
> I'm not sure if you need to change this bit of code. I suspect,
> syscall.ERROR_NO_MORE_FILES covers EOF scenario that you're dancing
> around. "Else" part of code deal with "other" errors. But, I don't think
> it is important what fi set on return, when not EOF error occurs, so
> let's leave it.


I think it is what Russ wanted (see the new docs ... if -1 it returns
everything it tried to get so far?)

[bsiegert, 2011-05-14 17:10:35]

On Sat, May 14, 2011 at 02:40, brainman <alex.brainman@gmail.com> wrote:
> There is no vi, so you have to find another editor, perhaps some Windows
> editor that can do files with LFs.

Or you download vim for Windows from vim.org and install it. This will
give you both a "vim" and a "gvim" command.

--Benny.

-- 
The first essential in chemistry is that you should perform practical
work and conduct experiments, for he who performs not practical work
nor makes experiments will never attain the least degree of mastery.
        -- Abu Musa Jabir ibn Hayyan (721-815)

[Joe Poirier, 2011-05-14 20:42:08]

On May 14, 2011, at May 14 - 12:10 PM, Benny Siegert wrote:

> On Sat, May 14, 2011 at 02:40, brainman <alex.brainman@gmail.com> wrote:
>> There is no vi, so you have to find another editor, perhaps some Windows
>> editor that can do files with LFs.
> 
> Or you download vim for Windows from vim.org and install it. This will
> give you both a "vim" and a "gvim" command.
> 


Also the latest go-mingw zip I uploaded includes mingw-get, so you should be
able to do $ mingw-get install msys-vim

-joe

[rsc, 2011-05-16 16:13:33]

LGTM

http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/dir_windows.go
File src/pkg/os/dir_windows.go (right):

http://codereview.appspot.com/4536058/diff/4001/src/pkg/os/dir_windows.go#new...
src/pkg/os/dir_windows.go:8: fis, e := file.Readdir(n)
s/e/err/
throughout.
it was e because the language used to be different

[bradfitz, 2011-05-16 16:26:22]

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

os: make Readdir & Readdirnames return os.EOF at end

Fixes issue 678

R=rsc, r, alex.brainman, bsiegert, jdpoirier
CC=golang-dev
http://codereview.appspot.com/4536058