Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
|
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). |
| @@ -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.
The reason will be displayed to describe this comment to others. Learn more.
Is not it always true?
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Not sure. This was inherited by existing code. Doc says Availability: Unix, Windows. which suggests there may be cases where it's not available?
| 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.
The reason will be displayed to describe this comment to others. Learn more.
See https://bugs.python.org/issue30480 .
Not always samefile() can be replaced by samestat() on Windows.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
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.
The reason will be displayed to describe this comment to others. Learn more.
Code updated. I think this is better left as-is as it just reflects the previous problematic behavior which IMO should be fixed separately.
@serhiy-storchaka , @vstinner,
thanks for joining the code review; replies to your comments are inline.
| @@ -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.
The reason will be displayed to describe this comment to others. Learn more.
Not sure. This was inherited by existing code. Doc says Availability: Unix, Windows. which suggests there may be cases where it's not available?
| 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.
The reason will be displayed to describe this comment to others. Learn more.
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