Merge lp:~mandel/ubuntuone-client/add-virtual-watches into lp:ubuntuone-client
| Status: | Rejected |
|---|---|
| Rejected by: | dobey on 2012-04-27 |
| Proposed branch: | lp:~mandel/ubuntuone-client/add-virtual-watches |
| Merge into: | lp:ubuntuone-client |
| Diff against target: |
1545 lines (+636/-333) 3 files modified
tests/platform/windows/test_filesystem_notifications.py (+527/-255) ubuntuone/platform/windows/filesystem_notifications.py (+102/-75) ubuntuone/platform/windows/os_helper.py (+7/-3) |
| To merge this branch: | bzr merge lp:~mandel/ubuntuone-client/add-virtual-watches |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Facundo Batista | Needs Fixing on 2012-01-25 | ||
| Diego Sarmentero (community) | 2012-01-16 | Approve on 2012-01-17 | |
|
Review via email:
|
|||
Commit Message
Fixes lp:907511
This branch allows the Windows Watch implementation to behave more like the one found on pyinotify. The way this is achieved is the following:
* Moved _subdirs var to be a dictionary that store a map between a child path and if it is watched or not. The reason is that child_paths are relative to self._path which means that we store less info. The boolean state if we are watching the path, if not, we ignore all the events from that path.
* When we start watching all child events are added to _subdirs are not being watch until sd explicitly asks it.
* When adding a child we add it children to the _subdirs. The are not thread or run issues because the sd will add the watches always in the main twisted thread which is the one used to process the event.
* Moved _ignored_paths to be a set for performance.
* Remove the auto-add parameters and var since this are not expose to the rest of sd and they do not work on linux.
* Modify the tests to use a MementoWatch. The MementoWatch memorizes all the events that have been triggered by the ReadDirecotryCh
* Added a new filter that ensures that IN_MODIFY|IS_DIR events are not raised when the child is not watched.
Description of the Change
Fixes lp:907511
This branch allows the Windows Watch implementation to behave more like the one found on pyinotify. The way this is achieved is the following:
* Moved _subdirs var to be a dictionary that store a map between a child path and if it is watched or not. The reason is that child_paths are relative to self._path which means that we store less info. The boolean state if we are watching the path, if not, we ignore all the events from that path.
* When we start watching all child events are added to _subdirs are not being watch until sd explicitly asks it.
* When adding a child we add it children to the _subdirs. The are not thread or run issues because the sd will add the watches always in the main twisted thread which is the one used to process the event.
* Moved _ignored_paths to be a set for performance.
* Remove the auto-add parameters and var since this are not expose to the rest of sd and they do not work on linux.
* Modify the tests to use a MementoWatch. The MementoWatch memorizes all the events that have been triggered by the ReadDirecotryCh
* Added a new filter that ensures that IN_MODIFY|IS_DIR events are not raised when the child is not watched.
| Brian Curtin (brian.curtin) wrote : | # |
| Manuel de la Peña (mandel) wrote : | # |
> Here's a mini-review. I don't have my dev environment setup and don't know a
> lot of the context, plus I can't run it yet...but here's a few things I
> noticed.
>
> ######
> test_filesystem
>
> 80 + self.mask = fs.FILE_
> 81 + fs.FILE_
>
> Minor style thing, but I noticed in a big import (that was removed) that long
> lines were wrapped with parentheses instead of breaking with backslashes.
> Maybe do the same here? (Sorry, had to get my first nitpick cosmetic comment
> out of the way :)
It makes sense, I actually prefer to use () than \ since for me it seems cleaner. Will get that fix.
>
> =======
> filesystem_
>
> _get_partial_
>
> It's about 2-3x faster to take a slice of the last character and compare it
> rather than using path.endswith. This may not be as clean as using endswith,
> but figured it may be worth mentioning. Something like "if not path[-1] ==
> os.path.sep:"
>
On it! good catch!
> Other than that, this method looks good.
> ******
> + self.log.
>
> none existing > nonexistent
Fixing
> ******
> + def add_child_
>
> This new method looks fine to me.
> ******
> + """Add a new path to be watch.
>
> watch > watched
Fixing.
> ============
> os_helper.py
>
> native_listdir
>
> What is the purpose of having listdir and native_listdir if they are the same?
Really good question, and it is indeed a bug. os.listdir does return certain values, such as system folders, that we want to ignore. I should be using the wrapped method from os_helper.
Thx for the review!
- 1199. By Manuel de la Peña on 2012-01-17
-
Fixed code according to review.
| Alejandro J. Cura (alecu) wrote : | # |
The branch looks interesting.
A few comments:
----
941 + if any([file_
942 + for path, watched in self._subdirs.
The list comprehension used inside the "any" could be replaced by a generator expression just by dropping the [ and ]. With that change a new list won't have to be constructed, and the any will be able to finish faster by not checking the rest if any item matches the expression.
----
"we get no events, lets therefore ignore them" ->
"we get no events, let's therefore ignore them
----
"lets remove the path" ->
"let's remove the path"
----
There are many references to "manager._wdm[0].", and it looks a bit flaky.
I think that instead this line:
self.patch(fs, 'Watch', MementoWatch)
should be replaced by something like:
self.patch(fs, 'Watch', lambda *args, **kwargs: self.mementowat
and:
def mementowatch_
return self.memento_watch
----
"suddirs" -> "subdirs"
- 1200. By Manuel de la Peña on 2012-01-23
-
Fixed code according to review comments.
- 1201. By Manuel de la Peña on 2012-01-23
-
Fixed some stupidly broken tests.
- 1202. By Manuel de la Peña on 2012-01-23
-
Merged with trunk.
| Facundo Batista (facundo) wrote : | # |
There's a race condition, as we talked by IRC, when getting subdirs for the _subdirs internal structure, and the real directory/files creation in disk.
| Alejandro J. Cura (alecu) wrote : | # |
Line 132 seems to be missing a leading space:
130 + def mementowatch_
131 + """Factory that creates a new Watch for the tests."""
132 + kwargs[
133 + self.memento_watch = MementoWatch(*args, **kwargs)
134 + return self.memento_watch
- 1203. By Manuel de la Peña on 2012-01-26
-
Added a number of extra tests to ensure that we can work around the race condition.
- 1204. By Manuel de la Peña on 2012-01-30
-
Face palm.
- 1205. By Manuel de la Peña on 2012-01-30
-
Face palm.
- 1206. By Manuel de la Peña on 2012-01-31
-
Fixed the race condition to ensure that if a child is missing we add it to the _subdirs dict. Fixed old tests that relied in some implementation details.
- 1207. By Manuel de la Peña on 2012-01-31
-
Fixed broken listdir.
- 1208. By Manuel de la Peña on 2012-02-03
-
Simplified the number of if statements used to ensure that the child path is present.
- 1209. By Manuel de la Peña on 2012-02-03
-
Face palm.
- 1210. By Manuel de la Peña on 2012-02-03
-
Place the parent_dir definition closer to the use. Fix the deep test because a and A are the same on windows.
Unmerged revisions
- 1210. By Manuel de la Peña on 2012-02-03
-
Place the parent_dir definition closer to the use. Fix the deep test because a and A are the same on windows.
- 1209. By Manuel de la Peña on 2012-02-03
-
Face palm.
- 1208. By Manuel de la Peña on 2012-02-03
-
Simplified the number of if statements used to ensure that the child path is present.
- 1207. By Manuel de la Peña on 2012-01-31
-
Fixed broken listdir.
- 1206. By Manuel de la Peña on 2012-01-31
-
Fixed the race condition to ensure that if a child is missing we add it to the _subdirs dict. Fixed old tests that relied in some implementation details.
- 1205. By Manuel de la Peña on 2012-01-30
-
Face palm.
- 1204. By Manuel de la Peña on 2012-01-30
-
Face palm.
- 1203. By Manuel de la Peña on 2012-01-26
-
Added a number of extra tests to ensure that we can work around the race condition.
- 1202. By Manuel de la Peña on 2012-01-23
-
Merged with trunk.
- 1201. By Manuel de la Peña on 2012-01-23
-
Fixed some stupidly broken tests.


Here's a mini-review. I don't have my dev environment setup and don't know a lot of the context, plus I can't run it yet...but here's a few things I noticed.
###### _notifications. py
test_filesystem
80 + self.mask = fs.FILE_ NOTIFY_ CHANGE_ FILE_NAME | \ NOTIFY_ CHANGE_ DIR_NAME | \
81 + fs.FILE_
Minor style thing, but I noticed in a big import (that was removed) that long lines were wrapped with parentheses instead of breaking with backslashes. Maybe do the same here? (Sorry, had to get my first nitpick cosmetic comment out of the way :)
======= ======= ======= ====== notifications. py
filesystem_
_get_partial_ child_path_ dir
It's about 2-3x faster to take a slice of the last character and compare it rather than using path.endswith. This may not be as clean as using endswith, but figured it may be worth mentioning. Something like "if not path[-1] == os.path.sep:"
Other than that, this method looks good. warn('Adding watch to none existing path "%s"', path)
******
+ self.log.
none existing > nonexistent watch(self, path):
******
+ def add_child_
This new method looks fine to me.
******
+ """Add a new path to be watch.
watch > watched
============
os_helper.py
native_listdir
What is the purpose of having listdir and native_listdir if they are the same?