Merge lp:~mandel/ubuntuone-client/fix-rm-path into lp:ubuntuone-client

[Manuel de la Peña]

Improved the implementation of the rm_path method so that it is simpler.

Improved the implementation of the rm_path method so that it is simpler.

[lp:~mandel/ubuntuone-client/fix-rm-path]

1102. By Manuel de la Peña on 2011-07-28

Merged remove_watcher_q into fix-rm-path.

1103. By Manuel de la Peña on 2011-07-28

Merged remove_watcher_q into fix-rm-path.

1104. By Manuel de la Peña on 2011-07-28

Merged with previous branch.

1105. By Manuel de la Peña on 2011-07-28

Merged remove_watcher_q into fix-rm-path.

1106. By Manuel de la Peña on 2011-07-28

Merged remove_watcher_q into fix-rm-path.

[Guillermo Gonzalez (verterok)]

Looks ok, tests pass.

review: Approve

[Natalia Bidart (nataliabidart)]

Typo: test_undo_ignore_path_other_ignoed

Remove the "" from: self.log.debug('Adding exclude filter for "%r"', path)

review: Needs Fixing

[Natalia Bidart (nataliabidart)]

Also:
./tests/platform/windows/test_filesystem_notifications.py:
    632: local variable 'exclude_filter' is assigned to but never used

review: Needs Fixing

[lp:~mandel/ubuntuone-client/fix-rm-path]

1107. By Manuel de la Peña on 2011-07-29

Fixed some small typos and removed an unused var.

[Natalia Bidart (nataliabidart)]

All Clear!

review: Approve

=== modified file 'tests/platform/windows/test_filesystem_notifications.py'
--- tests/platform/windows/test_filesystem_notifications.py	2011-07-28 20:13:00 +0000
+++ tests/platform/windows/test_filesystem_notifications.py	2011-07-29 13:35:16 +0000
@@ -448,13 +448,129 @@
         self.assertEqual(0, len(events))
     test_open_dir_muted.skip = "we must rethink this test."
 
-
-class FakeEvent():
-    """A fake event."""
-
-    def __init__(self, path):
-        """Create an event with the given path."""
-        self.pathname = path
+    def test_ignore_path(self):
+        """Test that events from a path are ignored."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event)
+
+        path = u'\\\\?\\C:\\path'  # a valid windows path
+        child = u'child'
+        watch = Watch(1, path, None, True, fake_processor)
+        watch.ignore_path(os.path.join(path, child))
+        paths_to_ignore = []
+        for file_name in 'abcdef':
+            paths_to_ignore.append((1, os.path.join(child, file_name)))
+        # ensure that the watch is watching
+        watch._watching = True
+        watch._process_events(paths_to_ignore)
+        self.assertEqual(0, len(events),
+                         'All events should have been ignored.')
+
+    def test_not_ignore_path(self):
+        """Test that we do get the events when they do not match."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event)
+
+        path = u'\\\\?\\C:\\path'  # a valid windows path
+        child = u'child'
+        watch = Watch(1, path, None, True, fake_processor)
+        watch.ignore_path(os.path.join(path, child))
+        paths_not_to_ignore = []
+        for file_name in 'abcdef':
+            paths_not_to_ignore.append((1, os.path.join(
+                                                child + file_name, file_name)))
+        # ensure that the watch is watching
+        watch._watching = True
+        watch._process_events(paths_not_to_ignore)
+        self.assertEqual(len(paths_not_to_ignore), len(events),
+                         'No events should have been ignored.')
+
+    def test_mixed_ignore_path(self):
+        """Test that we do get the correct events."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event.pathname)
+
+        child = u'child'
+        path = u'\\\\?\\C:\\path\\'  # a valid windows path
+        watch = Watch(1, path, None, True, fake_processor)
+        watch.ignore_path(os.path.join(path, child))
+        paths_not_to_ignore = []
+        paths_to_ignore = []
+        expected_events = []
+        for file_name in 'abcdef':
+            valid = os.path.join(child + file_name, file_name)
+            paths_to_ignore.append((1, os.path.join(child, file_name)))
+            paths_not_to_ignore.append((1, valid))
+            expected_events.append(os.path.join('C:\\path', valid))
+        # ensure that the watch is watching
+        watch._watching = True
+        watch._process_events(paths_not_to_ignore)
+        self.assertEqual(len(paths_not_to_ignore), len(events),
+                         'Wrong number of events ignored.')
+        self.assertTrue(all([event in expected_events for event in events]),
+                        'Paths ignored that should have not been ignored.')
+
+    def test_undo_ignore_path_ignored(self):
+        """Test that we do deal with events from and old ignored path."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event)
+
+        path = u'\\\\?\\C:\\path'  # a valid windows path
+        child = u'child'
+        watch = Watch(1, path, None, True, fake_processor)
+        watch.ignore_path(os.path.join(path, child))
+        watch.remove_ignored_path(os.path.join(path, child))
+        paths_not_to_ignore = []
+        for file_name in 'abcdef':
+            paths_not_to_ignore.append((1, os.path.join(child, file_name)))
+        # ensure that the watch is watching
+        watch._watching = True
+        watch._process_events(paths_not_to_ignore)
+        self.assertEqual(len(paths_not_to_ignore), len(events),
+                         'All events should have been accepted.')
+
+    def test_undo_ignore_path_other_ignored(self):
+        """Test that we can undo and the other path is ignored."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event.pathname)
+
+        path = u'\\\\?\\C:\\path'  # a valid windows path
+        child_a = u'childa'
+        child_b = u'childb'
+        watch = Watch(1, path, None, True, fake_processor)
+        watch.ignore_path(os.path.join(path, child_a))
+        watch.ignore_path(os.path.join(path, child_b))
+        watch.remove_ignored_path(os.path.join(path, child_a))
+        paths_to_ignore = []
+        paths_not_to_ignore = []
+        expected_events = []
+        for file_name in 'abcdef':
+            paths_to_ignore.append((1, os.path.join(child_b, file_name)))
+            valid = os.path.join(child_a, file_name)
+            paths_not_to_ignore.append((1, valid))
+            expected_events.append(os.path.join('C:\\path', valid))
+        # ensure that the watch is watching
+        watch._watching = True
+        watch._process_events(paths_not_to_ignore)
+        self.assertEqual(len(paths_not_to_ignore), len(events),
+                         'All events should have been accepted.')
+        self.assertTrue(all([event in expected_events for event in events]),
+                        'Paths ignored that should have not been ignored.')
 
 
 class TestWatchManager(BaseTwistedTestCase):
@@ -513,15 +629,12 @@
 
         mask = 'bit_mask'
         auto_add = True
-        exclude_filter = lambda x: False
-        self.manager.add_watch(self.path, mask, auto_add=auto_add,
-                               exclude_filter=exclude_filter)
+        self.manager.add_watch(self.path, mask, auto_add=auto_add)
         self.assertEqual(1, len(self.manager._wdm))
         self.assertTrue(self.was_called, 'The watch start was not called.')
-        self.assertEqual(self.path, self.manager._wdm[0]._path)
+        self.assertEqual(self.path + os.path.sep, self.manager._wdm[0]._path)
         self.assertEqual(mask, self.manager._wdm[0]._mask)
         self.assertEqual(auto_add, self.manager._wdm[0]._auto_add)
-        self.assertEqual(exclude_filter, self.manager._wdm[0].exclude_filter)
 
     def test_update_present_watch(self):
         """Test the update of a present watch."""
@@ -531,7 +644,7 @@
 
     def test_get_watch_present_wd(self):
         """Test that the correct path is returned."""
-        self.assertEqual(self.path, self.manager.get_path(1))
+        self.assertEqual(self.path + os.path.sep, self.manager.get_path(1))
 
     def test_get_watch_missing_wd(self):
         """Test that the correct path is returned."""
@@ -554,16 +667,35 @@
 
     def test_rm_root_path(self):
         """Test the removal of a root path."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event.pathname)
+
+        self.watch._processor = fake_processor
         self.manager.rm_path(self.path)
         self.assertEqual(self.watch, self.manager._wdm.get(1))
-        event = FakeEvent(self.path)
-        self.assertTrue(self.watch.exclude_filter(event))
+        self.watch._watching = True
+        self.watch._process_events([(1, os.path.join(self.path, 'test'))])
+        self.assertEqual(0, len(events))
 
     def test_rm_child_path(self):
         """Test the removal of a child path."""
+        events = []
+
+        def fake_processor(event):
+            """Memorize the processed events."""
+            events.append(event.pathname)
+
+        self.watch._processor = fake_processor
         child = os.path.join(self.path, u'child')
-        event = FakeEvent(child)
         self.manager.rm_path(child)
         self.assertEqual(self.watch, self.manager._wdm[1])
-        # assert that the path is ignored
-        self.assertTrue(self.watch.exclude_filter(event))
+        # assert that the correct event is ignored
+        self.watch._watching = True
+        self.watch._process_events([(1, os.path.join('child', 'test'))])
+        self.assertEqual(0, len(events))
+        # assert that other events are not ignored
+        self.watch._process_events([(1, 'test')])
+        self.assertEqual(1, len(events))

=== modified file 'ubuntuone/platform/windows/filesystem_notifications.py'
--- ubuntuone/platform/windows/filesystem_notifications.py	2011-07-28 19:04:19 +0000
+++ ubuntuone/platform/windows/filesystem_notifications.py	2011-07-29 13:35:16 +0000
@@ -122,7 +122,7 @@
     """Implement the same functions as pyinotify.Watch."""
 
     def __init__(self, watch_descriptor, path, mask, auto_add, processor,
-        exclude_filter=None, buf_size=8192):
+        buf_size=8192):
         super(Watch, self).__init__()
         self.log = logging.getLogger('ubuntuone.SyncDaemon.platform.windows.' +
             'filesystem_notifications.Watch')
@@ -135,7 +135,7 @@
         self._watching = False
         self._descriptor = watch_descriptor
         self._auto_add = auto_add
-        self.exclude_filter = exclude_filter
+        self._ignore_paths = []
         self._cookie = None
         self._source_pathname = None
         self._process_thread = None
@@ -144,6 +144,8 @@
         self._subdirs = []
         # ensure that we work with an abspath and that we can deal with
         # long paths over 260 chars.
+        if not path.endswith(os.path.sep):
+            path += os.path.sep
         self._path = os.path.abspath(path)
         self._mask = mask
         # this deferred is fired when the watch has started monitoring
@@ -152,8 +154,7 @@
 
     def _is_excluded(self, event):
         """Return if an event is ignored."""
-        return (self.exclude_filter and self.exclude_filter(event)) or\
-            event.mask == IN_OPEN | IN_ISDIR
+        return  event.mask == IN_OPEN | IN_ISDIR
 
     # XXX: confirm is using this decorator is correct!!! (nessita)
     @is_valid_windows_path(path_indexes=[1])
@@ -184,6 +185,9 @@
         # we transform the events to be the same as the one in pyinotify
         # and then use the proc_fun
         for action, file_name in events:
+            if any([file_name.startswith(path)
+                        for path in self._ignore_paths]):
+                continue
             # map the windows events to the pyinotify ones, tis is dirty but
             # makes the multiplatform better, linux was first :P
             syncdaemon_path = get_syncdaemon_valid_path(
@@ -279,6 +283,25 @@
 
         CloseHandle(handle)
 
+    @is_valid_windows_path(path_indexes=[1])
+    def ignore_path(self, path):
+        """Add the path of the events to ignore."""
+        if not path.endswith(os.path.sep):
+            path += os.path.sep
+        if path.startswith(self._path):
+            path = path[len(self._path):]
+            self._ignore_paths.append(path)
+
+    @is_valid_windows_path(path_indexes=[1])
+    def remove_ignored_path(self, path):
+        """Reaccept path."""
+        if not path.endswith(os.path.sep):
+            path += os.path.sep
+        if path.startswith(self._path):
+            path = path[len(self._path):]
+            if path in self._ignore_paths:
+                self._ignore_paths.remove(path)
+
     def start_watching(self):
         """Tell the watch to start processing events."""
         for current_child in os.listdir(self._path):
@@ -322,7 +345,7 @@
 
     """
 
-    def __init__(self, processor, exclude_filter=lambda path: False):
+    def __init__(self, processor):
         """Init the manager to keep trak of the different watches."""
         super(WatchManager, self).__init__()
         self._processor = processor
@@ -331,7 +354,6 @@
         self.log.setLevel(TRACE)
         self._wdm = {}
         self._wd_count = 0
-        self._exclude_filter = exclude_filter
         self._ignored_paths = []
 
     def stop(self):
@@ -356,20 +378,19 @@
             logging.error(str(e))
 
     def _add_single_watch(self, path, mask, auto_add=False,
-        quiet=True, exclude_filter=None):
+        quiet=True):
         if path in self._ignored_paths:
             # simply removed it from the filter
             self._ignored_paths.remove(path)
             return
         # we need to add a new watch
-        self.log.debug('add_single_watch(%s, %s, %s, %s, %s)', path, mask,
-            auto_add, quiet, exclude_filter)
+        self.log.debug('add_single_watch(%s, %s, %s, %s)', path, mask,
+            auto_add, quiet)
 
         # adjust the number of threads based on the UDFs watched
         reactor.suggestThreadPoolSize(THREADPOOL_MAX + self._wd_count + 1)
         self._wdm[self._wd_count] = Watch(self._wd_count, path,
-                                          mask, auto_add, self._processor,
-                                          exclude_filter=exclude_filter)
+                                          mask, auto_add, self._processor)
         d = self._wdm[self._wd_count].start_watching()
         self._wd_count += 1
         self.log.debug('Watch count increased to %s', self._wd_count)
@@ -377,7 +398,7 @@
 
     @is_valid_windows_path(path_indexes=[1])
     def add_watch(self, path, mask, auto_add=False,
-        quiet=True, exclude_filter=None):
+        quiet=True):
         """Add a new path tp be watch.
 
         The method will ensure that the path is not already present.
@@ -387,8 +408,7 @@
             return defer.fail(e)
         if self.get_wd(path) is None:
             self.log.debug('Adding single watch on %r', path)
-            return self._add_single_watch(path, mask, auto_add, quiet,
-                                          exclude_filter)
+            return self._add_single_watch(path, mask, auto_add, quiet)
         else:
             self.log.debug('Watch already exists on %r', path)
             return defer.succeed(False)
@@ -400,6 +420,8 @@
     @is_valid_windows_path(path_indexes=[1])
     def get_wd(self, path):
         """Return the watcher that is used to watch the given path."""
+        if not path.endswith(os.path.sep):
+            path = path + os.path.sep
         for current_wd in self._wdm:
             watch_path = self._wdm[current_wd].path
             if ((watch_path == path or (
@@ -429,23 +451,8 @@
         """Remove a watch to the given path."""
         wd = self.get_wd(path)
         if wd is not None:
-            self.log.debug('Adding exclude filter for "%s"', path)
-            # we have a watch that contains the path as a child path
-            if not path in self._ignored_paths:
-                self._ignored_paths.append(path)
-
-            def ignore_path(event):
-                """Ignore an event if it has a given path."""
-                for path in self._ignored_paths:
-                    if path in event.pathname:
-                        return True
-                return False
-
-            # FIXME: This assumes that we do not have other function
-            # which in our usecase is correct, but what is we move this
-            # to other project?!? Maybe using the manager
-            # exclude_filter is better
-            self._wdm[wd].exclude_filter = ignore_path
+            self.log.debug('Adding exclude filter for %r', path)
+            self._wdm[wd].ignore_path(path)
 
 
 class NotifyProcessor(ProcessEvent):