From aa8d670c3708627925a3df8ca1307315947d9e59 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 18 Mar 2020 12:39:59 +0100 Subject: [PATCH 1/9] SymlinkView: test removal of stale links reproduces https://github.com/geigerzaehler/beets-alternatives/issues/47 --- test/cli_test.py | 38 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) 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 From 8232d927ced3c358a263e827c0d7e2aaaafb7fbb Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 18 Mar 2020 14:04:49 +0100 Subject: [PATCH 2/9] in SymlinkView, make sync_art a no-op it might be interesting to actually symlink the artwork here, but running the embedding on the symlinked tracks really shouldn't be done --- beetsplug/alternatives.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 7565563..398084f 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -389,6 +389,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): From 6d8f03f5c654438b1e883efc9c51b9d12f37bff7 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 18 Mar 2020 14:06:25 +0100 Subject: [PATCH 3/9] modify beets' util.remove to be able to delete symlinks The `soft` kwarg will make util.remove silently skip the deletion of symlinks (os.remove doesn't appear to follow symlinks, but os.path.exists does) --- beetsplug/alternatives.py | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 398084f..cc0d25c 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,27 @@ 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): @@ -276,7 +293,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] From 27e26ce98ae9221b8d3da9cfaf6624375147579d Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 18 Mar 2020 14:08:54 +0100 Subject: [PATCH 4/9] fix detecting whether a file is in the SymlinkView previously, symlinks were followed by os.path.isfile --- beetsplug/alternatives.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index cc0d25c..b919831 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -183,9 +183,12 @@ 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 matched_item_action(self, item): path = self.get_path(item) - if path and os.path.isfile(syspath(path)): + if path and self.is_in_external(path): dest = self.destination(item) _, path_ext = os.path.splitext(path) _, dest_ext = os.path.splitext(dest) @@ -376,6 +379,9 @@ def parse_config(self, config): super(SymlinkView, self).parse_config(config) + def is_in_external(self, path): + return os.path.islink(syspath(path)) + def update(self, create=None): for (item, actions) in self.items_actions(): dest = self.destination(item) From 77e0ec3d629c13c42b46b9327150020c901a7379 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Wed, 18 Mar 2020 14:16:54 +0100 Subject: [PATCH 5/9] fox SymlinkView, do not check mtimes, but verify the links' existence and targets First, this also a file-system only operation, i.e. cheap, and secondly, the mtime code follows symlinks, so it is not really quite obscure whether and why it does the right thing for symlinks --- beetsplug/alternatives.py | 69 +++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 14 deletions(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index b919831..40fcd2a 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -45,6 +45,20 @@ def _remove(path, soft=True): 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): @@ -186,6 +200,29 @@ def parse_config(self, config): 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 self.is_in_external(path): @@ -195,20 +232,8 @@ def matched_item_action(self, item): 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]) @@ -382,6 +407,22 @@ def parse_config(self, 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) From d89a2966f67da064e06c7297a8e3ba527f4def1b Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Thu, 19 Mar 2020 19:05:29 +0100 Subject: [PATCH 6/9] add a few debug print_'s --- beetsplug/alternatives.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 40fcd2a..edac630 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -225,6 +225,12 @@ def item_change_actions(self, item, path, dest): def matched_item_action(self, item): path = self.get_path(item) + try: + print_("get_path(item): " + displayable_path(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) From 39b40fd43f99f653cc6575960894c402d3585f29 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Thu, 19 Mar 2020 20:34:17 +0100 Subject: [PATCH 7/9] change debug code to ensure that there's always an indication of it being run --- beetsplug/alternatives.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index edac630..58145ef 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -225,8 +225,9 @@ def item_change_actions(self, item, path, dest): def matched_item_action(self, item): path = self.get_path(item) + print_("get_path(item): ") try: - print_("get_path(item): " + displayable_path(path)) + print_(repr(path)) if path: print_("lstat(get_path(item)): " + str(os.lstat(syspath(path)))) except: From 7da3403374d42fb0acf171fa0e577deae1e17d74 Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Thu, 19 Mar 2020 20:34:45 +0100 Subject: [PATCH 8/9] user query to fix up broken links --- beetsplug/alternatives.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 58145ef..bfae8aa 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -443,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))) From 04ad8dde419e1cb51892fee40ae3dc02632ef10f Mon Sep 17 00:00:00 2001 From: wisp3rwind <17089248+wisp3rwind@users.noreply.github.com> Date: Sat, 21 Mar 2020 11:10:13 +0100 Subject: [PATCH 9/9] fix overwriting symlinks... --- beetsplug/alternatives.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index bfae8aa..ca2a047 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -451,7 +451,7 @@ def update(self, create=None): .format(displayable_path(dest)), require=False) elif (os.path.isfile(syspath(dest))): - replace = input_yn(u"There is a file at '{0}', " + 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)), @@ -465,13 +465,13 @@ 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