Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

Add an IsAttrib method on the FileEvent struct #79

Merged
merged 4 commits into from
Jan 17, 2014
Merged

Add an IsAttrib method on the FileEvent struct #79

merged 4 commits into from
Jan 17, 2014

Conversation

abustany
Copy link
Contributor

This IsAttrib function can be used to distinguish events that only
concern a file's metadata (eg. atime, mtime etc.).

The background for this commit is an app that I have which
monitors a folder, and reads new files that get copied there. On
OS X (at least, I don't get the same behaviour on Linux with a FS
mounted with the relatime option), reading the file will update the
atime and trigger a new event (creating a loop since then the app
will read the file again, and so on). The IsAttrib method allows me
to filter out attribute events.

Testing has been done on OS X and Linux.

@nathany
Copy link
Contributor

nathany commented Dec 24, 2013

Interesting. I wonder if some of what I thought was due to Spotlight #62 was actually due to the issue you were seeing.

Has this been tested on BSD, Linux, Windows and OS X? (see Contributing)
If possible, it would be nice to have an integration test.

@nathany
Copy link
Contributor

nathany commented Jan 4, 2014

@abustany See fsnotify_test for the integration tests we currently have.

@nathany
Copy link
Contributor

nathany commented Jan 7, 2014

Also, I think your description of IsAttrib here is a little more meaningful than "attribute change" in the comments.

@abustany
Copy link
Contributor Author

abustany commented Jan 9, 2014

Sorry about the delay, have been taken by other activities so far - I'll try to revive this PR soon taking your comments into account.

@nathany
Copy link
Contributor

nathany commented Jan 15, 2014

Fyi, I've mentioned this in the os/fsnotify API doc for Go 1.3 http://goo.gl/MrYxyA

@ottob
Copy link

ottob commented Jan 15, 2014

Maybe add IsAttrib to (e *FileEvent) String() also?

@nathany nathany mentioned this pull request Jan 16, 2014
9 tasks
@ottob
Copy link

ottob commented Jan 16, 2014

I would really like to see this go in. It helped me solve an issue I had with revel on OSX. https://github.com/robfig/revel/issues/460

@abustany
Copy link
Contributor Author

"funny" that I'm also the one that wrote the commit that triggers the revel bug...

I agree about the String() method remark, I'm still fairly busy but will bump this task up on my list of priorities.

This IsAttrib function can be used to distinguish events that only
concern a file's metadata (eg. atime, mtime etc.).
This is useful when you want to check the event count delta after doing
some operations.
@abustany
Copy link
Contributor Author

  • Rephrased a bit the comments in the source code for the IsAttrib method, to make it hopefully clearer
  • Added tests for the IsAttrib method. I ran the tests on Fedora 20, OS X 10.9.1 and Windows 7

@nathany
Copy link
Contributor

nathany commented Jan 17, 2014

@howeyc This looks good to me. 👍

I also pulled the branch and ran the tests successfully under OS X 10.9.1, Windows 7, and the 64-bit VMs in #80 (Ubuntu Precise 12.04, FreeBSD 9.1).

@howeyc
Copy link
Owner

howeyc commented Jan 17, 2014

Okay, just one concern. I noticed that attrib is still part of Modify. So an attribute change will still show IsModify() to be true as before, but also IsAttrib() to be true.

Is this what we want? I'm fine with it as long as we are aware this is how it works.

@abustany
Copy link
Contributor Author

The reasoning behind that was that I wanted the change to be backwards compatible... Not sure what kind of guarantees we want to give in this regard.

The question of whether an attribute change is a "modify" event or not is maybe more of a philosophical one, I must admit I don't have a strong opinion there :)

howeyc added a commit that referenced this pull request Jan 17, 2014
Add an IsAttrib method on the FileEvent struct
@howeyc howeyc merged commit 494ebc7 into howeyc:master Jan 17, 2014
@howeyc
Copy link
Owner

howeyc commented Jan 17, 2014

I like that its backwards compatible, looks good to me.

@nathany
Copy link
Contributor

nathany commented Jan 31, 2014

A backwards incompatible change to IsModify() is something we could consider for the new os/fsnotify API.

rsc pushed a commit to golang/exp that referenced this pull request Dec 7, 2014
Handle ERROR_MORE_DATA on Windows
(howeyc/fsnotify#49)

Run tests in random temp directories
(howeyc/fsnotify#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.
(howeyc/fsnotify#71)

Fix: Race on OS X between Close() and readEvents()
(howeyc/fsnotify#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.
(howeyc/fsnotify#77)

Add an IsAttrib method on the FileEvent struct
(howeyc/fsnotify#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://golang.org/cl/58500043
GoogleCodeExporter pushed a commit to bsed/go-zh.exp that referenced this pull request May 31, 2015
Handle ERROR_MORE_DATA on Windows
(howeyc/fsnotify#49)

Run tests in random temp directories
(howeyc/fsnotify#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.
(howeyc/fsnotify#71)

Fix: Race on OS X between Close() and readEvents()
(howeyc/fsnotify#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.
(howeyc/fsnotify#77)

Add an IsAttrib method on the FileEvent struct
(howeyc/fsnotify#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 up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants