Pathlib.rename isn't robust #90475

Activity

1. oz123 commented on Jan 9, 2022

   

   oz123mannequin

   on Jan 9, 2022

   MannequinAuthor

   More actions

   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.
2. oz123

   added

   3.9 (EOL)end of lifeend of life

   3.10 (EOL)end of lifeend of life

   3.11only security fixesonly security fixes

   stdlibStandard Library Python modules in the Lib/ directoryStandard Library Python modules in the Lib/ directory

   on Jan 9, 2022
3. barneygale commented on Jan 14, 2022

   

   barneygalemannequin

   on Jan 14, 2022

   Mannequin

   More actions

   Sounds good. Would you expose the copy_function argument in pathlib, or do something else (like metadata=True)?
4. oz123 commented on Jan 14, 2022

   

   oz123mannequin

   on Jan 14, 2022

   MannequinAuthor

   More actions

   @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.
5. barneygale commented on Jan 14, 2022

   

   barneygalemannequin

   on Jan 14, 2022

   Mannequin

   More actions

   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.
6. oz123 commented on Jan 15, 2022

   

   oz123mannequin

   on Jan 15, 2022

   MannequinAuthor

   More actions

   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.
7. barneygale commented on Jan 15, 2022

   

   barneygalemannequin

   on Jan 15, 2022

   Mannequin

   More actions

   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!
8. ezio-melotti

   transferred this issue fromon Apr 10, 2022
9. brettcannon

   added

   topic-pathlib

   on Jun 21, 2022
10. barneygale commented on Apr 14, 2023

    

    barneygale

    on Apr 14, 2023

    Contributor

    More actions

    Hey @oz123, I've just spotted that this issue is a duplicate of #73991. There's some more discussion there. I'll resolve this issue as a duplicate, and re-title your PR. Hope that's alright, thanks!

    👍React with 👍1Reacted by Oz Tiram
11. barneygale

    closed this as not plannedon Apr 14, 2023