From 13995201a17ef3fe57d0a7560384e27bc74ddd58 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Fri, 18 May 2012 15:16:38 -0700 Subject: [PATCH] human-readable filesystem errors (#387) --- beets/importer.py | 4 +-- beets/library.py | 6 ++-- beets/ui/__init__.py | 3 ++ beets/util/__init__.py | 74 ++++++++++++++++++++++++++++++++---------- docs/changelog.rst | 3 ++ test/test_files.py | 12 +++---- 6 files changed, 72 insertions(+), 30 deletions(-) diff --git a/beets/importer.py b/beets/importer.py index 70b33d872..487173fb0 100644 --- a/beets/importer.py +++ b/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) diff --git a/beets/library.py b/beets/library.py index c4c4bb28e..da4b4dd1e 100644 --- a/beets/library.py +++ b/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) diff --git a/beets/ui/__init__.py b/beets/ui/__init__.py index 06430c20d..443b40698 100644 --- a/beets/ui/__init__.py +++ b/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. diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 1eaead9d0..bddf1374f 100644 --- a/beets/util/__init__.py +++ b/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 diff --git a/docs/changelog.rst b/docs/changelog.rst index f768fd1a3..232c71ec6 100644 --- a/docs/changelog.rst +++ b/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 diff --git a/test/test_files.py b/test/test_files.py index accdc0bdb..121df5eb9 100644 --- a/test/test_files.py +++ b/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):