Merge lp:~facundo/ubuntuone-client/delete-to-trash into lp:ubuntuone-client

[Facundo Batista]

Move to trash instead of plain deletion (LP: #690673)

Move to trash instead of plain deletion

For this, os_helper grown a new "move_to_trash" operation (only the linux part, for the windows version see #747741).

FSM calls this new method instead of just removing the file. The only issue was that for tree deletion the code relied on shutil.rmtree's on_error calls to make the directories RW, and we couldn't do that (because if we need to remove a whole tree, that whole tree must go to trash, not files and directories individually).

So, I had to refactor how EnableShareWrite works, to support a "recursive" option.

Finally, note that this new behaviour is configurable. However, to make it work, I had to fix the path in config.py, to take also the configuration from the branch (this was broken somewhen, you only notice that when a new parameter is added).

Tests included for everything.

[Lucio Torre (lucio.torre)]

=== modified file 'data/syncdaemon.conf'
+use_trash.help = Send the files and folders to XDG Trash folder instead of
+ removing them permanently.

syncdaemon.conf is not platform dependent and i am not sure the trashbin in windows or other OSes is done/managed or anything by xdg.

we should get guillos coment on this one as well.

review: Approve

[lp:~facundo/ubuntuone-client/delete-to-trash]

936. By Facundo Batista on 2011-04-04

Better help message

[Guillermo Gonzalez (verterok)]

+1

review: Approve

=== modified file 'data/syncdaemon.conf'
--- data/syncdaemon.conf	2011-02-09 08:23:04 +0000
+++ data/syncdaemon.conf	2011-04-04 18:56:34 +0000
@@ -83,6 +83,12 @@
                  \A.*\.sw[nopx]\Z
                  \A.*\.swpx\Z
                  \A\..*\.tmp\Z
+ 
+use_trash.default = True
+use_trash.parser = bool
+use_trash.action = store_true
+use_trash.help = Send the files and folders to Trash folder instead of
+    removing them permanently.
 
 
 [notifications]

=== modified file 'tests/platform/linux/test_os_helper.py'
--- tests/platform/linux/test_os_helper.py	2011-01-18 15:24:38 +0000
+++ tests/platform/linux/test_os_helper.py	2011-04-04 18:56:34 +0000
@@ -19,14 +19,20 @@
 """Linux specific tests for the platform module."""
 
 import errno
+import logging
 import os
 
+import gio
+
+from ubuntuone.devtools.handlers import MementoHandler
+
 from contrib.testing.testcase import BaseTwistedTestCase
 from ubuntuone.platform import (
     access,
     allow_writes,
     listdir,
     make_dir,
+    move_to_trash,
     open_file,
     path_exists,
     remove_dir,
@@ -39,17 +45,37 @@
     stat_path,
 )
 
+class FakeGIOFile(object):
+    """Fake File for gio."""
+
+    _bad_trash_call = None
+
+    def __init__(self, path):
+        pass
+
+    def trash(self):
+        """Fake trash call."""
+        return self._bad_trash_call
+
 
 class OSWrapperTests(BaseTwistedTestCase):
     """Tests for os wrapper functions."""
 
     def setUp(self):
+        """Set up."""
         BaseTwistedTestCase.setUp(self)
         self.basedir = self.mktemp('test_root')
         self.testfile = os.path.join(self.basedir, "test_file")
         open(self.testfile, 'w').close()
 
+        self.handler = MementoHandler()
+        self.handler.setLevel(logging.DEBUG)
+        self._logger = logging.getLogger('ubuntuone.SyncDaemon')
+        self._logger.addHandler(self.handler)
+
     def tearDown(self):
+        """Tear down."""
+        self._logger.removeHandler(self.handler)
         self.rmtree(self.basedir)
         BaseTwistedTestCase.tearDown(self)
 
@@ -252,3 +278,82 @@
         """The dir is not there."""
         self.rmtree(self.basedir)
         self.assertFalse(path_exists(self.basedir))
+
+    def test_movetotrash_file_ok(self):
+        """Move a file to trash ok.
+
+        Just check it was removed because can't monkeypatch gio.File.trash
+        to see that that was actually called.  Note that the gio
+        infrastructure is used is obvious after the other trash tests here.
+        """
+        path = os.path.join(self.basedir, 'foo')
+        open(path, 'w').close()
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+
+    def test_movetotrash_dir_ok(self):
+        """Move a dir to trash ok.
+
+        Just check it was removed because can't monkeypatch gio.File.trash
+        to see that that was actually called.  Note that the gio
+        infrastructure is used is obvious after the other trash tests here.
+        """
+        path = os.path.join(self.basedir, 'foo')
+        os.mkdir(path)
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+
+    def test_movetotrash_file_bad(self):
+        """Something bad happen when moving to trash, removed anyway."""
+        FakeGIOFile._bad_trash_call = False   # error
+        self.patch(gio, "File", FakeGIOFile)
+        path = os.path.join(self.basedir, 'foo')
+        open(path, 'w').close()
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+        self.assertTrue(self.handler.check_warning("Problems moving to trash!",
+                                                   "Removing anyway", "foo"))
+
+    def test_movetotrash_dir_bad(self):
+        """Something bad happen when moving to trash, removed anyway."""
+        FakeGIOFile._bad_trash_call = False   # error
+        self.patch(gio, "File", FakeGIOFile)
+        path = os.path.join(self.basedir, 'foo')
+        os.mkdir(path)
+        open(os.path.join(path, 'file inside directory'), 'w').close()
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+        self.assertTrue(self.handler.check_warning("Problems moving to trash!",
+                                                   "Removing anyway", "foo"))
+
+
+    def test_movetotrash_file_systemnotcapable(self):
+        """The system is not capable of moving into trash."""
+        FakeGIOFile._bad_trash_call = gio.ERROR_NOT_SUPPORTED
+        self.patch(gio, "File", FakeGIOFile)
+        path = os.path.join(self.basedir, 'foo')
+        open(path, 'w').close()
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+        self.assertTrue(self.handler.check_warning("Problems moving to trash!",
+                                                   "Removing anyway", "foo",
+                                                   "ERROR_NOT_SUPPORTED"))
+
+    def test_movetotrash_dir_systemnotcapable(self):
+        """The system is not capable of moving into trash."""
+        FakeGIOFile._bad_trash_call = gio.ERROR_NOT_SUPPORTED
+        self.patch(gio, "File", FakeGIOFile)
+        path = os.path.join(self.basedir, 'foo')
+        os.mkdir(path)
+        open(os.path.join(path, 'file inside directory'), 'w').close()
+        move_to_trash(path)
+        self.assertFalse(os.path.exists(path))
+        self.assertTrue(self.handler.check_warning("Problems moving to trash!",
+                                                   "Removing anyway", "foo",
+                                                   "ERROR_NOT_SUPPORTED"))
+
+    def test_movetotrash_notthere(self):
+        """Try to move to trash something that is not there."""
+        path = os.path.join(self.basedir, 'notthere')
+        e = self.assertRaises(OSError, move_to_trash, path)
+        self.assertEqual(e.errno, errno.ENOENT)

=== modified file 'tests/syncdaemon/test_config.py'
--- tests/syncdaemon/test_config.py	2011-02-09 08:23:04 +0000
+++ tests/syncdaemon/test_config.py	2011-04-04 18:56:34 +0000
@@ -585,3 +585,8 @@
         self.cp.parse_all()
         self.assertEquals(self.cp.get('__main__', 'ignore').value,
                           ['.*\\.pyc', '.*\\.sw[opnx]'])
+
+    def test_use_trash_default(self):
+        """Test default configuration for use_trash."""
+        self.cp.parse_all()
+        self.assertEqual(self.cp.get('__main__', 'use_trash').value, True)

=== modified file 'tests/syncdaemon/test_fsm.py'
--- tests/syncdaemon/test_fsm.py	2011-03-12 00:11:44 +0000
+++ tests/syncdaemon/test_fsm.py	2011-04-04 18:56:34 +0000
@@ -27,6 +27,7 @@
 import unittest
 
 from twisted.internet import defer
+from twisted.trial.unittest import TestCase as TwistedTestCase
 
 from contrib.testing.testcase import (
     BaseTwistedTestCase,
@@ -37,6 +38,7 @@
 
 from ubuntuone.devtools.handlers import MementoHandler
 from ubuntuone.syncdaemon.filesystem_manager import (
+    DirectoryNotRemovable,
     EnableShareWrite,
     FileSystemManager,
     InconsistencyError,
@@ -45,6 +47,7 @@
     TrashTritcaskShelf,
     TRASH_ROW_TYPE,
 )
+from ubuntuone.syncdaemon import filesystem_manager, config
 from ubuntuone.syncdaemon.file_shelf import FileShelf
 from ubuntuone.syncdaemon.tritcask import Tritcask
 from ubuntuone.syncdaemon.event_queue import EventQueue
@@ -69,13 +72,12 @@
     defer.returnValue(share)
 
 
-class FSMTestCase(unittest.TestCase):
+class FSMTestCase(TwistedTestCase):
     """ Base test case for FSM """
 
     @defer.inlineCallbacks
     def setUp(self):
         """ Setup the test """
-        unittest.TestCase.setUp(self)
         try:
             os.mkdir(TESTS_DIR)
         except OSError:
@@ -96,6 +98,7 @@
         self.fsm.register_eq(self.eq)
         self.share = yield self.create_share('share', 'share_name')
         self.share_path = self.share.path
+        config.get_user_config().set_use_trash(True)
 
         # add a in-memory logger handler
         self.handler = MementoHandler()
@@ -1164,7 +1167,7 @@
         self.assertEqual(mdobj.baz, "baz1")
 
         # test using uuid
-        self.assertRaises(ValueError, self.fsm.set_by_node_id, None, "sh", f=3)
+        self.assertRaises(ValueError, self.fsm.set_by_node_id, None, "sh", j=3)
         self.fsm.set_by_node_id("uuid", "share", foo="foo2")
         self.fsm.set_by_node_id("uuid", "share", bar="bar2", baz="baz2")
         mdobj = self.fsm.get_by_node_id("share", "uuid")
@@ -2175,8 +2178,8 @@
         mdobj = self.fsm.get_by_path(otherfile)
         self.assertEqual(mdobj.path, "pa")
 
-    def test_delete_file(self):
-        """Test that a file is deleted."""
+    def _delete_file(self):
+        """Helper to test that a file is deleted."""
         testfile = os.path.join(self.share_path, "path")
         open(testfile, "w").close()
         mdid = self.fsm.create(testfile, "share")
@@ -2187,8 +2190,34 @@
         self.assertFalse(os.path.exists(testfile))
         self.assert_no_metadata(mdid, testfile, "share", "uuid")
 
-    def test_delete_dir(self):
-        """Test that an empty dir is deleted."""
+    def test_delete_file_directly(self):
+        """Really delete the file."""
+        config.get_user_config().set_use_trash(False)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = filesystem_manager.remove_file
+        self.patch(filesystem_manager, 'remove_file',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_file()
+        self.assertTrue(called)
+
+    def test_delete_file_trash(self):
+        """Move the file to trash."""
+        config.get_user_config().set_use_trash(True)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = filesystem_manager.move_to_trash
+        self.patch(filesystem_manager, 'move_to_trash',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_file()
+        self.assertTrue(called)
+
+    def _delete_dir(self):
+        """Helper to test that an empty dir is deleted."""
         testdir = os.path.join(self.share.path, "path")
         os.mkdir(testdir)
         mdid = self.fsm.create(testdir, "share", is_dir=True)
@@ -2207,8 +2236,34 @@
         self.assertFalse(os.path.exists(testdir))
         self.assert_no_metadata(mdid, testdir, "share", "uuid")
 
-    def test_delete_dir_when_non_empty_and_no_modifications(self):
-        """Test that a dir is deleted, when is not empty and unmodified."""
+    def test_delete_dir_directly(self):
+        """Really delete the dir."""
+        config.get_user_config().set_use_trash(False)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = filesystem_manager.remove_dir
+        self.patch(filesystem_manager, 'remove_dir',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_dir()
+        self.assertTrue(called)
+
+    def test_delete_dir_trash(self):
+        """Move the dir to trash."""
+        config.get_user_config().set_use_trash(True)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = filesystem_manager.move_to_trash
+        self.patch(filesystem_manager, 'move_to_trash',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_dir()
+        self.assertTrue(called)
+
+    def _delete_dir_when_non_empty_and_no_modifications(self):
+        """Helper to test that a dir is deleted, non empty but ok to clean."""
         local_dir = os.path.join(self.root_dir, "foo")
         os.mkdir(local_dir)
         mdid = self.fsm.create(local_dir, "", is_dir=True)
@@ -2230,6 +2285,32 @@
         self.assertFalse(os.path.exists(local_dir))
         self.assert_no_metadata(mdid, local_dir, "", "uuid")
 
+    def test_delete_nonempty_cleanable_dir_directly(self):
+        """Really delete the non empty but cleanable dir."""
+        config.get_user_config().set_use_trash(False)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = shutil.rmtree
+        self.patch(shutil, 'rmtree',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_dir_when_non_empty_and_no_modifications()
+        self.assertTrue(called)
+
+    def test_delete_nonempty_cleanable_dir_trash(self):
+        """Move the non empty but cleanable dir to trash."""
+        config.get_user_config().set_use_trash(True)
+
+        # check it was sent to trash, not just deleted
+        called = []
+        orig_call = filesystem_manager.move_to_trash
+        self.patch(filesystem_manager, 'move_to_trash',
+                   lambda path: called.append(True) or orig_call(path))
+
+        self._delete_dir_when_non_empty_and_no_modifications()
+        self.assertTrue(called)
+
     def test_delete_dir_when_non_empty_and_modifications_prior_delete(self):
         """Test that a dir is deleted, when is not empty and modified."""
         local_dir = os.path.join(self.root_dir, "foo")
@@ -2245,7 +2326,8 @@
 
         assert len(os.listdir(local_dir)) > 0 # local_dir is not empty
         assert self.fsm.changed(path=local_file) == self.fsm.CHANGED_LOCAL
-        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+        self.assertRaises(DirectoryNotRemovable,
+                          self.fsm.delete_file, local_dir)
 
     def test_delete_dir_when_non_empty_and_prior_conflict_on_file(self):
         """Test that a dir is not deleted, when there is a conflicted file."""
@@ -2260,7 +2342,8 @@
         open(local_file, 'w').close() # touch bar.txt.u1conflict
 
         assert not local_file in self.fsm._idx_path
-        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+        self.assertRaises(DirectoryNotRemovable,
+                          self.fsm.delete_file, local_dir)
 
         infos = [record.message for record in self.handler.records
                  if record.levelname == 'INFO']
@@ -2280,7 +2363,8 @@
         os.mkdir(local_subdir)
 
         assert not local_subdir in self.fsm._idx_path
-        self.assertRaises(OSError, self.fsm.delete_file, local_dir)
+        self.assertRaises(DirectoryNotRemovable,
+                          self.fsm.delete_file, local_dir)
 
         infos = [record.message for record in self.handler.records
                  if record.levelname == 'INFO']
@@ -3054,9 +3138,12 @@
         self.fsm.upload_finished(mdid, self.fsm.get_by_mdid(mdid).local_hash)
         self.assertTrue(os.path.exists(testdir))
         self.assertTrue(os.path.exists(testfile))
-        # make the dir read-only
+
+        # make the dir read-only, the error should be logged
         os.chmod(testdir, 0555)
-        self.assertRaises(OSError, self.fsm.delete_file, testdir)
+        self.fsm.delete_file(testdir)
+        self.assertTrue(self.handler.check_warning("OSError", testdir,
+                                                   "when trying to remove"))
         self.assertTrue(os.path.exists(testdir))
         self.assertTrue(os.path.exists(testfile))
 
@@ -3118,7 +3205,7 @@
 
     @defer.inlineCallbacks
     def test_file_rw_share(self):
-        """ Test that the creation of a file using fsm, works on a rw-share."""
+        """Test that the creation of a file using fsm, works on a rw-share."""
         self.share = yield self.create_share('ro_share', 'ro_share_name')
         testfile = os.path.join(self.share.path, "a_file")
         self.fsm.create(testfile, self.share.volume_id, is_dir=False)
@@ -3165,7 +3252,7 @@
         self.share_ro_path = self.share_ro.path
 
     def test_write_in_ro_share(self):
-        """test the EnableShareWrite context manager in a ro share"""
+        """Test the EnableShareWrite context manager in a ro share."""
         path = os.path.join(self.share_ro_path, 'foo', 'a_file_in_a_ro_share')
         data = 'yes I can write!'
         can_write_parent = os.access(os.path.dirname(self.share_ro_path),
@@ -3174,16 +3261,15 @@
             self.assertTrue(enabled.ro)
             with open(path, 'w') as f:
                 f.write(data)
-        self.assertEquals(data, open(path, 'r').read())
+        self.assertEqual(data, open(path, 'r').read())
         self.assertFalse(os.access(self.share_ro_path, os.W_OK))
         # check that the parent permissions are ok
-        self.assertEquals(can_write_parent,
-                          os.access(os.path.dirname(self.share_ro_path),
-                                    os.W_OK))
+        self.assertEqual(can_write_parent,
+                         os.access(os.path.dirname(self.share_ro_path),
+                                   os.W_OK))
         # fail to write directly in the share
         self.assertRaises(IOError, open, path, 'w')
 
-
     def test_write_in_rw_share(self):
         """test the EnableShareWrite context manager in a rw share"""
         path = os.path.join(self.share_path, 'a_file_in_a_rw_share')
@@ -3204,7 +3290,6 @@
     @defer.inlineCallbacks
     def setUp(self):
         """ Setup the test """
-        unittest.TestCase.setUp(self)
         try:
             os.mkdir(TESTS_DIR)
         except OSError:
@@ -3885,11 +3970,3 @@
         """Tear down."""
         self.db.shutdown()
         self.rmtree(self.trash_dir)
-
-
-def test_suite():
-    # pylint: disable-msg=C0111
-    return unittest.TestLoader().loadTestsFromName(__name__)
-
-if __name__ == "__main__":
-    unittest.main()

=== modified file 'ubuntuone/platform/linux/__init__.py'
--- ubuntuone/platform/linux/__init__.py	2011-02-09 16:24:05 +0000
+++ ubuntuone/platform/linux/__init__.py	2011-04-04 18:56:34 +0000
@@ -36,6 +36,7 @@
     remove_file,
     rename,
     make_link,
+    move_to_trash,
     set_dir_readonly,
     set_dir_readwrite,
     set_no_rights,
@@ -46,7 +47,7 @@
 from ubuntuone.platform.linux.vm_helper import (
     create_shares_link,
     get_share_path,
-    get_udf_path_name, 
+    get_udf_path_name,
     get_udf_path,
     VMMetadataUpgraderMixIn
 )

=== modified file 'ubuntuone/platform/linux/os_helper.py'
--- ubuntuone/platform/linux/os_helper.py	2011-01-14 02:50:32 +0000
+++ ubuntuone/platform/linux/os_helper.py	2011-04-04 18:56:34 +0000
@@ -22,13 +22,20 @@
 to support the linux platform.
 """
 
+import errno
+import logging
 import os
+import shutil
 import stat
 
+import gio
+
 from contextlib import contextmanager
 
 platform = "linux2"
 
+logger = logging.getLogger('ubuntuone.SyncDaemon')
+
 
 def set_no_rights(path):
     """Remove all rights from the file."""
@@ -101,3 +108,22 @@
     """Return stat info about a path."""
     return os.lstat(path)
 
+def move_to_trash(path):
+    """Move the file or dir to trash.
+
+    If had any error, or the system can't do it, just remove it.
+    """
+    try:
+        return_code = gio.File(path).trash()
+    except gio.Error:
+        exc = OSError()
+        exc.errno = errno.ENOENT
+        raise exc
+
+    if not return_code or return_code == gio.ERROR_NOT_SUPPORTED:
+        logger.warning("Problems moving to trash! (%s) Removing anyway: %r",
+                       return_code, path)
+        if os.path.isdir(path):
+            shutil.rmtree(path)
+        else:
+            os.remove(path)

=== modified file 'ubuntuone/syncdaemon/config.py'
--- ubuntuone/syncdaemon/config.py	2011-02-09 08:23:04 +0000
+++ ubuntuone/syncdaemon/config.py	2011-04-04 18:56:34 +0000
@@ -147,9 +147,15 @@
     # reverse the list as load_config_paths returns the user dir first
     config_files.reverse()
     # if we are running from a branch, get the config files from it too
-    config_file = os.path.join('data', CONFIG_FILE)
-    if os.path.exists(config_file):
-        config_files.append(config_file)
+    try:
+        from bzrlib.workingtree import WorkingTree
+    except ImportError:
+        pass   # surely not inside a branch
+    else:
+        rootdir = WorkingTree.open_containing(__file__)[0].basedir
+        config_file = os.path.join(rootdir, 'data', CONFIG_FILE)
+        if os.path.exists(config_file):
+            config_files.append(config_file)
 
     config_logs = os.path.join('data', CONFIG_LOGS)
     if os.path.exists(config_logs):
@@ -365,6 +371,14 @@
     def get_autoconnect(self):
         return self.get_parsed(MAIN, 'autoconnect')
 
+    @requires_section(MAIN)
+    def get_use_trash(self):
+        return self.get_parsed(MAIN, 'use_trash')
+
+    @requires_section(MAIN)
+    def set_use_trash(self, enabled):
+        self.set(MAIN, 'use_trash', str(enabled))
+
     @requires_section(NOTIFICATIONS)
     def set_show_all_notifications(self, enabled):
         self.set(NOTIFICATIONS, 'show_all_notifications', str(enabled))

=== modified file 'ubuntuone/syncdaemon/filesystem_manager.py'
--- ubuntuone/syncdaemon/filesystem_manager.py	2011-03-12 00:11:44 +0000
+++ ubuntuone/syncdaemon/filesystem_manager.py	2011-04-04 18:56:34 +0000
@@ -29,13 +29,14 @@
 import stat
 import uuid
 
-from ubuntuone.syncdaemon import file_shelf
+from ubuntuone.syncdaemon import file_shelf, config
 from ubuntuone.syncdaemon.volume_manager import VolumeDoesNotExist
 from ubuntuone.syncdaemon.interfaces import IMarker
 from ubuntuone.syncdaemon.marker import MDMarker
 from ubuntuone.syncdaemon.tritcask import TritcaskShelf
 from ubuntuone.platform import (
     make_dir,
+    move_to_trash,
     open_file,
     path_exists,
     remove_dir,
@@ -151,13 +152,19 @@
 is_forbidden = set("info path node_id share_id is_dir".split()
                   ).intersection
 
+
 class InconsistencyError(Exception):
     """Inconsistency between internal records and filesystem itself."""
 
+
 class Despair(Exception):
     """This should never happen, we're in an impossible condition!"""
 
 
+class DirectoryNotRemovable(Exception):
+    """The directory can not be emptied to delete."""
+
+
 class _MDObject(object):
     """Wrapper around MD dict."""
     def __init__(self, **mdobj):
@@ -339,6 +346,9 @@
             load_method = getattr(self, "_load_metadata_%s" % md_version)
             load_method(md_version)
 
+        # load some config
+        self.user_config = config.get_user_config()
+
         logger("initialized: idx_path: %s, idx_node_id: %s, shares: %s",
                len(self._idx_path), len(self._idx_node_id), len(self.shares))
 
@@ -874,21 +884,17 @@
             del self._idx_node_id[(mdobj["share_id"], mdobj["node_id"])]
         del self.fs[mdid]
 
-    def _delete_dir_tree(self, path, share_id):
-        """Helper function to recursively remove a non-empty directory.
-
-        Remove the dir if there aren't local changes, and return True.
-        If there are, return False.
-
-        Notifications are disabled for every sub-path within path.
-
+    def _delete_dir_tree(self, path):
+        """Tell if it's ok to delete a dir tree.
+
+        Raise an exception if the directory can not be removed.
         """
         # check metadata to see if any node in LOCAL
         subtree = self.get_paths_starting_with(path, include_base=False)
         for p, is_dir in subtree:
             if self.changed(path=p) == self.CHANGED_LOCAL:
                 logger("Conflicting dir on remove because %r is local", p)
-                return False
+                raise DirectoryNotRemovable()
 
         # check disk searching for previous conflicts
         for (dirpath, dirnames, filenames) in os.walk(path):
@@ -896,29 +902,9 @@
                 if fname.endswith(self.CONFLICT_SUFFIX):
                     logger("Conflicting dir on remove because of previous "
                            "conflict on: %r", os.path.join(dirpath, fname))
-                    return False
-
-        # all green
-        for p, is_dir in subtree:
-            filter_name = is_dir and "FS_DIR_DELETE" or "FS_FILE_DELETE"
-            self.eq.add_to_mute_filter(filter_name, path=p)
-            self.delete_metadata(p)
-
-        # get the share and if it's read-only
-        share = self._get_share(share_id)
-        is_ro_share = not share.can_write()
-        def handle_ro_share(func, subpath, exc_info):
-            """Handle rmtree EACCES error for ro-shares."""
-            if exc_info[0] == OSError and exc_info[1].errno == errno.EACCES \
-               and is_ro_share:
-                # use EnableShareWrite directly to avoid a lot of share
-                # lookups.
-                with EnableShareWrite(share, subpath):
-                    func(subpath)
-            else:
-                raise
-        shutil.rmtree(path, onerror=handle_ro_share)
-        return True
+                    raise DirectoryNotRemovable()
+
+        return subtree
 
     def delete_file(self, path):
         """Delete a file/dir and the metadata."""
@@ -928,26 +914,48 @@
         mdobj = self.fs[mdid]
         log_debug("delete: path=%r mdid=%r", path, mdid)
 
-        # delete the file
-        if self.is_dir(path=path):
-            expected_event = "FS_DIR_DELETE"
-            remove_action = remove_dir
+        is_dir = self.is_dir(path=path)
+        if is_dir:
+            filter_event = "FS_DIR_DELETE"
         else:
-            expected_event = "FS_FILE_DELETE"
-            remove_action = remove_file
+            filter_event = "FS_FILE_DELETE"
+        self.eq.add_to_mute_filter(filter_event, path=path)
+
         try:
-            with self._enable_share_write(mdobj['share_id'], path):
-                self.eq.add_to_mute_filter(expected_event, path=path)
-                remove_action(path)
+            if is_dir:
+                if os.listdir(path):
+                    # not empty, need to check if we can delete it
+                    subtree = self._delete_dir_tree(path=path)
+                    for p, is_dir in subtree:
+                        filter_name = "FS_DIR_DELETE" if is_dir else "FS_FILE_DELETE"
+                        self.eq.add_to_mute_filter(filter_name, path=p)
+                        self.delete_metadata(p)
+
+                    with self._enable_share_write(mdobj['share_id'], path, recursive=True):
+                        if self.user_config.get_use_trash():
+                            move_to_trash(path)
+                        else:
+                            shutil.rmtree(path)
+                else:
+                    # empty, just delete it
+                    with self._enable_share_write(mdobj['share_id'], path):
+                        if self.user_config.get_use_trash():
+                            move_to_trash(path)
+                        else:
+                            remove_dir(path)
+            else:
+                # it's a file, just delete it
+                with self._enable_share_write(mdobj['share_id'], path):
+                    if self.user_config.get_use_trash():
+                        move_to_trash(path)
+                    else:
+                        remove_file(path)
+
         except OSError, e:
-            self.eq.rm_from_mute_filter(expected_event, path=path)
-            if e.errno == errno.ENOTEMPTY:
-                if not self._delete_dir_tree(path=path,
-                                             share_id=mdobj['share_id']):
-                    raise
-            else:
-                m = "OSError %s when trying to remove file/dir %r"
-                log_warning(m, e, path)
+            self.eq.rm_from_mute_filter(filter_event, path=path)
+            log_warning("OSError %s when trying to remove file/dir %r",
+                        e, path)
+
         self.delete_metadata(path)
 
     def move_to_conflict(self, mdid):
@@ -1277,10 +1285,10 @@
         else:
             return os.path.join(share_path, path)
 
-    def _enable_share_write(self, share_id, path):
+    def _enable_share_write(self, share_id, path, recursive=False):
         """Helper to create a EnableShareWrite context manager."""
         share = self._get_share(share_id)
-        return EnableShareWrite(share, path)
+        return EnableShareWrite(share, path, recursive)
 
     def get_paths_starting_with(self, base_path, include_base=True):
         """Return a list of paths that are starts with base_path.
@@ -1464,45 +1472,77 @@
 class EnableShareWrite(object):
     """Context manager to allow write in ro-shares."""
 
-    def __init__(self, share, path):
+    def __init__(self, share, path, recursive=False):
         self.share = share
         self.path = path
         self.ro = not self.share.can_write()
+        self.recursive = recursive
+
+        # list of (path, isdir) to restore permissions
+        self._changed_nodes = []
 
     def __enter__(self):
-        """ContextManager API."""
+        """Change the nodes to be writable."""
+        if not self.ro:
+            return self
+
+        # the parent should be writable for us to change path
+        parent = os.path.dirname(self.path)
+        parent_stat = get_stat(parent)
+        if parent_stat is None:
+            # if we don't have the parent yet, create it
+            with EnableShareWrite(self.share, parent):
+                make_dir(parent)
+        set_dir_readwrite(parent)
+        self._changed_nodes.append((parent, True))
+
+        # so, change path if exists
         path_stat = get_stat(self.path)
-        if path_stat is not None and stat.S_ISDIR(path_stat.st_mode) and \
-           os.path.normpath(self.path) == os.path.normpath(self.share.path):
-            self.parent = self.path
-            parent_stat = path_stat
-        else:
-            self.parent = os.path.dirname(self.path)
-            parent_stat = get_stat(self.parent)
-
-        if self.ro:
-            if parent_stat is None:
-                # if we don't have the parent yet, create it
-                with EnableShareWrite(self.share, self.parent):
-                    make_dir(self.parent)
-            set_dir_readwrite(self.parent)
-            # refresh the stat
-            path_stat = get_stat(self.path)
-            if path_stat is not None and not stat.S_ISDIR(path_stat.st_mode):
+        if path_stat is not None:
+            if stat.S_ISDIR(path_stat.st_mode):
+                set_dir_readwrite(self.path)
+                self._changed_nodes.append((self.path, True))
+            else:
                 set_file_readwrite(self.path)
+                self._changed_nodes.append((self.path, False))
+
+        # if needed, change the whole subtree
+        if self.recursive:
+            for dirpath, dirnames, filenames in os.walk(self.path, topdown=False):
+                for dname in dirnames:
+                    path = os.path.join(dirpath, dname)
+                    set_dir_readwrite(path)
+                    self._changed_nodes.append((path, True))
+                for fname in filenames:
+                    path = os.path.join(dirpath, fname)
+                    set_file_readwrite(path)
+                    self._changed_nodes.append((path, False))
         return self
 
     def __exit__(self, *exc_info):
-        """ContextManager API."""
-        if self.ro:
-            path_stat = get_stat(self.path)
-            if path_stat is not None:
-                if stat.S_ISDIR(path_stat.st_mode):
-                    set_dir_readonly(self.path)
+        """Restore node permissions.
+
+        Note that this is done backwards, from the leaf to the root.
+        """
+        if not self.ro:
+            return
+
+        # restore self.path, that may not have existed at __enter__ time
+        path_stat = get_stat(self.path)
+        if path_stat is not None:
+            if stat.S_ISDIR(path_stat.st_mode):
+                set_dir_readonly(self.path)
+            else:
+                set_file_readonly(self.path)
+
+        # restore all saved ones
+        exists = os.path.exists
+        for path, isdir in self._changed_nodes[::-1]:
+            if exists(path):
+                if isdir:
+                    set_dir_readonly(path)
                 else:
-                    set_file_readonly(self.path)
-            if path_exists(self.parent):
-                set_dir_readonly(self.parent)
+                    set_file_readonly(path)
 
 
 def get_stat(path):

=== modified file 'ubuntuone/syncdaemon/sync.py'
--- ubuntuone/syncdaemon/sync.py	2011-03-31 00:36:23 +0000
+++ ubuntuone/syncdaemon/sync.py	2011-04-04 18:56:34 +0000
@@ -30,7 +30,10 @@
     StateMachineRunner, StateMachine
 from ubuntuone.syncdaemon import u1fsfsm
 from ubuntuone.syncdaemon.logger import DebugCapture
-from ubuntuone.syncdaemon.filesystem_manager import InconsistencyError
+from ubuntuone.syncdaemon.filesystem_manager import (
+    DirectoryNotRemovable,
+    InconsistencyError,
+)
 from ubuntuone.syncdaemon.volume_manager import VolumeDoesNotExist
 from ubuntuone.platform import (
     stat_path,
@@ -658,12 +661,12 @@
         """server file was deleted."""
         try:
             self.key.delete_file()
+        except DirectoryNotRemovable:
+            # directory not empty and with stuff that should not be deleted
+            self.key.move_to_conflict()
+            self.key.delete_metadata()
         except OSError, e:
-            if e.errno == 39:
-                # if directory not empty
-                self.key.move_to_conflict()
-                self.key.delete_metadata()
-            elif e.errno == 2:
+            if e.errno == 2:
                 # file gone
                 pass
             else: