Repository: PyFilesystem/pyfilesystem2
Pull request #542 (2022-07-07 13:50:15)

Fix backward incompatibility introduced in 2.4.16 (#535)
## Type of changes

<!-- Remove unrelated categories -->

- Bug fix
- Tests

## Checklist

- [x] I've run the latest [black](https://github.com/ambv/black) with default args on new code.
- [x] I've updated CHANGELOG.md and CONTRIBUTORS.md where appropriate.
- [x] I've added tests for new code.
- [x] I accept that @PyFilesystem/maintainers may be pedantic in the code review.

## Description

Added `overwrite=True` to the optimized path in `fs.move.move_file`. `move_file` now behaves like in versions <2.4.16.


coveralls: 
[![Coverage Status](https://coveralls.io/builds/50674804/badge)](https://coveralls.io/builds/50674804)

Coverage remained the same at 95.128% when pulling **4eb0854f97f51272a5eb3d56cd03b2e94cd5bf19 on tfeldmann:master** into **50b1c9956ee0c674c93ffbf675a7b0f58db2564d on PyFilesystem:master**.

tfeldmann: I found another case where the optimized and the regular code path behave differently:

```python
with open_fs(fs_url) as tmp:
    tmp.writetext("file.txt", "content")
    fs.move.move_file(tmp, "file.txt", tmp, "file.txt")
```

If you move a file onto itself, the optimized code path works as expected, but the regular code path happens to delete the file. This is not wanted, I guess? I added some code to fix this.
lurch: Eeek! If attempting to move a file onto itself, I guess it makes sense to just early-exit? :thinking: 
Wallaku: What stops this PR from being resolved?
@althonos 
@tfeldmann 
lurch: I still think you ought to do `normpath` on `src_path` and `dst_path` before comparing them, in case they contain relative-paths like `foo/../bar` ?
(also, you'll need to rebase this PR to resolve the conflicts)
tfeldmann: Yep, some things left todo on my side. Didn't have the time yet.
lurch: > I found this happens also if you do `some_fs.move("file.txt", "file.txt", overwrite=True)`. It deletes the file.

@tfeldmann Can you create an issue for https://github.com/PyFilesystem/pyfilesystem2/blob/master/fs/base.py#L1138 please?
tfeldmann: > @tfeldmann Can you create an issue for https://github.com/PyFilesystem/pyfilesystem2/blob/master/fs/base.py#L1138 please?

On it!
Wallaku: HOORAY! Good job everybody let's bump a version for this library already!
@lurch 
Wallaku: Anyone? 
@lurch 
@althonos 

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4ffe939..ef16734 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+
 ### Added
 
 - Added `filter_glob` and `exclude_glob` parameters to `fs.walk.Walker`.
@@ -16,6 +17,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Fixed
 - Elaborated documentation of `filter_dirs` and `exclude_dirs` in `fs.walk.Walker`.
   Closes [#371](https://github.com/PyFilesystem/pyfilesystem2/issues/371).
+- Fixes a backward incompatibility where `fs.move.move_file` raises `DestinationExists`
+  ([#535](https://github.com/PyFilesystem/pyfilesystem2/issues/535)).
 - Fixed a bug where files could be truncated or deleted when moved / copied onto itself.
   Closes [#546](https://github.com/PyFilesystem/pyfilesystem2/issues/546)
 
diff --git a/fs/move.py b/fs/move.py
index fdbe96f..752b581 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.


Test patch:
diff --git a/tests/test_move.py b/tests/test_move.py
index 5401082..8eb1af7 100644
--- a/tests/test_move.py
+++ b/tests/test_move.py
@@ -151,6 +151,45 @@ class TestMove(unittest.TestCase):
                 dst_ro.exists("target.txt"), "file should not have been copied over"
             )
 
+    @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"))
+            self.assertFalse(dst.exists("file.txt"))
+            self.assertTrue(dst.exists("target.txt"))
+            self.assertEquals(dst.readtext("target.txt"), "source content")
+
+    @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: