classification
Title: Pathlib.rename isn't robust
Type: Stage: patch review
Components: Library (Lib) Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Oz.Tiram, barneygale
Priority: normal Keywords: patch

Created on 2022-01-09 14:52 by Oz.Tiram, last changed 2022-01-17 19:38 by Oz.Tiram.

Pull Requests
URL Status Linked Edit
PR 30650 open Oz.Tiram, 2022-01-17 19:38
Messages (6)
msg410155 - (view) Author: Oz Tiram (Oz.Tiram) * Date: 2022-01-09 14:52
Pathlib.rename will fail across file system with:

OSError: [Errno 18] Invalid cross-device link

e.g:
-> path_dict["current_path"].rename(path_dict["destination"])
(Pdb) n
OSError: [Errno 18] Invalid cross-device link: '/tmp/pipenv-k1m0oynt-yaml/PyYAML-6.0/lib/yaml' -> '/home/oz123/Software/pipenv/pipenv/patched/yaml3'

This is because it uses os.rename under the hood:
https://github.com/python/cpython/blob/3.10/Lib/pathlib.py#L306

One can either replace it with `shutil.move` which works, or one could
add another method to Pathlib.move(...) with similar signature and return value, which calls `shutil.move` under the hood.

Before submitting a patch for that, I would like to get feedback for that.
msg410590 - (view) Author: Barney Gale (barneygale) * Date: 2022-01-14 19:17
Sounds good. Would you expose the `copy_function` argument in pathlib, or do something else (like `metadata=True`)?
msg410593 - (view) Author: Oz Tiram (Oz.Tiram) * Date: 2022-01-14 19:45
@barney, I am not sure that I understand your question.

I think adding another method `Pathlib.Path` and `Pathlib._Accessor` is my preferred way. The would be something like:

    class _NormalAccessor(_Accessor):
       ...
       self.move = shutil.move
    

    class Path:
       ....

       def move(self, src, dest):
          self._accessor.move(self, target)
          return self.__class__(target)


Now, this is hardly a patch. I need to submit a PR with proper docs, tests and NEWS entry... I will be glad to work on it. However, I guess I need someone to "sponsor" it and merge it.
msg410595 - (view) Author: Barney Gale (barneygale) * Date: 2022-01-14 19:59
shutil.move() accepts a `copy_function` argument:

    shutil.move(src, dst, copy_function=copy2)

It's possible to set `copy_function=copy` to skip copying file metadata.
msg410648 - (view) Author: Oz Tiram (Oz.Tiram) * Date: 2022-01-15 14:29
Thanks for the answer, it makes sense now. Yes, I would adopt this.
Allowing users to use `copy2` (or any other functio ...) using a keyword.
msg410649 - (view) Author: Barney Gale (barneygale) * Date: 2022-01-15 15:08
Fair enough. Users who wanted to avoid copying file metadata would then do something like this, I suppose?

    import pathlib
    import shutil

    path = pathlib.Path('foo')
    path.move('bar', copy_function=shutil.copy)

I guess the downside here is that users would still need to `import shutil` to do this. But I see the utility of allowing any copy_function to be supplied!
History
Date User Action Args
2022-01-17 19:38:48Oz.Tiramsetkeywords: + patch
stage: patch review
pull_requests: + pull_request28851
2022-01-15 15:08:46barneygalesetmessages: + msg410649
2022-01-15 14:29:48Oz.Tiramsetmessages: + msg410648
2022-01-14 19:59:17barneygalesetmessages: + msg410595
2022-01-14 19:45:01Oz.Tiramsetmessages: + msg410593
2022-01-14 19:17:02barneygalesetnosy: + barneygale
messages: + msg410590
2022-01-09 14:52:56Oz.Tiramcreate