code review 5794073: os: IsExist() and IsNotExist() should also consider ENO... (issue 5794073)

[minu...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com (cc: golan...@googlegroups.com),

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

Description:
os: IsExist() and IsNotExist() should also consider ENOTDIR on Windows

Please review this at http://codereview.appspot.com/5794073/

Affected files:
M src/pkg/os/error.go
M src/pkg/os/error_test.go
M src/pkg/os/error_windows.go

Index: src/pkg/os/error.go
===================================================================
--- a/src/pkg/os/error.go
+++ b/src/pkg/os/error.go
@@ -43,14 +43,14 @@
return &SyscallError{syscall, err}
}

-// IsExist returns whether the error is known to report that a file
already exists.
-// It is satisfied by ErrExist as well as some syscall errors.
+// IsExist returns whether the error is known to report that a file or
directory
+// already exists. It is satisfied by ErrExist as well as some syscall
errors.
func IsExist(err error) bool {
return isExist(err)
}

-// IsNotExist returns whether the error is known to report that a file
does not exist.
-// It is satisfied by ErrNotExist as well as some syscall errors.
+// IsNotExist returns whether the error is known to report that a file or
directory
+// does not exist. It is satisfied by ErrNotExist as well as some syscall
errors.
func IsNotExist(err error) bool {
return isNotExist(err)
}
Index: src/pkg/os/error_test.go
===================================================================
--- a/src/pkg/os/error_test.go
+++ b/src/pkg/os/error_test.go
@@ -5,8 +5,10 @@
package os_test

import (
+ "fmt"
"io/ioutil"
"os"
+ "path/filepath"
"testing"
)

@@ -24,8 +26,56 @@
t.Fatal("Open should have failed")
return
}
- if !os.IsExist(err) {
- t.Fatalf("os.IsExist does not work as expected for %#v", err)
+ if s := checkErrorPredicate("os.IsExist", os.IsExist, err); s != "" {
+ t.Fatal(s)
return
}
}
+
+func testErrNotExist(name string) string {
+ f, err := os.Open(name)
+ if err == nil {
+ f.Close()
+ return "Open should have failed"
+ }
+ if s := checkErrorPredicate("os.IsNotExist", os.IsNotExist, err); s != ""
{
+ return s
+ }
+
+ err = os.Chdir(name)
+ if err == nil {
+ return "Chdir should have failed"
+ }
+ if s := checkErrorPredicate("os.IsNotExist", os.IsNotExist, err); s != ""
{
+ return s
+ }
+ return ""
+}
+
+func TestErrIsNotExist(t *testing.T) {
+ tmpDir, err := ioutil.TempDir("", "_Go_ErrIsNotExist")
+ if err != nil {
+ t.Fatalf("create ErrIsNotExist tempdir: %s", err)
+ return
+ }
+ defer os.RemoveAll(tmpDir)
+
+ name := filepath.Join(tmpDir, "NotExists")
+ if s := testErrNotExist(name); s != "" {
+ t.Fatal(s)
+ return
+ }
+
+ name = filepath.Join(name, "NotExists2")
+ if s := testErrNotExist(name); s != "" {
+ t.Fatal(s)
+ return
+ }
+}
+
+func checkErrorPredicate(predName string, pred func(error) bool, err
error) string {
+ if !pred(err) {
+ return fmt.Sprintf("%s does not work as expected for %#v", predName, err)
+ }
+ return ""
+}
Index: src/pkg/os/error_windows.go
===================================================================
--- a/src/pkg/os/error_windows.go
+++ b/src/pkg/os/error_windows.go
@@ -18,7 +18,9 @@
if pe, ok := err.(*PathError); ok {
err = pe.Err
}
- return err == syscall.ENOENT || err == ErrNotExist
+ // Windows treat some of the error traditionally governed by ENOENT as
ENOTDIR
+ // for example: chdir("C:/not_exists/not_exists2").
+ return err == syscall.ENOENT || err == syscall.ENOTDIR || err ==
ErrNotExist
}

func isPermission(err error) bool {

[minu...@gmail.com]

PTAL, changed the description to:
os: IsNotExist() should also consider ENOTDIR on Windows

IsExist() is not affected by this CL.

Rationale for this CL:
chdir("C:\notexists\notexists2") return the error as ENOTDIR, but the
same error
on Unix is ENOENT;
we already include both file and dir errors in IsExist(), so I think we
should also
make IsNotExist() matching behavior in this regard.

This CL also updates the documentation about IsExist() and IsNotExist(),
because
they are not only about files.

http://codereview.appspot.com/5794073/

[r...@golang.org]

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go
File src/pkg/os/error_windows.go (right):

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode13
src/pkg/os/error_windows.go:13: return err == syscall.EEXIST || err ==
syscall.ERROR_ALREADY_EXISTS ||
Nothing in package syscall for Windows generates EEXIST; please remove.

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode23
src/pkg/os/error_windows.go:23: return err == syscall.ENOENT || err ==
syscall.ENOTDIR || err == ErrNotExist
s/ENOENT/ERROR_FILE_NOT_FOUND/
s/ENOTDIR/ERROR_PATH_NOT_FOUND/

Then you don't need the comment.
It's unfortunate that we didn't remove those aliases a while back
but it is too late now.

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode30
src/pkg/os/error_windows.go:30: return err == syscall.EACCES || err ==
syscall.EPERM || err == ErrPermission
Nothing on Windows generates EACCES or EPERM; please remove.

http://codereview.appspot.com/5794073/

[minux]

PTAL.

On Tue, Mar 13, 2012 at 9:24 PM, <r...@golang.org> wrote:

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go
File src/pkg/os/error_windows.go (right):

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode13
src/pkg/os/error_windows.go:13: return err == syscall.EEXIST || err ==
syscall.ERROR_ALREADY_EXISTS ||
Nothing in package syscall for Windows generates EEXIST; please remove.

Done. 

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode23
src/pkg/os/error_windows.go:23: return err == syscall.ENOENT || err ==
syscall.ENOTDIR || err == ErrNotExist
s/ENOENT/ERROR_FILE_NOT_FOUND/
s/ENOTDIR/ERROR_PATH_NOT_FOUND/

Then you don't need the comment.
It's unfortunate that we didn't remove those aliases a while back
but it is too late now.

Done. Should I add comment about these inaccurate aliases in syscall?

http://codereview.appspot.com/5794073/diff/5/src/pkg/os/error_windows.go#newcode30
src/pkg/os/error_windows.go:30: return err == syscall.EACCES || err ==
syscall.EPERM || err == ErrPermission
Nothing on Windows generates EACCES or EPERM; please remove.

Done.

[r...@golang.org]

LGTM

This is fine for now. I don't think we need an explicit comment in
syscall.

http://codereview.appspot.com/5794073/

[minu...@gmail.com]

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

os: IsNotExist() should also consider ERROR_PATH_NOT_FOUND on Windows
Also update documentation about IsExist() and IsNotExist(), they are
not
about files only.

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

http://codereview.appspot.com/5794073/