pyfilesystem2 icon indicating copy to clipboard operation
pyfilesystem2 copied to clipboard

OSFS.movedir could probably use os.rename

Open willmcgugan opened this issue 6 years ago • 10 comments

It should be possible to implement OSFS.movedir with an os.rename call, to avoid unnecessary copying.

willmcgugan avatar Dec 25 '18 22:12 willmcgugan

@willmcgugan I would like to work on this and implement the required functionality

varunu28 avatar Dec 28 '18 02:12 varunu28

Hi @varunu28 I would be happy to accept a PR!

willmcgugan avatar Dec 28 '18 13:12 willmcgugan

@varunu28 I'd be happy to test this in my environment. We use Symlinks, SMB shares, and DFS referrals in various places. This means that sometimes a move operation looks like it is within the same filesystem, when in fact it crosses filesystems.

geoffjukes avatar Dec 29 '18 21:12 geoffjukes

@geoffjukes I'm hoping that in those cases os.rename will fail and we can fall back to the default copy+delete

willmcgugan avatar Dec 29 '18 21:12 willmcgugan

All the docs say is

The operation may fail on some Unix flavors if src and dst are on different filesystems

I have had os.rename operations act funny across DFS referrals, because the OS sees them as being the same filesystem. Happy to test tho.

geoffjukes avatar Dec 29 '18 21:12 geoffjukes

@geoffjukes @willmcgugan Can an alternative be to use shutil.move in place of os.rename?

Also correct me if I am wrong, but in both os.rename and shutil.move , I would have to use src_path and dst_path as parameters as they do not take filesystem objects as a parameter?

varunu28 avatar Dec 30 '18 00:12 varunu28

Hi @varunu28

Sorry for the delay. So shutil.move has the rename functionality, so that should work. Bear in mind it would still need to behave in exactly the way that OSFS.movedir does at the moment (as well as identical exceptions).

Also correct me if I am wrong, but in both os.rename and shutil.move , I would have to use src_path and dst_path as parameters as they do not take filesystem objects as a parameter?

movedir takes FS Paths, you would need to translate those to syspaths, to do the copy.

willmcgugan avatar Jan 05 '19 16:01 willmcgugan

@willmcgugan Thanks for the clarification. I will go ahead with using shutil

varunu28 avatar Jan 08 '19 02:01 varunu28

I was just about to report this as an issue, when I saw it already existed ;-) Any progress @varunu28 ?

@willmcgugan Given that https://pyfilesystem2.readthedocs.io/en/latest/reference/base.html#fs.base.FS.move raises an exception if src_path is a directory, should https://pyfilesystem2.readthedocs.io/en/latest/reference/base.html#fs.base.FS.movedir raise an exception if src_path is a file? And should movedir also raise a DestinationExists error?

lurch avatar Aug 09 '19 08:08 lurch

Hey, was wondering if there was any update on this? Upgrading from pyfs1, and the new behavior of copy -> delete introduced errors in tests. I've patched it on our side for now, but would be nice to have movedir perform os.rename natively.

dsoulis avatar Oct 31 '23 22:10 dsoulis