Issue 58500043: code review 58500043: go.exp/fsnotify: bring up-to-date with GitHub (move dev... (Closed)
Descriptiongo.exp/fsnotify: bring up-to-date with GitHub (move development here)
Handle ERROR_MORE_DATA on Windows
(https://github.com/howeyc/fsnotify/issues/49)
Run tests in random temp directories
(https://github.com/howeyc/fsnotify/issues/57)
Fix: RemoveWatch is not removing the path from the watch list
The issue was that files watched internally were not being removed
when the parent directory's watch was removed.
(https://github.com/howeyc/fsnotify/issues/71)
Fix: Race on OS X between Close() and readEvents()
(https://github.com/howeyc/fsnotify/issues/70)
Fix: deadlock on BSD
The removeWatch routine could return without releasing the lock on
w.bufmut. This change unlocks the mutex before checking for errors.
(https://github.com/howeyc/fsnotify/pull/77)
Add an IsAttrib method on the FileEvent struct
(https://github.com/howeyc/fsnotify/pull/79)
Fix: a few typos
Test helpers for shared setup.
Patch Set 1 #Patch Set 2 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #Patch Set 3 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #
Total comments: 16
Patch Set 4 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #Patch Set 5 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #
Total comments: 9
Patch Set 6 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #Patch Set 7 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #Patch Set 8 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #
Total comments: 8
Patch Set 9 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #Patch Set 10 : diff -r 70d2efbdf5cf https://code.google.com/p/go.exp #
MessagesTotal messages: 32
Hello golang-codereviews@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go.exp
Sign in to reply to this message.
Have all the contributors to the external repo signed the Go CLA? > On 30 Jan 2014, at 14:05, nj@nathany.com wrote: > > Reviewers: golang-codereviews, > > Message: > Hello golang-codereviews@googlegroups.com, > > I'd like you to review this change to > https://code.google.com/p/go.exp > > > Description: > go.exp/fsnotify: bring up-to-date with GitHub (move development here) > > Handle ERROR_MORE_DATA on Windows > (https://github.com/howeyc/fsnotify/issues/49) > > Run tests in random temp directories > (https://github.com/howeyc/fsnotify/issues/57) > > Fix: RemoveWatch is not removing the path from the watch list > The issue was that files watched internally were not being removed > when the parent directory's watch was removed. > (https://github.com/howeyc/fsnotify/issues/71) > > Fix: Race on OS X between Close() and readEvents() > (https://github.com/howeyc/fsnotify/issues/70) > > Fix: deadlock on BSD > The removeWatch routine could return without releasing the lock on > w.bufmut. This change unlocks the mutex before checking for errors. > (https://github.com/howeyc/fsnotify/pull/77) > > Add an IsAttrib method on the FileEvent struct > (https://github.com/howeyc/fsnotify/pull/79) > > Fix: a few typos > > Please review this at https://codereview.appspot.com/58500043/ > > Affected files (+285, -78 lines): > M fsnotify/fsnotify.go > M fsnotify/fsnotify_bsd.go > M fsnotify/fsnotify_linux.go > M fsnotify/fsnotify_symlink_test.go > M fsnotify/fsnotify_test.go > M fsnotify/fsnotify_windows.go > > > -- > You received this message because you are subscribed to the Google Groups "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 move it into http://golang.org/src/pkg/syscall/ztypes_windows.go
Sign in to reply to this message.
On 2014/01/30 03:10:58, dfc wrote: > Have all the contributors to the external repo signed the Go CLA? Somewhat unlikely. :-( I've gone back through the commits since the last copy over here and pinged everyone who made *code* changes: https://github.com/howeyc/fsnotify/issues/78#issuecomment-33657880 Adrien Bustany abustany Caleb Spare cespare John C Barstow jbowtie henrikedwards tsg
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 On 2014/01/30 03:44:02, brainman wrote: > move it into http://golang.org/src/pkg/syscall/ztypes_windows.go go.exp/fsnotify needs to continue working on past versions of Go. Let's move this after we begin working on os/fsnotify.
Sign in to reply to this message.
On 2014/01/30 03:48:11, nathany wrote: > > go.exp/fsnotify needs to continue working on past versions of Go. Let's move > this after we begin working on os/fsnotify. Fair enough. Alex
Sign in to reply to this message.
On 2014/01/30 03:44:13, nathany wrote: > On 2014/01/30 03:10:58, dfc wrote: > > Have all the contributors to the external repo signed the Go CLA? > > Somewhat unlikely. :-( I've gone back through the commits since the last copy > over here and pinged everyone who made *code* changes: > https://github.com/howeyc/fsnotify/issues/78#issuecomment-33657880 > > Adrien Bustany abustany > Caleb Spare cespare > John C Barstow jbowtie > henrikedwards > tsg I have previously signed the CLA.
Sign in to reply to this message.
On 2014/01/30 03:52:02, cespare wrote: > On 2014/01/30 03:44:13, nathany wrote: > > On 2014/01/30 03:10:58, dfc wrote: > > > Have all the contributors to the external repo signed the Go CLA? > > > > John C Barstow jbowtie I have just electronically signed the CLA.
Sign in to reply to this message.
On 2014/01/30 06:43:16, jbowtie wrote: > On 2014/01/30 03:52:02, cespare wrote: > > On 2014/01/30 03:44:13, nathany wrote: > > > On 2014/01/30 03:10:58, dfc wrote: > > > > Have all the contributors to the external repo signed the Go CLA? > > > > > > John C Barstow jbowtie > > I have just electronically signed the CLA. I electronically signed it too now.
Sign in to reply to this message.
On 2014/01/30 07:55:21, abustany wrote: > On 2014/01/30 06:43:16, jbowtie wrote: > > On 2014/01/30 03:52:02, cespare wrote: > > > On 2014/01/30 03:44:13, nathany wrote: > > > > On 2014/01/30 03:10:58, dfc wrote: > > > > > Have all the contributors to the external repo signed the Go CLA? > > > > > > > > John C Barstow jbowtie > > > > I have just electronically signed the CLA. > > I electronically signed it too now. And I have just electronically signed the CLA.
Sign in to reply to this message.
On 2014/01/30 08:20:15, henrikedwards wrote: > On 2014/01/30 07:55:21, abustany wrote: > > On 2014/01/30 06:43:16, jbowtie wrote: > > > On 2014/01/30 03:52:02, cespare wrote: > > > > On 2014/01/30 03:44:13, nathany wrote: > > > > > On 2014/01/30 03:10:58, dfc wrote: > > > > > > Have all the contributors to the external repo signed the Go CLA? > > > > > > > > > > John C Barstow jbowtie > > > > > > I have just electronically signed the CLA. > > > > I electronically signed it too now. > > And I have just electronically signed the CLA. Thanks guys. That's everyone. I double checked the commits and tsg's contribution was part of a previous CL: https://code.google.com/p/go/source/detail?r=1b287a064d6cf96dbd24f579508fd2e5... There was a contribution to the README and someone ran gofmt, but I've only brought over the code changes.
Sign in to reply to this message.
> There was a contribution to the README and someone ran gofmt, but I've only > brought over the code changes. I'm not sure if my one line change (https://github.com/howeyc/fsnotify/commit/e26bdc27cada5911ba01ffa9c7b61081376...) needs it but I've signed the CLA just in case it makes things easier.
Sign in to reply to this message.
I have electronically signed the CLA (@tmc / travis.cline@gmail.com)
Sign in to reply to this message.
R=golang-dev (assigned by dave@cheney.net)
Sign in to reply to this message.
There is an issue open on GitHub to ping everyone who contributed to fsnotify to sign the CLA (not just the people who committed these changes): https://github.com/howeyc/fsnotify/issues/85 Is there anything else that needs to be done (as far as the code) before this can be merged I can start on the next CL? :-)
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_linux.go File fsnotify/fsnotify_linux.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_linux.go... fsnotify/fsnotify_linux.go:59: selectWaitTime = 100e6 please use 100 * time.Millisecond https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_symlink_... File fsnotify/fsnotify_symlink_test.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_symlink_... fsnotify/fsnotify_symlink_test.go:23: var testDir string = testTempDir() testdir, err := ioutil.TempDir("", "fsnotify") https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_symlink_... fsnotify/fsnotify_symlink_test.go:28: } delete https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go File fsnotify/fsnotify_test.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:22: r = rand.New(rand.NewSource(time.Now().UnixNano())) why is this in an init block ? var r = rand.New(...) https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:25: func testTempDir() string { Drop this https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:36: func (c *counter) increment() { atomic.AddInt32(&c.val, 1) https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:66: var testDir string = testTempDir() see previous, use ioutil.TempDir to create a unique root for this test run, then create your subdirectory underneath https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 On 2014/01/30 03:48:12, nathany wrote: > On 2014/01/30 03:44:02, brainman wrote: > > move it into http://golang.org/src/pkg/syscall/ztypes_windows.go > > go.exp/fsnotify needs to continue working on past versions of Go. Let's move > this after we begin working on os/fsnotify. If that is the case, then this constant should be moved to a separate file and a build tag used for compatibility.
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_bsd.go File fsnotify/fsnotify_bsd.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_bsd.go#n... fsnotify/fsnotify_bsd.go:62: type Watcher struct { There are a horrifying number of mutexes in this package. Can you demonstrate that they improve the performance significantly over a course grained sync.RWMutex.
Sign in to reply to this message.
For the avoidance of doubt, I have a concern that the code contributed here has not been properly handled with respect to copyright ownership. IANAL so I will not comment on this further, the final decision to commit this change must come from someone else. On Fri, Jan 31, 2014 at 2:51 PM, <dave@cheney.net> wrote: > > https://codereview.appspot.com/58500043/diff/40001/ > fsnotify/fsnotify_bsd.go > File fsnotify/fsnotify_bsd.go (right): > > https://codereview.appspot.com/58500043/diff/40001/ > fsnotify/fsnotify_bsd.go#newcode62 > fsnotify/fsnotify_bsd.go:62: type Watcher struct { > There are a horrifying number of mutexes in this package. Can you > demonstrate that they improve the performance significantly over a > course grained sync.RWMutex. > > https://codereview.appspot.com/58500043/ >
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, alex.brainman@gmail.com, cespare@gmail.com, jbowtie@gmail.com, webustany@gmail.com, henrik.edwards@gmail.com, phmmnd@gmail.com, travis.cline@gmail.com, gobot@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
Thanks for the code review. Tests continue to pass on OS X 10.9, Windows 7, Ubuntu 12.04 and FreeBSD 9. (64-bit) https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_bsd.go File fsnotify/fsnotify_bsd.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_bsd.go#n... fsnotify/fsnotify_bsd.go:62: type Watcher struct { On 2014/01/31 03:51:30, dfc wrote: > There are a horrifying number of mutexes in this package. Can you demonstrate > that they improve the performance significantly over a course grained > sync.RWMutex. Horrifying is a good word for it. :-) I know I want to remove fsnFlags and fsnmut entirely as my next CL, so that's one less. I made a note to try out a RWMutex instead as part of a future CL. https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_linux.go File fsnotify/fsnotify_linux.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_linux.go... fsnotify/fsnotify_linux.go:59: selectWaitTime = 100e6 On 2014/01/31 03:50:20, dfc wrote: > please use 100 * time.Millisecond Good call. Turns out this code isn't actually used as Select() syscall was removed: https://github.com/howeyc/fsnotify/commit/7beb45183d2ab48bfc9616d9a019513a123... https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go File fsnotify/fsnotify_test.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:22: r = rand.New(rand.NewSource(time.Now().UnixNano())) On 2014/01/31 03:50:20, dfc wrote: > why is this in an init block ? > > var r = rand.New(...) It looks like this was only here for r.Int() in testTempDir(). Removing. https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 On 2014/01/31 03:50:20, dfc wrote: > If that is the case, then this constant should be moved to a separate file and a > build tag used for compatibility. This entire file is a _windows file with a +build tag for Windows. Sorry if I don't quite understand what's so special about this particular constant? If you let me know what this other file should be named, I'll make it so.
Sign in to reply to this message.
A few more comments. I think the duplication in the test setup needs to be factored out into a handler. https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go File fsnotify/fsnotify_test.go (right): https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:65: var testFile string = filepath.Join(testDir, "TestFsnotifySeq.testfile") testFile := filepath.Join(....) https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:66: var testFileRenamed string = filepath.Join(testDirToMoveFiles, "TestFsnotifySeqRename.testfile") same https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:69: err = watcher.Watch(testDir) if err := ... ; err != nil { ... } https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:71: t.Fatalf("watcher.Watch() failed: %s", err) "watcher.Watch(%q) failed: %v", testDir, err https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:187: var testFile string = filepath.Join(testDir, "TestFsnotifySeq.testfile") testFile := ... https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:190: err = watcher.Watch(testDir) as above https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:192: t.Fatalf("watcher.Watch() failed: %s", err) as above https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:193: } This code appears to be similar to the previous test, can it be factored into a helper ? https://codereview.appspot.com/58500043/diff/80001/fsnotify/fsnotify_test.go#... fsnotify/fsnotify_test.go:315: var testFileAlreadyExists string = filepath.Join(testDir, "TestFsnotifyEventsExisting.testfile") as above
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 On 2014/01/31 05:10:29, nathany wrote: > On 2014/01/31 03:50:20, dfc wrote: > > If that is the case, then this constant should be moved to a separate file and > a > > build tag used for compatibility. > > This entire file is a _windows file with a +build tag for Windows. > > Sorry if I don't quite understand what's so special about this particular > constant? If you let me know what this other file should be named, I'll make it > so. Alex is suggesting moving this constant into the windows constants in the syscall package for Go 1.3. You would need to add two files to this package fsnotify_windows_go13.go with the contents // +build !go1 // +build !go11 // +build !go12 package fsnotify import "syscall" const sys_ERROR_MORE_DATA = syscall.SOMETHING and another file, maybe fsnotify_windows_go12.go package fsnotify const sys_ERROR_MORE_DATA = 234
Sign in to reply to this message.
Thanks for the clarification. Though I must say, this seems like a bit much for one constant, given that fsnotify_windows_go12.go will be entirely unnecessary in os/fsnotify. Is there a good reason not to just add a "// TODO(nj):" and come back to it in os/fsnotify? On 2014/01/31 05:25:17, dfc wrote: > https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go > File fsnotify/fsnotify_windows.go (right): > > https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... > fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = 234 > On 2014/01/31 05:10:29, nathany wrote: > > On 2014/01/31 03:50:20, dfc wrote: > > > If that is the case, then this constant should be moved to a separate file > and > > a > > > build tag used for compatibility. > > > > This entire file is a _windows file with a +build tag for Windows. > > > > Sorry if I don't quite understand what's so special about this particular > > constant? If you let me know what this other file should be named, I'll make > it > > so. > > Alex is suggesting moving this constant into the windows constants in the > syscall package for Go 1.3. > > You would need to add two files to this package > > fsnotify_windows_go13.go > > with the contents > > // +build !go1 > // +build !go11 > // +build !go12 > > package fsnotify > > import "syscall" > > const sys_ERROR_MORE_DATA = syscall.SOMETHING > > and another file, maybe > > fsnotify_windows_go12.go > > package fsnotify > > const sys_ERROR_MORE_DATA = 234
Sign in to reply to this message.
Sounds reasonable to me. > On 31 Jan 2014, at 18:40, nj@nathany.com wrote: > > > Thanks for the clarification. Though I must say, this seems like a bit > much for one constant, given that fsnotify_windows_go12.go will be > entirely unnecessary in os/fsnotify. Is there a good reason not to just > add a "// TODO(nj):" and come back to it in os/fsnotify? > > > On 2014/01/31 05:25:17, dfc wrote: > > https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.go >> File fsnotify/fsnotify_windows.go (right): > > > https://codereview.appspot.com/58500043/diff/40001/fsnotify/fsnotify_windows.... >> fsnotify/fsnotify_windows.go:45: sys_ERROR_MORE_DATA syscall.Errno = > 234 >> On 2014/01/31 05:10:29, nathany wrote: >> > On 2014/01/31 03:50:20, dfc wrote: >> > > If that is the case, then this constant should be moved to a > separate file >> and >> > a >> > > build tag used for compatibility. >> > >> > This entire file is a _windows file with a +build tag for Windows. >> > >> > Sorry if I don't quite understand what's so special about this > particular >> > constant? If you let me know what this other file should be named, > I'll make >> it >> > so. > >> Alex is suggesting moving this constant into the windows constants in > the >> syscall package for Go 1.3. > >> You would need to add two files to this package > >> fsnotify_windows_go13.go > >> with the contents > >> // +build !go1 >> // +build !go11 >> // +build !go12 > >> package fsnotify > >> import "syscall" > >> const sys_ERROR_MORE_DATA = syscall.SOMETHING > >> and another file, maybe > >> fsnotify_windows_go12.go > >> package fsnotify > >> const sys_ERROR_MORE_DATA = 234 > > > > https://codereview.appspot.com/58500043/ > > -- > You received this message because you are subscribed to the Google Groups "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/groups/opt_out.
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, alex.brainman@gmail.com, gobot@golang.org (cc: bradfitz@golang.org, cespare@gmail.com, golang-codereviews@googlegroups.com, henrik.edwards@gmail.com, jbowtie@gmail.com, travis.cline@gmail.com, webustany@gmail.com), Please take another look.
Sign in to reply to this message.
On 2014/01/31 05:18:14, dfc wrote: > A few more comments. I think the duplication in the test setup needs to be > factored out into a handler. Thanks for motivating me to clean up the tests. I'm sure there's still more room for improvement, and we'll be in there a lot as the API is modified. As to Copyright and CLAs, the initial code is based on go.exp/inotify. I've double checked the commit log on GitHub to ensure everyone has been prompted to sign the CLA. This is everyone for the lime time of the project, not just the commits included in this CL. The list of names/emails is here: https://github.com/howeyc/fsnotify/issues/85 So far all but 2 people indicated that they have signed the CLA. They had relatively small changes: * running gofmt on a file, which I've also done on every file (bronze1man * a README update, the README on GitHub has not been transferred to go.exp/fsnotify, though we may want a doc.go at some point. (Denis Brandolini) Brad Fitzpatrick offered to verify that the CLAs have been submitted. Thanks very much!
Sign in to reply to this message.
On 2014/02/01 03:53:13, nathany wrote: > Brad Fitzpatrick offered to verify that the CLAs have been submitted. Thanks > very much! Everyone who has a commit on howeyc/fsnotify has now signed the CLA and Brad Fitzpatrick added them to AUTHORS/CONTRIBUTORS. https://github.com/howeyc/fsnotify/issues/85 Anything else, or can this CL be submitted?
Sign in to reply to this message.
https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_bsd.go File fsnotify/fsnotify_bsd.go (right): https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_bsd.go#... fsnotify/fsnotify_bsd.go:56: // IsAttrib reports whether the FileEvent was triggered by a change in the file metadata (eg. "e.g." is spelled with periods, and Go documentation avoids it anyhow. Please just write "for example." Or actually just omit the parenthetical clause entirely, anybody who understands what "atime" and "mtime" mean understands what "file metadata" is. https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_bsd.go#... fsnotify/fsnotify_bsd.go:275: pathsToRemove := make([]string, 0) Just write var pathsToRemove []string https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_bsd.go#... fsnotify/fsnotify_bsd.go:281: if _, extern := w.externalWatches[wpath]; !extern { I think you can just write if !w.externalWatches[wpath] It's a map to bool, so a missing element will be read as false. I don't think you need to use ,ok syntax here. https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_bsd.go#... fsnotify/fsnotify_bsd.go:288: for idx := 0; idx < len(pathsToRemove); idx++ { This seems to be simply for _, p := range pathsToRemove { w.removeWatch(p) } https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_linux.go File fsnotify/fsnotify_linux.go (right): https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_linux.g... fsnotify/fsnotify_linux.go:86: // atime, mtime etc.) Same comment as before. https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_linux.g... fsnotify/fsnotify_linux.go:220: n, errno = syscall.Read(w.fd, buf[0:]) s/0// https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_windows.go File fsnotify/fsnotify_windows.go (right): https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_windows... fsnotify/fsnotify_windows.go:76: // atime, mtime etc.) Same comment as before. https://codereview.appspot.com/58500043/diff/140001/fsnotify/fsnotify_windows... fsnotify/fsnotify_windows.go:456: //The i/o succeeded but buffer is full Add a space after each "//".
Sign in to reply to this message.
Hello golang-codereviews@googlegroups.com, dave@cheney.net, alex.brainman@gmail.com, gobot@golang.org, bradfitz@golang.org, iant@golang.org (cc: bradfitz@golang.org, bronze1man@gmail.com, cespare@gmail.com, denis.brandolini@gmail.com, golang-codereviews@googlegroups.com, henrik.edwards@gmail.com, jbowtie@gmail.com, travis.cline@gmail.com, webustany@gmail.com), Please take another look.
Sign in to reply to this message.
LGTM This is OK for go.exp. Thanks for working on it. Do you need someone to submit it for you?
Sign in to reply to this message.
On 2014/02/06 05:30:53, iant wrote: > LGTM > > This is OK for go.exp. Understood. I'm expecting more cleanup as we move this code base towards the new API. > Thanks for working on it. My pleasure. > Do you need someone to submit it for you? Yes please. ^_^
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=b6cb26fb5e26&repo=exp *** go.exp/fsnotify: bring up-to-date with GitHub (move development here) Handle ERROR_MORE_DATA on Windows (https://github.com/howeyc/fsnotify/issues/49) Run tests in random temp directories (https://github.com/howeyc/fsnotify/issues/57) Fix: RemoveWatch is not removing the path from the watch list The issue was that files watched internally were not being removed when the parent directory's watch was removed. (https://github.com/howeyc/fsnotify/issues/71) Fix: Race on OS X between Close() and readEvents() (https://github.com/howeyc/fsnotify/issues/70) Fix: deadlock on BSD The removeWatch routine could return without releasing the lock on w.bufmut. This change unlocks the mutex before checking for errors. (https://github.com/howeyc/fsnotify/pull/77) Add an IsAttrib method on the FileEvent struct (https://github.com/howeyc/fsnotify/pull/79) Fix: a few typos Test helpers for shared setup. LGTM=iant R=golang-codereviews, dave, alex.brainman, gobot, bradfitz, iant CC=bradfitz, bronze1man, cespare, denis.brandolini, golang-codereviews, henrik.edwards, jbowtie, travis.cline, webustany https://codereview.appspot.com/58500043 Committer: Ian Lance Taylor <iant@golang.org>
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
