moving/copying fails when destination exists (#230)

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.

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

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 = [

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__)