From df2ea341836cef666b1053b45335d83f5df0850e Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Antoine=20Beaupr=C3=A9?= <anarcat@debian.org>
Date: Thu, 13 Jan 2022 11:46:18 -0500
Subject: [PATCH] Draft: handle file exceptions correctly in move, art_set, and
 move_art

This reflects the `try_write` approach will handles write failures
elegantly, by logging the error and continuing. We do the same with
`move`, `art_set` and `move_art`.

We also handle exceptions on `before_item_moved` and `art_set` plugins
hooks, the latter of which is moved *before* the file operations to
remain consistent with other hook configurations.

That might be a mistake and API-breaking change, another approach
would be to have a new `before_art_set` hook instead.

We also introduce a new hook (`move_art`) for those operations as
well.

The point of this patch is to make it possible for plugins to send a
signal (through the already FileOperationError exception) to callers
that it should skip a specific item or artwork.

This is essential to allow beets to better integrate with other
utilities like bittorrent clients which may rewrite those files. The
rationale here is that some music collections will have *parts* of
them managed by such clients in which case we should be careful not to
overwrite or move those files.

Operations like copy or hardlink are not handled by this, for that
reason. We may also want to do proper error handling for those as
well, that said, but that seems out of scope for this specific
issue (#2617).
---
 beets/importer.py | 10 +++++-----
 beets/library.py  | 36 +++++++++++++++++++++++++++++++-----
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/beets/importer.py b/beets/importer.py
index 561cedd2c..04c1f34e1 100644
--- a/beets/importer.py
+++ b/beets/importer.py
@@ -747,14 +747,14 @@ class ImportTask(BaseImportTask):
                 if (operation != MoveOperation.MOVE
                         and self.replaced_items[item]
                         and session.lib.directory in util.ancestry(old_path)):
-                    item.move()
-                    # We moved the item, so remove the
-                    # now-nonexistent file from old_paths.
-                    self.old_paths.remove(old_path)
+                    if item.try_move():
+                        # We moved the item, so remove the
+                        # now-nonexistent file from old_paths.
+                        self.old_paths.remove(old_path)
                 else:
                     # A normal import. Just copy files and keep track of
                     # old paths.
-                    item.move(operation)
+                    item.try_move(operation)
 
             if write and (self.apply or self.choice_flag == action.RETAG):
                 item.try_write()
diff --git a/beets/library.py b/beets/library.py
index c8993f85b..cf7b5588f 100644
--- a/beets/library.py
+++ b/beets/library.py
@@ -826,8 +826,12 @@ class Item(LibModel):
         if not util.samefile(self.path, dest):
             dest = util.unique_path(dest)
         if operation == MoveOperation.MOVE:
-            plugins.send("before_item_moved", item=self, source=self.path,
-                         destination=dest)
+            try:
+                plugins.send("before_item_moved", item=self, source=self.path,
+                             destination=dest)
+            except FileOperationError as exc:
+                log.error("{0}", exc)
+                return
             util.move(self.path, dest)
             plugins.send("item_moved", item=self, source=self.path,
                          destination=dest)
@@ -949,6 +953,19 @@ class Item(LibModel):
         if operation == MoveOperation.MOVE:
             util.prune_dirs(os.path.dirname(old_path), self._db.directory)
 
+    def try_move(self, *args, **kwargs):
+        """Call `move()` but catch and log `FileOperationError`
+        exceptions.
+
+        Return `False` an exception was caught and `True` otherwise.
+        """
+        try:
+            self.move(*args, **kwargs)
+            return True
+        except FileOperationError as exc:
+            log.error("{0}", exc)
+            return False
+
     # Templating.
 
     def destination(self, fragment=False, basedir=None, platform=None,
@@ -1195,6 +1212,11 @@ class Album(LibModel):
         if new_art == old_art:
             return
 
+        try:
+            plugins.send('move_art', album=self)
+        except FileOperationError as exc:
+            log.error("{0}", exc)
+            return
         new_art = util.unique_path(new_art)
         log.debug('moving album art {0} to {1}',
                   util.displayable_path(old_art),
@@ -1239,8 +1261,8 @@ class Album(LibModel):
         # Move items.
         items = list(self.items())
         for item in items:
-            item.move(operation, basedir=basedir, with_album=False,
-                      store=store)
+            item.try_move(operation, basedir=basedir, with_album=False,
+                          store=store)
 
         # Move art.
         self.move_art(operation)
@@ -1327,6 +1349,11 @@ class Album(LibModel):
             self.artpath = path
             return
 
+        try:
+            plugins.send('art_set', album=self)
+        except FileOperationError as exc:
+            log.error("{0}", exc)
+            return
         # Normal operation.
         if oldart == artdest:
             util.remove(oldart)
@@ -1337,7 +1364,6 @@ class Album(LibModel):
             util.move(path, artdest)
         self.artpath = artdest
 
-        plugins.send('art_set', album=self)
 
     def store(self, fields=None):
         """Update the database with the album information.