From fa58552a08408963ab8e9e1d7b9a31334a60b00e 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/5] 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..100a08f 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): + """ 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 From cb72d01bb2fbddcfcaa88f1221113ef595c853e0 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/5] 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 dc420d42e4757f0bc3fd4271c91c0821ca05357f 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/5] 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 30a217569c3eeb741ecfaadc0601b754760eb2b9 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/5] fix detecting whether a file is in the SymlinkView previously, symlinks were followed by os.path.isfile --- beetsplug/alternatives.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index cc0d25c..772588e 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -185,7 +185,7 @@ def parse_config(self, config): 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) From ffa7af835ca4898ad10bdbf7926b81784a5ae9d7 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/5] 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 | 54 +++++++++++++++++++++++++++++---------- 1 file changed, 40 insertions(+), 14 deletions(-) diff --git a/beetsplug/alternatives.py b/beetsplug/alternatives.py index 772588e..9c448e2 100644 --- a/beetsplug/alternatives.py +++ b/beetsplug/alternatives.py @@ -183,6 +183,29 @@ 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.lexists(syspath(path)): @@ -192,20 +215,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]) @@ -376,6 +387,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)