Issue 1577: shutil.move() where destination is a directory was doing a

copy, now it is doing a os.rename() if it's on the same file-system.

Lib/shutil.py

@@ -187,26 +187,44 @@ def rmtree(path, ignore_errors=False, onerror=None):
     except os.error:
         onerror(os.rmdir, path, sys.exc_info())
 
-def move(src, dst):
-    """Recursively move a file or directory to another location.
 
-    If the destination is on our current filesystem, then simply use
-    rename.  Otherwise, copy src to the dst and then remove src.
+def _basename(path):
+    # A basename() variant which first strips the trailing slash, if present.
+    # Thus we always get the last component of the path, even for directories.
+    return os.path.basename(path.rstrip(os.path.sep))
+
+def move(src, dst):
+    """Recursively move a file or directory to another location. This is
+    similar to the Unix "mv" command.
+
+    If the destination is a directory or a symlink to a directory, the source
+    is moved inside the directory. The destination path must not already
+    exist.
+
+    If the destination already exists but is not a directory, it may be
+    overwritten depending on os.rename() semantics.
+
+    If the destination is on our current filesystem, then rename() is used.
+    Otherwise, src is copied to the destination and then removed.
     A lot more could be done here...  A look at a mv.c shows a lot of
     the issues this implementation glosses over.
 
     """
-
+    real_dst = dst
+    if os.path.isdir(dst):
+        real_dst = os.path.join(dst, _basename(src))
+        if os.path.exists(real_dst):
+            raise Error, "Destination path '%s' already exists" % real_dst
     try:
-        os.rename(src, dst)
+        os.rename(src, real_dst)
     except OSError:
         if os.path.isdir(src):
             if destinsrc(src, dst):
                 raise Error, "Cannot move a directory '%s' into itself '%s'." % (src, dst)
-            copytree(src, dst, symlinks=True)
+            copytree(src, real_dst, symlinks=True)
             rmtree(src)
         else:
-            copy2(src,dst)
+            copy2(src, real_dst)
             os.unlink(src)
 
 def destinsrc(src, dst):

Lib/test/test_shutil.py

@@ -63,17 +63,6 @@ class TestShutil(unittest.TestCase):
         self.assertRaises(OSError, shutil.rmtree, path)
         os.remove(path)
 
-    def test_dont_move_dir_in_itself(self):
-        src_dir = tempfile.mkdtemp()
-        try:
-            dst = os.path.join(src_dir, 'foo')
-            self.assertRaises(shutil.Error, shutil.move, src_dir, dst)
-        finally:
-            try:
-                os.rmdir(src_dir)
-            except:
-                pass
-
     def test_copytree_simple(self):
         def write_data(path, data):
             f = open(path, "w")
@@ -162,9 +151,123 @@ class TestShutil(unittest.TestCase):
                 shutil.rmtree(TESTFN, ignore_errors=True)
 
 
+class TestMove(unittest.TestCase):
+
+    def setUp(self):
+        filename = "foo"
+        self.src_dir = tempfile.mkdtemp()
+        self.dst_dir = tempfile.mkdtemp()
+        self.src_file = os.path.join(self.src_dir, filename)
+        self.dst_file = os.path.join(self.dst_dir, filename)
+        # Try to create a dir in the current directory, hoping that it is
+        # not located on the same filesystem as the system tmp dir.
+        try:
+            self.dir_other_fs = tempfile.mkdtemp(
+                dir=os.path.dirname(__file__))
+            self.file_other_fs = os.path.join(self.dir_other_fs,
+                filename)
+        except OSError:
+            self.dir_other_fs = None
+        with open(self.src_file, "wb") as f:
+            f.write("spam")
+
+    def tearDown(self):
+        for d in (self.src_dir, self.dst_dir, self.dir_other_fs):
+            try:
+                if d:
+                    shutil.rmtree(d)
+            except:
+                pass
+
+    def _check_move_file(self, src, dst, real_dst):
+        contents = open(src, "rb").read()
+        shutil.move(src, dst)
+        self.assertEqual(contents, open(real_dst, "rb").read())
+        self.assertFalse(os.path.exists(src))
+
+    def _check_move_dir(self, src, dst, real_dst):
+        contents = sorted(os.listdir(src))
+        shutil.move(src, dst)
+        self.assertEqual(contents, sorted(os.listdir(real_dst)))
+        self.assertFalse(os.path.exists(src))
+
+    def test_move_file(self):
+        # Move a file to another location on the same filesystem.
+        self._check_move_file(self.src_file, self.dst_file, self.dst_file)
+
+    def test_move_file_to_dir(self):
+        # Move a file inside an existing dir on the same filesystem.
+        self._check_move_file(self.src_file, self.dst_dir, self.dst_file)
+
+    def test_move_file_other_fs(self):
+        # Move a file to an existing dir on another filesystem.
+        if not self.dir_other_fs:
+            # skip
+            return
+        self._check_move_file(self.src_file, self.file_other_fs,
+            self.file_other_fs)
+
+    def test_move_file_to_dir_other_fs(self):
+        # Move a file to another location on another filesystem.
+        if not self.dir_other_fs:
+            # skip
+            return
+        self._check_move_file(self.src_file, self.dir_other_fs,
+            self.file_other_fs)
+
+    def test_move_dir(self):
+        # Move a dir to another location on the same filesystem.
+        dst_dir = tempfile.mktemp()
+        try:
+            self._check_move_dir(self.src_dir, dst_dir, dst_dir)
+        finally:
+            try:
+                shutil.rmtree(dst_dir)
+            except:
+                pass
+
+    def test_move_dir_other_fs(self):
+        # Move a dir to another location on another filesystem.
+        if not self.dir_other_fs:
+            # skip
+            return
+        dst_dir = tempfile.mktemp(dir=self.dir_other_fs)
+        try:
+            self._check_move_dir(self.src_dir, dst_dir, dst_dir)
+        finally:
+            try:
+                shutil.rmtree(dst_dir)
+            except:
+                pass
+
+    def test_move_dir_to_dir(self):
+        # Move a dir inside an existing dir on the same filesystem.
+        self._check_move_dir(self.src_dir, self.dst_dir,
+            os.path.join(self.dst_dir, os.path.basename(self.src_dir)))
+
+    def test_move_dir_to_dir_other_fs(self):
+        # Move a dir inside an existing dir on another filesystem.
+        if not self.dir_other_fs:
+            # skip
+            return
+        self._check_move_dir(self.src_dir, self.dir_other_fs,
+            os.path.join(self.dir_other_fs, os.path.basename(self.src_dir)))
+
+    def test_existing_file_inside_dest_dir(self):
+        # A file with the same name inside the destination dir already exists.
+        with open(self.dst_file, "wb"):
+            pass
+        self.assertRaises(shutil.Error, shutil.move, self.src_file, self.dst_dir)
+
+    def test_dont_move_dir_in_itself(self):
+        # Moving a dir inside itself raises an Error.
+        dst = os.path.join(self.src_dir, "bar")
+        self.assertRaises(shutil.Error, shutil.move, self.src_dir, dst)
+
+
 
 def test_main():
-    test_support.run_unittest(TestShutil)
+    test_support.run_unittest(TestShutil, TestMove)
 
 if __name__ == '__main__':
     test_main()

Misc/NEWS

@@ -82,6 +82,9 @@ Library
   On all linux systems the --with-system-ffi configure option defaults
   to "yes".
 
+- Issue 1577: shutil.move() now calls os.rename() if the destination is a
+  directory instead of copying-then-remove-source.
+
 Tests
 -----