diff --git a/beets/importer.py b/beets/importer.py index a499ec792..538d786a3 100644 --- a/beets/importer.py +++ b/beets/importer.py @@ -19,6 +19,7 @@ from __future__ import with_statement # Python 2.5 import os import logging import pickle +from collections import defaultdict from beets import autotag from beets import library @@ -508,6 +509,7 @@ def apply_choices(config): task = yield task if task.should_skip(): continue + items = task.items if task.is_album else [task.item] # Change metadata. if task.should_write_tags(): @@ -520,12 +522,26 @@ def apply_choices(config): if task.is_album: _infer_album_fields(task) + # Find existing item entries that these are replacing. Old + # album structures are automatically cleaned up when the + # last item is removed. + replaced_items = defaultdict(list) + for item in items: + dup_items = list(lib.items( + library.MatchQuery('path', item.path) + )) + for dup_item in dup_items: + if dup_item.id != item.id: + replaced_items[item].append(dup_item) + # Move/copy files. - items = task.items if task.is_album else [task.item] task.old_paths = [item.path for item in items] for item in items: if config.copy: - item.move(lib, True, task.is_album) + # If we're replacing an item, then move rather than + # copying. + do_copy = not bool(replaced_items[item]) + item.move(lib, do_copy, task.is_album) if config.write and task.should_write_tags(): item.write() @@ -542,16 +558,10 @@ def apply_choices(config): for item in items: lib.add(item) - # Remove old entries if we're re-importing old items. Old - # album structures are automatically cleaned up when the - # last item is removed. - for item, old_path in zip(items, task.old_paths): - dup_items = list(lib.items( - library.MatchQuery('path', old_path) - )) - for dup_item in dup_items: - if dup_item.id != item.id: - lib.remove(dup_item) + # Remove old ones. + for replaced in replaced_items.itervalues(): + for item in replaced: + lib.remove(item) finally: lib.save() diff --git a/beets/library.py b/beets/library.py index 01fd7c1fb..9db4eee7e 100644 --- a/beets/library.py +++ b/beets/library.py @@ -22,7 +22,7 @@ import logging from beets.mediafile import MediaFile from beets import plugins from beets import util -from beets.util import bytestring_path, syspath, normpath +from beets.util import bytestring_path, syspath, normpath, samefile MAX_FILENAME_LENGTH = 200 @@ -237,7 +237,7 @@ class Item(object): # Create necessary ancestry for the move. util.mkdirall(dest) - if not shutil._samefile(syspath(self.path), syspath(dest)): + if not samefile(self.path, dest): if copy: # copyfile rather than copy will not copy permissions # bits, thus possibly making the copy writable even when @@ -1213,10 +1213,10 @@ class Album(BaseAlbum): oldart = self.artpath artdest = self.art_destination(path) - if oldart and shutil._samefile(syspath(path), syspath(oldart)): + if oldart and samefile(path, oldart): # Art already set. return - elif shutil._samefile(syspath(path), syspath(artdest)): + elif samefile(path, artdest): # Art already in place. self.artpath = path return diff --git a/beets/util/__init__.py b/beets/util/__init__.py index 62fa0ec55..f73811ac9 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -175,6 +175,10 @@ def syspath(path, pathmod=None): return path +def samefile(p1, p2): + """Safer equality for paths.""" + return shutil._samefile(syspath(p1), syspath(p2)) + def soft_remove(path): """Remove the file if it exists.""" path = syspath(path) diff --git a/test/test_importer.py b/test/test_importer.py index 0c3dd5ac4..309ca92af 100644 --- a/test/test_importer.py +++ b/test/test_importer.py @@ -373,6 +373,74 @@ class ApplyExistingItemsTest(unittest.TestCase, _common.ExtraAsserts): self.assertTrue('differentTitle' in item.path) self.assertExists(item.path) + def test_apply_existing_item_new_metadata_copy_disabled(self): + # Import *without* copying to ensure that the path does *not* change. + self.config.copy = False + self._apply_asis([self.i]) + item = self.lib.items().next() + new_item = library.Item.from_path(item.path) + new_item.title = 'differentTitle' + self._apply_asis([new_item]) + + item = self.lib.items().next() + self.assertFalse('differentTitle' in item.path) + self.assertExists(item.path) + + def test_apply_existing_item_new_metadata_removes_old_files(self): + self.config.copy = True + self._apply_asis([self.i]) + item = self.lib.items().next() + oldpath = item.path + new_item = library.Item.from_path(item.path) + new_item.title = 'differentTitle' + self._apply_asis([new_item]) + + item = self.lib.items().next() + self.assertNotExists(oldpath) + + def test_apply_existing_item_new_metadata_delete_enabled(self): + # The "delete" flag should be ignored -- only the "copy" flag + # controls whether files move. + self.config.copy = True + self.config.delete = True # ! + self._apply_asis([self.i]) + item = self.lib.items().next() + oldpath = item.path + new_item = library.Item.from_path(item.path) + new_item.title = 'differentTitle' + self._apply_asis([new_item]) + + item = self.lib.items().next() + self.assertNotExists(oldpath) + self.assertTrue('differentTitle' in item.path) + self.assertExists(item.path) + + def test_apply_existing_item_preserves_file(self): + # With copying enabled, import the item twice with same metadata. + self.config.copy = True + self._apply_asis([self.i]) + item = self.lib.items().next() + oldpath = item.path + new_item = library.Item.from_path(item.path) + self._apply_asis([new_item]) + + self.assertEqual(len(list(self.lib.items())), 1) + item = self.lib.items().next() + self.assertEqual(oldpath, item.path) + self.assertExists(oldpath) + + def test_apply_existing_item_preserves_file_delete_enabled(self): + self.config.copy = True + self.config.delete = True # ! + self._apply_asis([self.i]) + item = self.lib.items().next() + new_item = library.Item.from_path(item.path) + self._apply_asis([new_item]) + + self.assertEqual(len(list(self.lib.items())), 1) + item = self.lib.items().next() + self.assertExists(item.path) + class InferAlbumDataTest(unittest.TestCase): def setUp(self): i1 = _common.item()