bpo-33695 shutil.copytree() + os.scandir() cache by giampaolo · Pull Request #7874 · python/cpython

bpo-33695 shutil.copytree() + os.scandir() cache by giampaolo · Pull Request #7874 · python/cpython

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).

giampaolo changed the title ~~bpo-33695 shutil os.scandir() cache~~ bpo-33695 shutil.copytree() + os.scandir() cache

		_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')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is not it always true?

Choose a reason for 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))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for 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.

Choose a reason for 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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

		_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')

Choose a reason for 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))

Choose a reason for 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 # with GH- in the commit message next time. Thanks!

ghost mentioned this pull request

@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.