diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 7565563..ca2a047 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -16,6 +16,7 @@ import argparse from concurrent import futures import six +import traceback import beets from beets import util, art @@ -23,11 +24,41 @@ 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): diff --git a/test/cli_test.py b/test/cli_test.py index d159bdb..280ffef 100644 --- a/test/cli_test.py +++ b/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