From 6763644ad976ad9e2ccb229b5d45ec88f9d4ad44 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Thu, 7 Jul 2022 15:42:40 +0200 Subject: [PATCH 1/7] add default overwrite arg (fixes #535) --- fs/move.py | 7 ++++++- tests/test_move.py | 8 ++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/fs/move.py b/fs/move.py index fdbe96fe..752b5816 100644 --- a/fs/move.py +++ b/fs/move.py @@ -82,7 +82,12 @@ def move_file( rel_dst = frombase(common, dst_syspath) with _src_fs.lock(), _dst_fs.lock(): with OSFS(common) as base: - base.move(rel_src, rel_dst, preserve_time=preserve_time) + base.move( + rel_src, + rel_dst, + overwrite=True, + preserve_time=preserve_time, + ) return # optimization worked, exit early except ValueError: # This is raised if we cannot find a common base folder. diff --git a/tests/test_move.py b/tests/test_move.py index 5401082e..3692d06d 100644 --- a/tests/test_move.py +++ b/tests/test_move.py @@ -151,6 +151,14 @@ def test_move_file_read_only_mem_dest(self): dst_ro.exists("target.txt"), "file should not have been copied over" ) + def test_overwrite(self): + with open_fs("temp://") as src, open_fs("temp://") as dst: + src.writetext("file.txt", "Content") + dst.writetext("target.txt", "Content") + fs.move.move_file(src, "file.txt", dst, "target.txt") + self.assertFalse(src.exists("file.txt")) + self.assertTrue(dst.exists("target.txt")) + @parameterized.expand([(True,), (False,)]) def test_move_file_cleanup_on_error(self, cleanup): with open_fs("mem://") as src, open_fs("mem://") as dst: From 4eb0854f97f51272a5eb3d56cd03b2e94cd5bf19 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Thu, 7 Jul 2022 15:47:32 +0200 Subject: [PATCH 2/7] update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 267c942b..10c794b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +- Fixes a backward incompatibility where `fs.move.move_file` raises `DestinationExists` + ([#535](https://github.com/PyFilesystem/pyfilesystem2/issues/535)). + ## [2.4.16] - 2022-05-02 From 17575bac0cdd7ea2cd6738828306ede0ec57f7ab Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 8 Jul 2022 11:34:20 +0200 Subject: [PATCH 3/7] add tests --- tests/test_move.py | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/tests/test_move.py b/tests/test_move.py index 3692d06d..3cb841ed 100644 --- a/tests/test_move.py +++ b/tests/test_move.py @@ -151,13 +151,29 @@ def test_move_file_read_only_mem_dest(self): dst_ro.exists("target.txt"), "file should not have been copied over" ) - def test_overwrite(self): - with open_fs("temp://") as src, open_fs("temp://") as dst: - src.writetext("file.txt", "Content") - dst.writetext("target.txt", "Content") + @parameterized.expand([("temp://",), ("mem://",)]) + def test_move_file_overwrite(self, fs_url): + # we use TempFS and MemoryFS in order to make sure the optimised code path + # behaves like the regular one + with open_fs(fs_url) as src, open_fs(fs_url) as dst: + src.writetext("file.txt", "source content") + dst.writetext("target.txt", "target content") fs.move.move_file(src, "file.txt", dst, "target.txt") self.assertFalse(src.exists("file.txt")) + 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") + + @parameterized.expand([("temp://",), ("mem://",)]) + def test_move_file_overwrite_itself(self, fs_url): + # we use TempFS and MemoryFS in order to make sure the optimised code path + # behaves like the regular one + with open_fs(fs_url) as tmp: + 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") @parameterized.expand([(True,), (False,)]) def test_move_file_cleanup_on_error(self, cleanup): From bdfc7743ce9dd4b0fdeecccc06db22189d310066 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Fri, 8 Jul 2022 11:34:39 +0200 Subject: [PATCH 4/7] fix deletion when moving file on itself --- fs/move.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/move.py b/fs/move.py index 752b5816..49f93483 100644 --- a/fs/move.py +++ b/fs/move.py @@ -64,6 +64,9 @@ def move_file( with manage_fs(src_fs, writeable=True) as _src_fs: with manage_fs(dst_fs, writeable=True, create=True) as _dst_fs: if _src_fs is _dst_fs: + if src_path == dst_path: + return + # Same filesystem, may be optimized _src_fs.move( src_path, dst_path, overwrite=True, preserve_time=preserve_time From 863dc8322c71c9c61db5a9a8285605108546411e Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Thu, 21 Jul 2022 13:19:35 +0200 Subject: [PATCH 5/7] compare normalized pathes in early exit --- fs/move.py | 5 +++-- tests/test_move.py | 31 +++++++++++++++++++++++-------- 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/fs/move.py b/fs/move.py index 49f93483..d2ac8add 100644 --- a/fs/move.py +++ b/fs/move.py @@ -10,7 +10,7 @@ from .errors import FSError from .opener import manage_fs from .osfs import OSFS -from .path import frombase +from .path import frombase, normpath if typing.TYPE_CHECKING: from typing import Text, Union @@ -64,7 +64,8 @@ def move_file( with manage_fs(src_fs, writeable=True) as _src_fs: with manage_fs(dst_fs, writeable=True, create=True) as _dst_fs: if _src_fs is _dst_fs: - if src_path == dst_path: + # Exit early if source and destination are the same file + if normpath(src_path) == normpath(dst_path): return # Same filesystem, may be optimized diff --git a/tests/test_move.py b/tests/test_move.py index 3cb841ed..8eb1af75 100644 --- a/tests/test_move.py +++ b/tests/test_move.py @@ -151,13 +151,17 @@ def test_move_file_read_only_mem_dest(self): dst_ro.exists("target.txt"), "file should not have been copied over" ) - @parameterized.expand([("temp://",), ("mem://",)]) - def test_move_file_overwrite(self, fs_url): - # we use TempFS and MemoryFS in order to make sure the optimised code path - # behaves like the regular one + @parameterized.expand([("temp", "temp://"), ("mem", "mem://")]) + def test_move_file_overwrite(self, _, fs_url): + # we use TempFS and MemoryFS in order to make sure the optimized code path + # behaves like the regular one (TempFS tests the optmized code path). with open_fs(fs_url) as src, open_fs(fs_url) as dst: src.writetext("file.txt", "source content") dst.writetext("target.txt", "target content") + self.assertTrue(src.exists("file.txt")) + self.assertFalse(src.exists("target.txt")) + self.assertFalse(dst.exists("file.txt")) + self.assertTrue(dst.exists("target.txt")) fs.move.move_file(src, "file.txt", dst, "target.txt") self.assertFalse(src.exists("file.txt")) self.assertFalse(src.exists("target.txt")) @@ -165,16 +169,27 @@ def test_move_file_overwrite(self, fs_url): self.assertTrue(dst.exists("target.txt")) self.assertEquals(dst.readtext("target.txt"), "source content") - @parameterized.expand([("temp://",), ("mem://",)]) - def test_move_file_overwrite_itself(self, fs_url): - # we use TempFS and MemoryFS in order to make sure the optimised code path - # behaves like the regular one + @parameterized.expand([("temp", "temp://"), ("mem", "mem://")]) + def test_move_file_overwrite_itself(self, _, fs_url): + # we use TempFS and MemoryFS in order to make sure the optimized code path + # behaves like the regular one (TempFS tests the optmized code path). with open_fs(fs_url) as tmp: 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") + @parameterized.expand([("temp", "temp://"), ("mem", "mem://")]) + def test_move_file_overwrite_itself_relpath(self, _, fs_url): + # we use TempFS and MemoryFS in order to make sure the optimized code path + # behaves like the regular one (TempFS tests the optmized code path). + with open_fs(fs_url) as tmp: + new_dir = tmp.makedir("dir") + 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") + @parameterized.expand([(True,), (False,)]) def test_move_file_cleanup_on_error(self, cleanup): with open_fs("mem://") as src, open_fs("mem://") as dst: From 38e6b3cf524d714541b52f8d69e9f87d9699490e Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Tue, 2 Aug 2022 13:07:22 +0200 Subject: [PATCH 6/7] check no longer needed --- fs/move.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/fs/move.py b/fs/move.py index d2ac8add..f06e7057 100644 --- a/fs/move.py +++ b/fs/move.py @@ -64,10 +64,6 @@ def move_file( with manage_fs(src_fs, writeable=True) as _src_fs: with manage_fs(dst_fs, writeable=True, create=True) as _dst_fs: if _src_fs is _dst_fs: - # Exit early if source and destination are the same file - if normpath(src_path) == normpath(dst_path): - return - # Same filesystem, may be optimized _src_fs.move( src_path, dst_path, overwrite=True, preserve_time=preserve_time From 93c2579f14fab4261c45178ccf5689e05f326426 Mon Sep 17 00:00:00 2001 From: Thomas Feldmann Date: Tue, 2 Aug 2022 13:10:26 +0200 Subject: [PATCH 7/7] remove unused import --- fs/move.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/move.py b/fs/move.py index f06e7057..752b5816 100644 --- a/fs/move.py +++ b/fs/move.py @@ -10,7 +10,7 @@ from .errors import FSError from .opener import manage_fs from .osfs import OSFS -from .path import frombase, normpath +from .path import frombase if typing.TYPE_CHECKING: from typing import Text, Union