bpo-33695 shutil.copytree() + os.scandir() cache #7874
Conversation
|
Note: I explicitly avoided using a context manager around os.scandir() for now so that patch it's easier to review (will add it before pushing). |
Lib/shutil.py
Outdated
| @@ -53,6 +53,10 @@ | |||
| COPY_BUFSIZE = 1024 * 1024 if _WINDOWS else 16 * 1024 | |||
| _HAS_SENDFILE = posix and hasattr(os, "sendfile") | |||
| _HAS_FCOPYFILE = posix and hasattr(posix, "_fcopyfile") # macOS | |||
| _HAS_SAMESTAT = hasattr(os.path, 'samestat') | |||
| _HAS_SAMEFILE = hasattr(os.path, 'samefile') | |||
There was a problem hiding this comment.
Is not it always true?
There was a problem hiding this comment.
Not sure. This was inherited by existing code. Doc says Availability: Unix, Windows. which suggests there may be cases where it's not available?
Lib/shutil.py
Outdated
| if hasattr(os.path, 'samefile'): | ||
| if isinstance(src, os.DirEntry) and _HAS_SAMESTAT: | ||
| try: | ||
| return os.path.samestat(src.stat(), os.stat(dst)) |
There was a problem hiding this comment.
See https://bugs.python.org/issue30480 .
Not always samefile() can be replaced by samestat() on Windows.
There was a problem hiding this comment.
I see, I wasn't aware of that. I'm confused though as it appears the problem should be fixed in samestat() (assuming it's possible), not in here. Also, as per https://bugs.python.org/issue30480 the problem already affects shutil regardless of this PR.
There was a problem hiding this comment.
Code updated. I think this is better left as-is as it just reflects the previous problematic behavior which IMO should be fixed separately.
There was a problem hiding this comment.
@serhiy-storchaka , @vstinner,
thanks for joining the code review; replies to your comments are inline.
Lib/shutil.py
Outdated
| @@ -53,6 +53,10 @@ | |||
| COPY_BUFSIZE = 1024 * 1024 if _WINDOWS else 16 * 1024 | |||
| _HAS_SENDFILE = posix and hasattr(os, "sendfile") | |||
| _HAS_FCOPYFILE = posix and hasattr(posix, "_fcopyfile") # macOS | |||
| _HAS_SAMESTAT = hasattr(os.path, 'samestat') | |||
| _HAS_SAMEFILE = hasattr(os.path, 'samefile') | |||
There was a problem hiding this comment.
Not sure. This was inherited by existing code. Doc says Availability: Unix, Windows. which suggests there may be cases where it's not available?
Lib/shutil.py
Outdated
| if hasattr(os.path, 'samefile'): | ||
| if isinstance(src, os.DirEntry) and _HAS_SAMESTAT: | ||
| try: | ||
| return os.path.samestat(src.stat(), os.stat(dst)) |
There was a problem hiding this comment.
I see, I wasn't aware of that. I'm confused though as it appears the problem should be fixed in samestat() (assuming it's possible), not in here. Also, as per https://bugs.python.org/issue30480 the problem already affects shutil regardless of this PR.
|
Is there anything else I can do regarding this PR? Are we happy? |
…tr() (speedup is neglibigle anyway)
|
@giampaolo: Please replace |
|
@giampaolo Maybe you can also look after my PR #11425 (bpo-35652) because it fixes the behaviour of this PR that the use_srcentry is only set if the original copy function is used. The problem is that a custom edited copy function can't use the srcentry directly. |
https://bugs.python.org/issue33695