GH-73991: Rework pathlib.Path.copytree() into copy() (#122369)

barneygale · AA-Turner · web-flow · commit a6644d446416 · 2024-08-11T22:43:18.000+01:00
Rename `pathlib.Path.copy()` to `_copy_file()` (i.e. make it private.)

Rename `pathlib.Path.copytree()` to `copy()`, and add support for copying
non-directories. This simplifies the interface for users, and nicely
complements the upcoming `move()` and `delete()` methods (which will also
accept any type of file.)

Co-authored-by: Adam Turner &lt;9087854+AA-Turner@users.noreply.github.com&gt;
diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst
@@ -1539,50 +1539,33 @@ Creating files and directories
 Copying, renaming and deleting
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
-.. method:: Path.copy(target, *, follow_symlinks=True, preserve_metadata=False)
+.. method:: Path.copy(target, *, follow_symlinks=True, dirs_exist_ok=False, \
+                      preserve_metadata=False, ignore=None, on_error=None)
 
-   Copy the contents of this file to the *target* file. If *target* specifies
-   a file that already exists, it will be replaced.
+   Copy this file or directory tree to the given *target*, and return a new
+   :class:`!Path` instance pointing to *target*.
 
-   If *follow_symlinks* is false, and this file is a symbolic link, *target*
-   will be created as a symbolic link. If *follow_symlinks* is true and this
-   file is a symbolic link, *target* will be a copy of the symlink target.
+   If the source is a file, the target will be replaced if it is an existing
+   file. If the source is a symlink and *follow_symlinks* is true (the
+   default), the symlink's target is copied. Otherwise, the symlink is
+   recreated at the destination.
 
-   If *preserve_metadata* is false (the default), only the file data is
-   guaranteed to be copied. Set *preserve_metadata* to true to ensure that the
-   file mode (permissions), flags, last access and modification times, and
-   extended attributes are copied where supported. This argument has no effect
-   on Windows, where metadata is always preserved when copying.
+   If the source is a directory and *dirs_exist_ok* is false (the default), a
+   :exc:`FileExistsError` is raised if the target is an existing directory.
+   If *dirs_exists_ok* is true, the copying operation will overwrite
+   existing files within the destination tree with corresponding files
+   from the source tree.
 
-   .. versionadded:: 3.14
-
-
-.. method:: Path.copytree(target, *, follow_symlinks=True, \
-                          preserve_metadata=False, dirs_exist_ok=False, \
-                          ignore=None, on_error=None)
-
-   Recursively copy this directory tree to the given destination.
-
-   If a symlink is encountered in the source tree, and *follow_symlinks* is
-   true (the default), the symlink's target is copied. Otherwise, the symlink
-   is recreated in the destination tree.
-
-   If *preserve_metadata* is false (the default), only the directory structure
+   If *preserve_metadata* is false (the default), only directory structures
    and file data are guaranteed to be copied. Set *preserve_metadata* to true
    to ensure that file and directory permissions, flags, last access and
    modification times, and extended attributes are copied where supported.
-   This argument has no effect on Windows, where metadata is always preserved
-   when copying.
-
-   If the destination is an existing directory and *dirs_exist_ok* is false
-   (the default), a :exc:`FileExistsError` is raised. Otherwise, the copying
-   operation will continue if it encounters existing directories, and files
-   within the destination tree will be overwritten by corresponding files from
-   the source tree.
+   This argument has no effect when copying files on Windows (where
+   metadata is always preserved).
 
    If *ignore* is given, it should be a callable accepting one argument: a
-   file or directory path within the source tree. The callable may return true
-   to suppress copying of the path.
+   source file or directory path. The callable may return true to suppress
+   copying of the path.
 
    If *on_error* is given, it should be a callable accepting one argument: an
    instance of :exc:`OSError`. The callable may re-raise the exception or do
diff --git a/Doc/whatsnew/3.14.rst b/Doc/whatsnew/3.14.rst
@@ -146,10 +146,8 @@ pathlib
 
 * Add methods to :class:`pathlib.Path` to recursively copy or remove files:
 
-  * :meth:`~pathlib.Path.copy` copies the content of one file to another, like
-    :func:`shutil.copyfile`.
-  * :meth:`~pathlib.Path.copytree` copies one directory tree to another, like
-    :func:`shutil.copytree`.
+  * :meth:`~pathlib.Path.copy` copies a file or directory tree to a given
+    destination.
   * :meth:`~pathlib.Path.delete` removes a file or directory tree.
 
   (Contributed by Barney Gale in :gh:`73991`.)
diff --git a/Lib/pathlib/__init__.py b/Lib/pathlib/__init__.py
@@ -5,8 +5,8 @@
 operating systems.
 """
 
-from ._os import *
-from ._local import *
+from pathlib._abc import *
+from pathlib._local import *
 
-__all__ = (_os.__all__ +
+__all__ = (_abc.__all__ +
            _local.__all__)
diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py
@@ -16,7 +16,16 @@
 import posixpath
 from glob import _GlobberBase, _no_recurse_symlinks
 from stat import S_ISDIR, S_ISLNK, S_ISREG, S_ISSOCK, S_ISBLK, S_ISCHR, S_ISFIFO
-from ._os import UnsupportedOperation, copyfileobj
+from pathlib._os import copyfileobj
+
+
+__all__ = ["UnsupportedOperation"]
+
+
+class UnsupportedOperation(NotImplementedError):
+    """An exception that is raised when an unsupported operation is attempted.
+    """
+    pass
 
 
 @functools.cache
@@ -761,6 +770,13 @@ def symlink_to(self, target, target_is_directory=False):
         """
         raise UnsupportedOperation(self._unsupported_msg('symlink_to()'))
 
+    def _symlink_to_target_of(self, link):
+        """
+        Make this path a symlink with the same target as the given link. This
+        is used by copy().
+        """
+        self.symlink_to(link.readlink())
+
     def hardlink_to(self, target):
         """
         Make this path a hard link pointing to the same file as *target*.
@@ -806,21 +822,12 @@ def _copy_metadata(self, target, *, follow_symlinks=True):
             metadata = self._read_metadata(keys, follow_symlinks=follow_symlinks)
             target._write_metadata(metadata, follow_symlinks=follow_symlinks)
 
-    def copy(self, target, *, follow_symlinks=True, preserve_metadata=False):
+    def _copy_file(self, target):
         """
-        Copy the contents of this file to the given target. If this file is a
-        symlink and follow_symlinks is false, a symlink will be created at the
-        target.
+        Copy the contents of this file to the given target.
         """
-        if not isinstance(target, PathBase):
-            target = self.with_segments(target)
         if self._samefile_safe(target):
             raise OSError(f"{self!r} and {target!r} are the same file")
-        if not follow_symlinks and self.is_symlink():
-            target.symlink_to(self.readlink())
-            if preserve_metadata:
-                self._copy_metadata(target, follow_symlinks=False)
-            return
         with self.open('rb') as source_f:
             try:
                 with target.open('wb') as target_f:
@@ -832,42 +839,39 @@ def copy(self, target, *, follow_symlinks=True, preserve_metadata=False):
                         f'Directory does not exist: {target}') from e
                 else:
                     raise
-        if preserve_metadata:
-            self._copy_metadata(target)
 
-    def copytree(self, target, *, follow_symlinks=True,
-                 preserve_metadata=False, dirs_exist_ok=False,
-                 ignore=None, on_error=None):
+    def copy(self, target, *, follow_symlinks=True, dirs_exist_ok=False,
+             preserve_metadata=False, ignore=None, on_error=None):
         """
-        Recursively copy this directory tree to the given destination.
+        Recursively copy this file or directory tree to the given destination.
         """
         if not isinstance(target, PathBase):
             target = self.with_segments(target)
-        if on_error is None:
-            def on_error(err):
-                raise err
         stack = [(self, target)]
         while stack:
-            source_dir, target_dir = stack.pop()
+            src, dst = stack.pop()
             try:
-                sources = source_dir.iterdir()
-                target_dir.mkdir(exist_ok=dirs_exist_ok)
-                if preserve_metadata:
-                    source_dir._copy_metadata(target_dir)
-                for source in sources:
-                    if ignore and ignore(source):
-                        continue
-                    try:
-                        if source.is_dir(follow_symlinks=follow_symlinks):
-                            stack.append((source, target_dir.joinpath(source.name)))
-                        else:
-                            source.copy(target_dir.joinpath(source.name),
-                                        follow_symlinks=follow_symlinks,
-                                        preserve_metadata=preserve_metadata)
-                    except OSError as err:
-                        on_error(err)
+                if not follow_symlinks and src.is_symlink():
+                    dst._symlink_to_target_of(src)
+                    if preserve_metadata:
+                        src._copy_metadata(dst, follow_symlinks=False)
+                elif src.is_dir():
+                    children = src.iterdir()
+                    dst.mkdir(exist_ok=dirs_exist_ok)
+                    for child in children:
+                        if not (ignore and ignore(child)):
+                            stack.append((child, dst.joinpath(child.name)))
+                    if preserve_metadata:
+                        src._copy_metadata(dst)
+                else:
+                    src._copy_file(dst)
+                    if preserve_metadata:
+                        src._copy_metadata(dst)
             except OSError as err:
+                if on_error is None:
+                    raise
                 on_error(err)
+        return target
 
     def rename(self, target):
         """
diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py
@@ -18,9 +18,9 @@
 except ImportError:
     grp = None
 
-from ._os import (UnsupportedOperation, copyfile, file_metadata_keys,
-                  read_file_metadata, write_file_metadata)
-from ._abc import PurePathBase, PathBase
+from pathlib._os import (copyfile, file_metadata_keys, read_file_metadata,
+                         write_file_metadata)
+from pathlib._abc import UnsupportedOperation, PurePathBase, PathBase
 
 
 __all__ = [
@@ -788,25 +788,18 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False):
     _write_metadata = write_file_metadata
 
     if copyfile:
-        def copy(self, target, *, follow_symlinks=True, preserve_metadata=False):
+        def _copy_file(self, target):
             """
-            Copy the contents of this file to the given target. If this file is a
-            symlink and follow_symlinks is false, a symlink will be created at the
-            target.
+            Copy the contents of this file to the given target.
             """
             try:
                 target = os.fspath(target)
             except TypeError:
                 if not isinstance(target, PathBase):
                     raise
+                PathBase._copy_file(self, target)
             else:
-                try:
-                    copyfile(os.fspath(self), target, follow_symlinks)
-                    return
-                except UnsupportedOperation:
-                    pass  # Fall through to generic code.
-            PathBase.copy(self, target, follow_symlinks=follow_symlinks,
-                          preserve_metadata=preserve_metadata)
+                copyfile(os.fspath(self), target)
 
     def chmod(self, mode, *, follow_symlinks=True):
         """
@@ -894,6 +887,14 @@ def symlink_to(self, target, target_is_directory=False):
             """
             os.symlink(target, self, target_is_directory)
 
+    if os.name == 'nt':
+        def _symlink_to_target_of(self, link):
+            """
+            Make this path a symlink with the same target as the given link.
+            This is used by copy().
+            """
+            self.symlink_to(link.readlink(), link.is_dir())
+
     if hasattr(os, "link"):
         def hardlink_to(self, target):
             """
diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py
@@ -20,15 +20,6 @@
     _winapi = None
 
 
-__all__ = ["UnsupportedOperation"]
-
-
-class UnsupportedOperation(NotImplementedError):
-    """An exception that is raised when an unsupported operation is attempted.
-    """
-    pass
-
-
 def get_copy_blocksize(infd):
     """Determine blocksize for fastcopying on Linux.
     Hopefully the whole file will be copied in a single call.
@@ -101,44 +92,12 @@ def copyfd(source_fd, target_fd):
     copyfd = None
 
 
-if _winapi and hasattr(_winapi, 'CopyFile2') and hasattr(os.stat_result, 'st_file_attributes'):
-    def _is_dirlink(path):
-        try:
-            st = os.lstat(path)
-        except (OSError, ValueError):
-            return False
-        return (st.st_file_attributes & stat.FILE_ATTRIBUTE_DIRECTORY and
-                st.st_reparse_tag == stat.IO_REPARSE_TAG_SYMLINK)
-
-    def copyfile(source, target, follow_symlinks):
+if _winapi and hasattr(_winapi, 'CopyFile2'):
+    def copyfile(source, target):
         """
         Copy from one file to another using CopyFile2 (Windows only).
         """
-        if follow_symlinks:
-            _winapi.CopyFile2(source, target, 0)
-        else:
-            # Use COPY_FILE_COPY_SYMLINK to copy a file symlink.
-            flags = _winapi.COPY_FILE_COPY_SYMLINK
-            try:
-                _winapi.CopyFile2(source, target, flags)
-                return
-            except OSError as err:
-                # Check for ERROR_ACCESS_DENIED
-                if err.winerror == 5 and _is_dirlink(source):
-                    pass
-                else:
-                    raise
-
-            # Add COPY_FILE_DIRECTORY to copy a directory symlink.
-            flags |= _winapi.COPY_FILE_DIRECTORY
-            try:
-                _winapi.CopyFile2(source, target, flags)
-            except OSError as err:
-                # Check for ERROR_INVALID_PARAMETER
-                if err.winerror == 87:
-                    raise UnsupportedOperation(err) from None
-                else:
-                    raise
+        _winapi.CopyFile2(source, target, 0)
 else:
     copyfile = None
 
diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py
@@ -709,27 +709,27 @@ def test_copy_link_preserve_metadata(self):
 
     @unittest.skipIf(sys.platform == "win32" or sys.platform == "wasi", "directories are always readable on Windows and WASI")
     @unittest.skipIf(root_in_posix, "test fails with root privilege")
-    def test_copytree_no_read_permission(self):
+    def test_copy_dir_no_read_permission(self):
         base = self.cls(self.base)
         source = base / 'dirE'
         target = base / 'copyE'
-        self.assertRaises(PermissionError, source.copytree, target)
+        self.assertRaises(PermissionError, source.copy, target)
         self.assertFalse(target.exists())
         errors = []
-        source.copytree(target, on_error=errors.append)
+        source.copy(target, on_error=errors.append)
         self.assertEqual(len(errors), 1)
         self.assertIsInstance(errors[0], PermissionError)
         self.assertFalse(target.exists())
 
-    def test_copytree_preserve_metadata(self):
+    def test_copy_dir_preserve_metadata(self):
         base = self.cls(self.base)
         source = base / 'dirC'
         if hasattr(os, 'chmod'):
             os.chmod(source / 'dirD', stat.S_IRWXU | stat.S_IRWXO)
         if hasattr(os, 'chflags') and hasattr(stat, 'UF_NODUMP'):
             os.chflags(source / 'fileC', stat.UF_NODUMP)
         target = base / 'copyA'
-        source.copytree(target, preserve_metadata=True)
+        source.copy(target, preserve_metadata=True)
 
         for subpath in ['.', 'fileC', 'dirD', 'dirD/fileD']:
             source_st = source.joinpath(subpath).stat()
@@ -741,13 +741,13 @@ def test_copytree_preserve_metadata(self):
                 self.assertEqual(source_st.st_flags, target_st.st_flags)
 
     @os_helper.skip_unless_xattr
-    def test_copytree_preserve_metadata_xattrs(self):
+    def test_copy_dir_preserve_metadata_xattrs(self):
         base = self.cls(self.base)
         source = base / 'dirC'
         source_file = source.joinpath('dirD', 'fileD')
         os.setxattr(source_file, b'user.foo', b'42')
         target = base / 'copyA'
-        source.copytree(target, preserve_metadata=True)
+        source.copy(target, preserve_metadata=True)
         target_file = target.joinpath('dirD', 'fileD')
         self.assertEqual(os.getxattr(target_file, b'user.foo'), b'42')
 
diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py
diff --git a/Misc/NEWS.d/next/Library/2024-05-15-01-36-08.gh-issue-73991.CGknDf.rst b/Misc/NEWS.d/next/Library/2024-05-15-01-36-08.gh-issue-73991.CGknDf.rst
diff --git a/Misc/NEWS.d/next/Library/2024-06-19-03-09-11.gh-issue-73991.lU_jK9.rst b/Misc/NEWS.d/next/Library/2024-06-19-03-09-11.gh-issue-73991.lU_jK9.rst
