human-readable filesystem errors (#387)

beets/importer.py

@@ -716,7 +716,7 @@ def apply_choices(config):
                 if lib.directory in util.ancestry(duplicate_path):
                     log.debug(u'deleting replaced duplicate %s' %
                               util.displayable_path(duplicate_path))
-                    util.soft_remove(duplicate_path)
+                    util.remove(duplicate_path)
                     util.prune_dirs(os.path.dirname(duplicate_path),
                                     lib.directory)
 
@@ -832,7 +832,7 @@ def finalize(config):
             for old_path in task.old_paths:
                 # Only delete files that were actually copied.
                 if old_path not in new_paths:
-                    os.remove(syspath(old_path))
+                    util.remove(syspath(old_path), False)
                     # Clean up directory if it is emptied.
                     if task.toppath:
                         task.prune(old_path)

beets/library.py

@@ -1162,7 +1162,7 @@ class Library(BaseLibrary):
                 album.remove(delete, False)
 
         if delete:
-            util.soft_remove(item.path)
+            util.remove(item.path)
             util.prune_dirs(os.path.dirname(item.path), self.directory)
 
         self._memotable = {}
@@ -1391,7 +1391,7 @@ class Album(BaseAlbum):
             # Delete art file.
             artpath = self.artpath
             if artpath:
-                util.soft_remove(artpath)
+                util.remove(artpath)
 
         with self._library.transaction() as tx:
             if with_items:
@@ -1489,7 +1489,7 @@ class Album(BaseAlbum):
 
         # Normal operation.
         if oldart == artdest:
-            util.soft_remove(oldart)
+            util.remove(oldart)
         artdest = util.unique_path(artdest)
         if copy:
             util.copy(path, artdest)

beets/ui/__init__.py

@@ -749,6 +749,9 @@ def main(args=None, configfh=None):
     except UserError as exc:
         message = exc.args[0] if exc.args else None
         subcommand.parser.error(message)
+    except util.FilesystemError as exc:
+        log.error(u'file manipulation error: {0}'.format(exc.args[0]))
+        sys.exit(1)
     except IOError as exc:
         if exc.errno == errno.EPIPE:
             # "Broken pipe". End silently.

beets/util/__init__.py

@@ -1,5 +1,5 @@
 # This file is part of beets.
-# Copyright 2011, Adrian Sampson.
+# Copyright 2012, Adrian Sampson.
 #
 # Permission is hereby granted, free of charge, to any person obtaining
 # a copy of this software and associated documentation files (the
@@ -24,6 +24,39 @@ from collections import defaultdict
 
 MAX_FILENAME_LENGTH = 200
 
+class FilesystemError(Exception):
+    """An error that occurred while performing a filesystem manipulation
+    via a function in this module. Has three fields: `reason`, the
+    underlying exception or a string describing the problem; `verb`, the
+    action being performed; and `paths`, a list of pathnames involved.
+    """
+    def __init__(self, reason, verb, paths):
+        self.reason = reason
+        self.verb = verb
+        self.paths = paths
+
+        # Use a nicer English phrasing for some specific verbs.
+        gerund = verb[:-1] if verb.endswith('e') else verb
+        gerund += 'ing'
+        if verb in ('move', 'copy'):
+            clause = 'while {0} {1} to {2}'.format(gerund, repr(paths[0]),
+                                                   repr(paths[1]))
+        elif verb in ('delete',):
+            clause = 'while {0} {1}'.format(gerund, repr(paths[0]))
+        else:
+            clause = 'during {0} of paths {1}'.format(
+                self.verb, u', '.join(repr(p) for p in paths)
+            )
+
+        if isinstance(reason, basestring):
+            reason_str = reason
+        elif hasattr(reason, 'strerror'):  # i.e., EnvironmentError
+            reason_str = reason.strerror
+        else:
+            reason_str = u'"{0}"'.format(reason)
+        msg = u'{0} {1}'.format(reason_str, clause)
+        super(FilesystemError, self).__init__(msg)
+
 def normpath(path):
     """Provide the canonical form of the path suitable for storing in
     the database.
@@ -221,19 +254,19 @@ def samefile(p1, p2):
     """Safer equality for paths."""
     return shutil._samefile(syspath(p1), syspath(p2))
 
-def soft_remove(path):
-    """Remove the file if it exists."""
+def remove(path, soft=True):
+    """Remove the file. If `soft`, then no error will be raised if the
+    file does not exist.
+    """
     path = syspath(path)
-    if os.path.exists(path):
+    if soft and not os.path.exists(path):
+        return
+    try:
         os.remove(path)
+    except (OSError, IOError) as exc:
+        raise FilesystemError(exc, 'delete', (path,))
 
-def _assert_not_exists(path, pathmod=None):
-    """Raises an OSError if the path exists."""
-    pathmod = pathmod or os.path
-    if pathmod.exists(path):
-        raise OSError('file exists: %s' % path)
-
-def copy(path, dest, replace=False, pathmod=None):
+def copy(path, dest, replace=False, pathmod=os.path):
     """Copy a plain file. Permissions are not copied. If dest already
     exists, raises an OSError unless replace is True. Has no effect if
     path is the same as dest. Paths are translated to system paths
@@ -243,11 +276,14 @@ def copy(path, dest, replace=False, pathmod=None):
         return
     path = syspath(path)
     dest = syspath(dest)
-    if not replace:
-        _assert_not_exists(dest, pathmod)
-    return shutil.copyfile(path, dest)
+    if not replace and pathmod.exists(dest):
+        raise FilesystemError('file exists', 'copy', (path, dest))
+    try:
+        shutil.copyfile(path, dest)
+    except (OSError, IOError) as exc:
+        raise FilesystemError(exc, 'copy', (path, dest))
 
-def move(path, dest, replace=False, pathmod=None):
+def move(path, dest, replace=False, pathmod=os.path):
     """Rename a file. dest may not be a directory. If dest already
     exists, raises an OSError unless replace is True. Hos no effect if
     path is the same as dest. Paths are translated to system paths.
@@ -256,8 +292,12 @@ def move(path, dest, replace=False, pathmod=None):
         return
     path = syspath(path)
     dest = syspath(dest)
-    _assert_not_exists(dest, pathmod)
-    return shutil.move(path, dest)
+    if pathmod.exists(dest):
+        raise FilesystemError('file exists', 'move', (path, dest))
+    try:
+        shutil.move(path, dest)
+    except (OSError, IOError) as exc:
+        raise FilesystemError(exc, 'move', (path, dest))
 
 def unique_path(path):
     """Returns a version of ``path`` that does not exist on the

docs/changelog.rst

@@ -11,6 +11,9 @@ Changelog
   Thanks to Fabrice Laporte.
 * Errors when communicating with MusicBrainz now log an error message instead of
   halting the importer.
+* Similarly, filesystem manipulation errors now print helpful error messages
+  instead of a messy traceback. They still interrupt beets, but they should now
+  be easier for users to understand.
 * New plugin event: ``import_task_choice`` is called after an import task has an
   action assigned.
 * New plugin event: ``library_opened`` is called when beets starts up and

test/test_files.py

@@ -449,12 +449,12 @@ class SoftRemoveTest(unittest.TestCase, _common.ExtraAsserts):
             os.remove(self.path)
 
     def test_soft_remove_deletes_file(self):
-        util.soft_remove(self.path)
+        util.remove(self.path, True)
         self.assertNotExists(self.path)
 
     def test_soft_remove_silent_on_no_file(self):
         try:
-            util.soft_remove(self.path + 'XXX')
+            util.remove(self.path + 'XXX', True)
         except OSError:
             self.fail('OSError when removing path')
 
@@ -473,10 +473,6 @@ class SafeMoveCopyTest(unittest.TestCase, _common.ExtraAsserts):
         if os.path.exists(self.dest):
             os.remove(self.dest)
 
-    def test_existence_check(self):
-        with self.assertRaises(OSError):
-            util._assert_not_exists(self.path)
-
     def test_successful_move(self):
         util.move(self.path, self.dest)
         self.assertExists(self.dest)
@@ -488,11 +484,11 @@ class SafeMoveCopyTest(unittest.TestCase, _common.ExtraAsserts):
         self.assertExists(self.path)
 
     def test_unsuccessful_move(self):
-        with self.assertRaises(OSError):
+        with self.assertRaises(util.FilesystemError):
             util.move(self.path, self.otherpath)
 
     def test_unsuccessful_copy(self):
-        with self.assertRaises(OSError):
+        with self.assertRaises(util.FilesystemError):
             util.copy(self.path, self.otherpath)
 
     def test_self_move(self):