From 94569a774e8075337f96a071ce77e1df692add98 Mon Sep 17 00:00:00 2001 From: Adrian Sampson Date: Sun, 28 Aug 2011 18:25:38 -0700 Subject: [PATCH] moving/copying fails when destination exists (#230) --- NEWS | 1 + beets/library.py | 14 +++++--------- beets/util/__init__.py | 26 ++++++++++++++++++++++++++ test/test_files.py | 39 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 9 deletions(-) diff --git a/NEWS b/NEWS index d7cd0a1dd..0745302d6 100644 --- a/NEWS +++ b/NEWS @@ -28,6 +28,7 @@ * A new plugin, called "web", encapsulates a simple Web-based GUI for beets. The current iteration can browse the library and play music in browsers that support HTML5 Audio. +* Files are no longer silently overwritten when moving and copying files. * Handle exceptions thrown when running Mutagen. * Fix a missing __future__ import in embedart on Python 2.5. * Fix ID3 and MPEG-4 tag names for the album-artist field. diff --git a/beets/library.py b/beets/library.py index 51d376b36..90f7952a3 100644 --- a/beets/library.py +++ b/beets/library.py @@ -15,7 +15,6 @@ import sqlite3 import os import re -import shutil import sys from string import Template import logging @@ -242,12 +241,9 @@ class Item(object): 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 - # the original is read-only. - shutil.copyfile(syspath(self.path), syspath(dest)) + util.copy(self.path, dest) else: - shutil.move(syspath(self.path), syspath(dest)) + util.move(self.path, dest) # Either copying or moving succeeded, so update the stored path. old_path = self.path @@ -1179,9 +1175,9 @@ class Album(BaseAlbum): new_art = self.art_destination(old_art, newdir) if new_art != old_art: if copy: - shutil.copy(syspath(old_art), syspath(new_art)) + util.copy(old_art, new_art) else: - shutil.move(syspath(old_art), syspath(new_art)) + util.move(old_art, new_art) self.artpath = new_art if not copy: # Prune old path. util.prune_dirs(os.path.dirname(old_art), @@ -1236,5 +1232,5 @@ class Album(BaseAlbum): # Normal operation. if oldart == artdest: util.soft_remove(oldart) - shutil.copyfile(syspath(path), syspath(artdest)) + util.copy(path, artdest) self.artpath = artdest diff --git a/beets/util/__init__.py b/beets/util/__init__.py index f73811ac9..67a878342 100644 --- a/beets/util/__init__.py +++ b/beets/util/__init__.py @@ -185,6 +185,32 @@ def soft_remove(path): if os.path.exists(path): os.remove(path) +def _assert_not_exists(path, pathmod=None): + """Raises an OSError if the path exists.""" + pathmod = pathmod or os.path + if pathmod.exists(path): + raise OSError('file exists: %s' % path) + +def copy(path, dest, replace=False, pathmod=None): + """Copy a plain file. Permissions are not copied. If dest already + exists, raises an OSError unless replace is True. Paths are + translated to system paths before the syscall. + """ + path = syspath(path) + dest = syspath(dest) + _assert_not_exists(dest, pathmod) + return shutil.copyfile(path, dest) + +def move(path, dest, replace=False, pathmod=None): + """Rename a file. dest may not be a directory. If dest already + exists, raises an OSError unless replace is True. Paths are + translated to system paths. + """ + path = syspath(path) + dest = syspath(dest) + _assert_not_exists(dest, pathmod) + return shutil.move(path, dest) + # Note: POSIX actually supports \ and : -- I just think they're # a pain. And ? has caused problems for some. CHAR_REPLACE = [ diff --git a/test/test_files.py b/test/test_files.py index da175fcb8..d99292959 100644 --- a/test/test_files.py +++ b/test/test_files.py @@ -248,6 +248,8 @@ class ArtFileTest(unittest.TestCase, _common.ExtraAsserts): self.assertTrue('testotherdir' in newart) def test_setart_copies_image(self): + os.remove(self.art) + newart = os.path.join(self.libdir, 'newart.jpg') touch(newart) i2 = item() @@ -261,6 +263,8 @@ class ArtFileTest(unittest.TestCase, _common.ExtraAsserts): self.assertTrue(os.path.exists(ai.artpath)) def test_setart_to_existing_art_works(self): + os.remove(self.art) + # Original art. newart = os.path.join(self.libdir, 'newart.jpg') touch(newart) @@ -293,6 +297,8 @@ class ArtFileTest(unittest.TestCase, _common.ExtraAsserts): self.assertTrue(os.path.exists(ai.artpath)) def test_setart_sets_permissions(self): + os.remove(self.art) + newart = os.path.join(self.libdir, 'newart.jpg') touch(newart) os.chmod(newart, 0400) # read-only @@ -393,6 +399,39 @@ class SoftRemoveTest(unittest.TestCase, _common.ExtraAsserts): except OSError: self.fail('OSError when removing path') +class SafeMoveCopyTest(unittest.TestCase): + def setUp(self): + self.path = os.path.join(_common.RSRC, 'testfile') + touch(self.path) + self.otherpath = os.path.join(_common.RSRC, 'testfile2') + touch(self.otherpath) + self.dest = self.path + '.dest' + def tearDown(self): + if os.path.exists(self.path): + os.remove(self.path) + if os.path.exists(self.otherpath): + os.remove(self.otherpath) + if os.path.exists(self.dest): + os.remove(self.dest) + + def test_existence_check(self): + with self.assertRaises(OSError): + util._assert_not_exists(self.path) + + def test_successful_move(self): + util.move(self.path, self.dest) + + def test_successful_copy(self): + util.copy(self.path, self.dest) + + def test_unsuccessful_move(self): + with self.assertRaises(OSError): + util.move(self.path, self.otherpath) + + def test_unsuccessful_copy(self): + with self.assertRaises(OSError): + util.copy(self.path, self.otherpath) + def suite(): return unittest.TestLoader().loadTestsFromName(__name__)