SymlinkView: Fix symlink removal (fixes #47)

CHANGELOG.md

@@ -3,6 +3,10 @@ Change Log
 
 ## Upcoming
 * Add `beet alt list-tracks` command
+* SymlinkView: Fix stale symlinks not being removed when files are moved in the
+  main library [#47][]
+
+[#47]: https://github.com/geigerzaehler/beets-alternatives/issues/47
 
 ## v0.10.1 - 2019-09-18
 * Running `beet completion` does not crash anymore [#38][]

beetsplug/alternatives.py

@@ -17,18 +17,35 @@
 import argparse
 from concurrent import futures
 import six
+import traceback
 
 import beets
 from beets import util, art
 from beets.plugins import BeetsPlugin
 from beets.ui import Subcommand, get_path_formats, input_yn, UserError, \
     print_, decargs
 from beets.library import parse_query_string, Item
-from beets.util import syspath, displayable_path, cpu_count, bytestring_path
+from beets.util import syspath, displayable_path, cpu_count, bytestring_path, \
+        FilesystemError
 
 from beetsplug import convert
 
 
+def _remove(path, soft=True):
+    """Remove the file. If `soft`, then no error will be raised if the
+    file does not exist.
+    In contrast to beets' util.remove, this uses lexists such that it can
+    actually remove symlink links.
+    """
+    path = syspath(path)
+    if soft and not os.path.lexists(path):
+        return
+    try:
+        os.remove(path)
+    except (OSError, IOError) as exc:
+        raise FilesystemError(exc, 'delete', (path,), traceback.format_exc())
+
+
 class AlternativesPlugin(BeetsPlugin):
 
     def __init__(self):
@@ -167,29 +184,40 @@ def parse_config(self, config):
             dir = os.path.join(self.lib.directory, dir)
         self.directory = dir
 
+    def item_change_actions(self, item, path, dest):
+        """ Returns the necessary actions for items that were previously in the
+        external collection, but might require metadata updates.
+        """
+        actions = []
+
+        if not util.samefile(path, dest):
+            actions.append(self.MOVE)
+
+        item_mtime_alt = os.path.getmtime(syspath(path))
+        if (item_mtime_alt < os.path.getmtime(syspath(item.path))):
+            actions.append(self.WRITE)
+        album = item.get_album()
+
+        if album:
+            if (album.artpath and
+                    os.path.isfile(syspath(album.artpath)) and
+                    (item_mtime_alt
+                     < os.path.getmtime(syspath(album.artpath)))):
+                actions.append(self.SYNC_ART)
+
+        return actions
+
     def matched_item_action(self, item):
         path = self.get_path(item)
-        if path and os.path.isfile(syspath(path)):
+        if path and os.path.lexists(syspath(path)):
             dest = self.destination(item)
             _, path_ext = os.path.splitext(path)
             _, dest_ext = os.path.splitext(dest)
             if not path_ext == dest_ext:
                 # formats config option changed
                 return (item, [self.REMOVE, self.ADD])
-            actions = []
-            if not util.samefile(path, dest):
-                actions.append(self.MOVE)
-            item_mtime_alt = os.path.getmtime(syspath(path))
-            if (item_mtime_alt < os.path.getmtime(syspath(item.path))):
-                actions.append(self.WRITE)
-            album = item.get_album()
-            if album:
-                if (album.artpath and
-                        os.path.isfile(syspath(album.artpath)) and
-                        (item_mtime_alt
-                         < os.path.getmtime(syspath(album.artpath)))):
-                    actions.append(self.SYNC_ART)
-            return (item, actions)
+            else:
+                return (item, self.item_change_actions(item, path, dest))
         else:
             return (item, [self.ADD])
 
@@ -277,7 +305,7 @@ def get_path(self, item):
 
     def remove_item(self, item):
         path = self.get_path(item)
-        util.remove(path)
+        _remove(path)
         util.prune_dirs(path, root=self.directory)
         del item[self.path_key]
 
@@ -360,6 +388,21 @@ def parse_config(self, config):
 
         super(SymlinkView, self).parse_config(config)
 
+    def item_change_actions(self, item, path, dest):
+        """ Returns the necessary actions for items that were previously in the
+        external collection, but might require metadata updates.
+        """
+        actions = []
+
+        if not path == dest:
+            # The path of the link itself changed
+            actions.append(self.MOVE)
+        elif not util.samefile(path, item.path):
+            # link target changed
+            actions.append(self.MOVE)
+
+        return actions
+
     def update(self, create=None):
         for (item, actions) in self.items_actions():
             dest = self.destination(item)
@@ -390,6 +433,10 @@ def create_symlink(self, item):
             if self.relativelinks == self.LINK_RELATIVE else item.path)
         util.link(link, dest)
 
+    def sync_art(self, item, path):
+        # FIXME: symlink art
+        pass
+
 
 class Worker(futures.ThreadPoolExecutor):
 

test/cli_test.py

@@ -155,6 +155,44 @@ def test_add_move_remove_album_relative(self):
         self.alt_config['link_type'] = 'relative'
         self.test_add_move_remove_album(absolute=False)
 
+    def test_add_update_move_album(self):
+        """ Test that symlinks are properly updated and no broken links left
+        when an item's path in the library changes.
+        Since moving the items causes the links in the symlink view to be
+        broken, this situation used to be incorrectly detected as
+        addition of new items, such that the old links weren't removed.
+        Contrast this to the `test_add_move_remove_album` test, in which the
+        old links do not break upon changing the path format.
+        * An album is added.
+        * The album name is changed, which also causes the tracks to be moved.
+        * The symlink view is updated.
+        """
+        self.add_album(artist='Michael Jackson', album='Thriller', year='1990')
+
+        self.runcli('alt', 'update', 'by-year')
+
+        by_year_path = self.lib_path(b'by-year/1990/Thriller/track 1.mp3')
+        self.assertSymlink(
+            link=by_year_path,
+            target=self.lib_path(b'Michael Jackson/Thriller/track 1.mp3'),
+            absolute=True,
+        )
+
+        # `-y` skips the prompt, `-a` updates album-level fields, `-m` forces
+        # actually moving the files
+        self.runcli('mod', '-y', '-a', '-m',
+                    'Thriller', 'album=Thriller (Remastered)')
+        self.runcli('alt', 'update', 'by-year')
+
+        self.assertIsNotFile(by_year_path)
+        self.assertSymlink(
+            link=self.lib_path(
+                b'by-year/1990/Thriller (Remastered)/track 1.mp3'),
+            target=self.lib_path(
+                b'Michael Jackson/Thriller (Remastered)/track 1.mp3'),
+            absolute=True,
+        )
+
     def test_valid_options(self):
         """ Test that an error is raised when option is invalid
         * Config link type is invalid