Merge 04ad8dd into b4a7865

beetsplug/alternatives.py

@@ -16,18 +16,49 @@
 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())
+
+
+def _samelink(p1, p2):
+    if p1 == p2:
+        return True
+    else:
+        # os.path.samefile / shutil._samefile follow symlinks, which is
+        # not really what we want to check here (although it will typically
+        # work, because if path and dest are not identical, then dest will
+        # not exist).
+        try:
+            return os.path.samestat(os.lstat(p1), os.lstat(p2))
+        except OSError:
+            return False
+
+
 class AlternativesPlugin(BeetsPlugin):
 
     def __init__(self):
@@ -166,29 +197,50 @@ def parse_config(self, config):
             dir = os.path.join(self.lib.directory, dir)
         self.directory = dir
 
+    def is_in_external(self, path):
+        return os.path.isfile(syspath(path))
+
+    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)):
+        print_("get_path(item): ")
+        try:
+            print_(repr(path))
+            if path:
+                    print_("lstat(get_path(item)): " + str(os.lstat(syspath(path))))
+        except:
+            pass
+        if path and self.is_in_external(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])
 
@@ -276,7 +328,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]
 
@@ -359,6 +411,25 @@ def parse_config(self, config):
 
         super(SymlinkView, self).parse_config(config)
 
+    def is_in_external(self, path):
+        return os.path.islink(syspath(path))
+
+    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 _samelink(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)
+
+        # FIXME: test album art link
+        return actions
+
     def update(self, create=None):
         for (item, actions) in self.items_actions():
             dest = self.destination(item)
@@ -372,7 +443,20 @@ def update(self, create=None):
                     self.set_path(item, dest)
                 elif action == self.ADD:
                     print_(u'+{0}'.format(displayable_path(dest)))
-                    self.create_symlink(item)
+                    replace = False
+                    if (os.path.islink(syspath(dest))):
+                        replace = input_yn(u"There is a symlink at '{0}', "
+                            "but it wasn't created by beets-alternatives.\n"
+                            "Overwrite it? (y/n)"
+                                .format(displayable_path(dest)),
+                            require=False)
+                    elif (os.path.isfile(syspath(dest))):
+                        replace = input_yn(u"There is a file at '{0}'. "
+                            "Are you sure you wan't to replace it with a "
+                            "symlink for your alternative collection? (y/n)"
+                            .format(displayable_path(dest)),
+                            require=True)
+                    self.create_symlink(item, replace)
                     self.set_path(item, dest)
                 elif action == self.REMOVE:
                     print_(u'-{0}'.format(displayable_path(path)))
@@ -381,13 +465,17 @@ def update(self, create=None):
                     continue
                 item.store()
 
-    def create_symlink(self, item):
+    def create_symlink(self, item, replace=False):
         dest = self.destination(item)
         util.mkdirall(dest)
         link = (
             os.path.relpath(item.path, os.path.dirname(dest))
             if self.relativelinks == self.LINK_RELATIVE else item.path)
-        util.link(link, dest)
+        util.link(link, dest, replace)
+
+    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, absolute=True):
+        """ 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 as 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=absolute,
+        )
+
+        # `-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=absolute,
+        )
+
     def test_valid_options(self):
         """ Test that an error is raised when option is invalid
         * Config link type is invalid