From fb0b35a5c69cc762f8ea18f253d467d03b03b04a Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Tue, 30 Aug 2022 14:45:23 +0200 Subject: [PATCH] add optimization --- CHANGELOG.md | 2 ++ fs/move.py | 39 +++++++++++++++++++++++++++++++-------- tests/test_move.py | 20 +++++++++++++++++--- 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ef16734e..3823095c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +- Optimized moving folders between filesystems with syspaths + ([#550](https://github.com/PyFilesystem/pyfilesystem2/pull/550)) ### Added diff --git a/fs/move.py b/fs/move.py index 752b5816..f68a4e1a 100644 --- a/fs/move.py +++ b/fs/move.py @@ -5,12 +5,15 @@ import typing +import os + from ._pathcompat import commonpath from .copy import copy_dir, copy_file -from .errors import FSError +from .error_tools import convert_os_errors +from .errors import DirectoryExpected, FSError, IllegalDestination, ResourceNotFound from .opener import manage_fs from .osfs import OSFS -from .path import frombase +from .path import frombase, isbase if typing.TYPE_CHECKING: from typing import Text, Union @@ -26,7 +29,6 @@ def move_fs( ): # type: (...) -> None """Move the contents of a filesystem to another filesystem. - Arguments: src_fs (FS or str): Source filesystem (instance or URL). dst_fs (FS or str): Destination filesystem (instance or URL). @@ -34,7 +36,6 @@ def move_fs( a single-threaded copy. preserve_time (bool): If `True`, try to preserve mtime of the resources (defaults to `False`). - """ move_dir(src_fs, "/", dst_fs, "/", workers=workers, preserve_time=preserve_time) @@ -49,7 +50,6 @@ def move_file( ): # type: (...) -> None """Move a file from one filesystem to another. - Arguments: src_fs (FS or str): Source filesystem (instance or URL). src_path (str): Path to a file on ``src_fs``. @@ -59,7 +59,6 @@ def move_file( resources (defaults to `False`). cleanup_dst_on_error (bool): If `True`, tries to delete the file copied to ``dst_fs`` if deleting the file from ``src_fs`` fails (defaults to `True`). - """ with manage_fs(src_fs, writeable=True) as _src_fs: with manage_fs(dst_fs, writeable=True, create=True) as _dst_fs: @@ -123,7 +122,6 @@ def move_dir( ): # type: (...) -> None """Move a directory from one filesystem to another. - Arguments: src_fs (FS or str): Source filesystem (instance or URL). src_path (str): Path to a directory on ``src_fs`` @@ -133,10 +131,35 @@ def move_dir( (default) for a single-threaded copy. preserve_time (bool): If `True`, try to preserve mtime of the resources (defaults to `False`). - + Raises: + fs.errors.ResourceNotFound: if ``src_path`` does not exist on `src_fs` + fs.errors.DirectoryExpected: if ``src_path`` or one of its + ancestors is not a directory. + fs.errors.IllegalDestination: when moving a folder into itself """ with manage_fs(src_fs, writeable=True) as _src_fs: with manage_fs(dst_fs, writeable=True, create=True) as _dst_fs: + if not _src_fs.exists(src_path): + raise ResourceNotFound(src_path) + if not _src_fs.isdir(src_path): + raise DirectoryExpected(src_path) + + # if both filesystems have a syspath we use `os.rename` to move the folder + if _src_fs.hassyspath(src_path) and _dst_fs.hassyspath(dst_path): + src_syspath = _src_fs.getsyspath(src_path) + dst_syspath = _dst_fs.getsyspath(dst_path) + # recheck if the move operation is legal using the syspaths + if isbase(src_syspath, dst_syspath): + raise IllegalDestination(dst_path) + with _src_fs.lock(), _dst_fs.lock(): + with convert_os_errors("move_dir", src_path, directory=True): + os.rename(src_syspath, dst_syspath) + # recreate the root dir if it has been renamed + if src_path == "/": + _src_fs.makedir("/") + return # optimization worked, exit early + + # standard copy then delete with _src_fs.lock(), _dst_fs.lock(): _dst_fs.makedir(dst_path, recreate=True) copy_dir( diff --git a/tests/test_move.py b/tests/test_move.py index 8eb1af75..0aeb9631 100644 --- a/tests/test_move.py +++ b/tests/test_move.py @@ -7,6 +7,7 @@ except ImportError: import mock +import os from parameterized import parameterized, parameterized_class import fs.move @@ -167,7 +168,7 @@ def test_move_file_overwrite(self, _, fs_url): self.assertFalse(src.exists("target.txt")) self.assertFalse(dst.exists("file.txt")) self.assertTrue(dst.exists("target.txt")) - self.assertEquals(dst.readtext("target.txt"), "source content") + self.assertEqual(dst.readtext("target.txt"), "source content") @parameterized.expand([("temp", "temp://"), ("mem", "mem://")]) def test_move_file_overwrite_itself(self, _, fs_url): @@ -177,7 +178,7 @@ def test_move_file_overwrite_itself(self, _, fs_url): tmp.writetext("file.txt", "content") fs.move.move_file(tmp, "file.txt", tmp, "file.txt") self.assertTrue(tmp.exists("file.txt")) - self.assertEquals(tmp.readtext("file.txt"), "content") + self.assertEqual(tmp.readtext("file.txt"), "content") @parameterized.expand([("temp", "temp://"), ("mem", "mem://")]) def test_move_file_overwrite_itself_relpath(self, _, fs_url): @@ -188,7 +189,7 @@ def test_move_file_overwrite_itself_relpath(self, _, fs_url): new_dir.writetext("file.txt", "content") fs.move.move_file(tmp, "dir/../dir/file.txt", tmp, "dir/file.txt") self.assertTrue(tmp.exists("dir/file.txt")) - self.assertEquals(tmp.readtext("dir/file.txt"), "content") + self.assertEqual(tmp.readtext("dir/file.txt"), "content") @parameterized.expand([(True,), (False,)]) def test_move_file_cleanup_on_error(self, cleanup): @@ -206,3 +207,16 @@ def test_move_file_cleanup_on_error(self, cleanup): ) self.assertTrue(src.exists("file.txt")) self.assertEqual(not dst.exists("target.txt"), cleanup) + + @parameterized.expand([("temp", "temp://", True), ("mem", "mem://", False)]) + def test_move_dir_optimized(self, _, fs_url, mv_called): + with open_fs(fs_url) as tmp: + with mock.patch.object(os, "rename", wraps=os.rename) as mv: + dir_ = tmp.makedir("dir") + dir_.writetext("file.txt", "content") + sub = dir_.makedir("sub") + sub.writetext("file.txt", "sub content") + fs.move.move_dir(tmp, "dir", tmp, "newdir") + self.assertEqual(tmp.readtext("newdir/file.txt"), "content") + self.assertEqual(tmp.readtext("newdir/sub/file.txt"), "sub content") + self.assertEqual(mv.called, mv_called)