From b7ce838ebc5cef95d762aa6321043660783c9e49 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 27 Jul 2024 23:20:04 +0100 Subject: [PATCH 01/17] GH-73991: Rework `pathlib.Path.copytree()` into `copy()`. 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.) --- Doc/library/pathlib.rst | 48 ++++--------- Doc/whatsnew/3.14.rst | 6 +- Lib/pathlib/_abc.py | 40 +++++------ Lib/pathlib/_local.py | 6 +- Lib/test/test_pathlib/test_pathlib.py | 14 ++-- Lib/test/test_pathlib/test_pathlib_abc.py | 71 ++++++++----------- ...4-05-15-01-36-08.gh-issue-73991.CGknDf.rst | 3 +- ...4-06-19-03-09-11.gh-issue-73991.lU_jK9.rst | 1 - 8 files changed, 76 insertions(+), 113 deletions(-) delete mode 100644 Misc/NEWS.d/next/Library/2024-06-19-03-09-11.gh-issue-73991.lU_jK9.rst diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 60c099af928123..ff34b0715b13e7 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1539,50 +1539,32 @@ 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, \ + preserve_metadata=False, dirs_exist_ok=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. + Recursively copy this file or directory tree to the given destination. - 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 a symlink is encountered 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. - - .. 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. + If the source and destination are existing directories 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. 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 index 7450597e8597ad..90c9c26f47a225 100644 --- a/Doc/whatsnew/3.14.rst +++ b/Doc/whatsnew/3.14.rst @@ -137,10 +137,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.rmtree` recursively removes a directory tree, like :func:`shutil.rmtree`. diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index c32e7762cefea3..adcd2c94a77baf 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -806,7 +806,7 @@ 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, *, follow_symlinks=True, preserve_metadata=False): """ 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 @@ -835,11 +835,10 @@ def copy(self, target, *, follow_symlinks=True, preserve_metadata=False): 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, preserve_metadata=False, + dirs_exist_ok=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) @@ -848,24 +847,21 @@ def on_error(err): raise err stack = [(self, target)] while stack: - source_dir, target_dir = stack.pop() + source, target = stack.pop() + if ignore and ignore(source): + continue 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 source.is_dir(follow_symlinks=follow_symlinks): + children = source.iterdir() + target.mkdir(exist_ok=dirs_exist_ok) + if preserve_metadata: + source._copy_metadata(target) + for child in children: + stack.append((child, target.joinpath(child.name))) + else: + source._copy_file(target, + follow_symlinks=follow_symlinks, + preserve_metadata=preserve_metadata) except OSError as err: on_error(err) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 4fd5279f9fe9ce..bf663ae785179f 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -787,7 +787,7 @@ 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, *, follow_symlinks=True, preserve_metadata=False): """ 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 @@ -804,8 +804,8 @@ def copy(self, target, *, follow_symlinks=True, preserve_metadata=False): return except UnsupportedOperation: pass # Fall through to generic code. - PathBase.copy(self, target, follow_symlinks=follow_symlinks, - preserve_metadata=preserve_metadata) + PathBase._copy_file(self, target, follow_symlinks=follow_symlinks, + preserve_metadata=preserve_metadata) def chmod(self, mode, *, follow_symlinks=True): """ diff --git a/Lib/test/test_pathlib/test_pathlib.py b/Lib/test/test_pathlib/test_pathlib.py index 7160e764dfb2fa..abf7016f71528d 100644 --- a/Lib/test/test_pathlib/test_pathlib.py +++ b/Lib/test/test_pathlib/test_pathlib.py @@ -709,19 +709,19 @@ 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'): @@ -729,7 +729,7 @@ def test_copytree_preserve_metadata(self): 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 index 37678c5d799e9a..a9d0090f683cc9 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1736,13 +1736,6 @@ def test_copy_file(self): self.assertTrue(target.exists()) self.assertEqual(source.read_text(), target.read_text()) - def test_copy_directory(self): - base = self.cls(self.base) - source = base / 'dirA' - target = base / 'copyA' - with self.assertRaises(OSError): - source.copy(target) - @needs_symlinks def test_copy_symlink_follow_symlinks_true(self): base = self.cls(self.base) @@ -1773,7 +1766,7 @@ def test_copy_directory_symlink_follow_symlinks_false(self): self.assertTrue(target.is_symlink()) self.assertEqual(source.readlink(), target.readlink()) - def test_copy_to_existing_file(self): + def test_copy_file_to_existing_file(self): base = self.cls(self.base) source = base / 'fileA' target = base / 'dirB' / 'fileB' @@ -1781,7 +1774,7 @@ def test_copy_to_existing_file(self): self.assertTrue(target.exists()) self.assertEqual(source.read_text(), target.read_text()) - def test_copy_to_existing_directory(self): + def test_copy_file_to_existing_directory(self): base = self.cls(self.base) source = base / 'fileA' target = base / 'dirA' @@ -1789,7 +1782,7 @@ def test_copy_to_existing_directory(self): source.copy(target) @needs_symlinks - def test_copy_to_existing_symlink(self): + def test_copy_file_to_existing_symlink(self): base = self.cls(self.base) source = base / 'dirB' / 'fileB' target = base / 'linkA' @@ -1802,7 +1795,7 @@ def test_copy_to_existing_symlink(self): self.assertEqual(source.read_text(), real_target.read_text()) @needs_symlinks - def test_copy_to_existing_symlink_follow_symlinks_false(self): + def test_copy_file_to_existing_symlink_follow_symlinks_false(self): base = self.cls(self.base) source = base / 'dirB' / 'fileB' target = base / 'linkA' @@ -1814,7 +1807,7 @@ def test_copy_to_existing_symlink_follow_symlinks_false(self): self.assertFalse(real_target.is_symlink()) self.assertEqual(source.read_text(), real_target.read_text()) - def test_copy_empty(self): + def test_copy_file_empty(self): base = self.cls(self.base) source = base / 'empty' target = base / 'copyA' @@ -1823,11 +1816,11 @@ def test_copy_empty(self): self.assertTrue(target.exists()) self.assertEqual(target.read_bytes(), b'') - def test_copytree_simple(self): + def test_copy_dir_simple(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' - source.copytree(target) + source.copy(target) self.assertTrue(target.is_dir()) self.assertTrue(target.joinpath('dirD').is_dir()) self.assertTrue(target.joinpath('dirD', 'fileD').is_file()) @@ -1837,7 +1830,7 @@ def test_copytree_simple(self): self.assertTrue(target.joinpath('fileC').read_text(), "this is file C\n") - def test_copytree_complex(self, follow_symlinks=True): + def test_copy_dir_complex(self, follow_symlinks=True): def ordered_walk(path): for dirpath, dirnames, filenames in path.walk(follow_symlinks=follow_symlinks): dirnames.sort() @@ -1853,7 +1846,7 @@ def ordered_walk(path): # Perform the copy target = base / 'copyC' - source.copytree(target, follow_symlinks=follow_symlinks) + source.copy(target, follow_symlinks=follow_symlinks) # Compare the source and target trees source_walk = ordered_walk(source) @@ -1879,24 +1872,24 @@ def ordered_walk(path): self.assertEqual(source_file.read_bytes(), target_file.read_bytes()) self.assertEqual(source_file.readlink(), target_file.readlink()) - def test_copytree_complex_follow_symlinks_false(self): - self.test_copytree_complex(follow_symlinks=False) + def test_copy_dir_complex_follow_symlinks_false(self): + self.test_copy_dir_complex(follow_symlinks=False) - def test_copytree_to_existing_directory(self): + def test_copy_dir_to_existing_directory(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' target.mkdir() target.joinpath('dirD').mkdir() - self.assertRaises(FileExistsError, source.copytree, target) + self.assertRaises(FileExistsError, source.copy, target) - def test_copytree_to_existing_directory_dirs_exist_ok(self): + def test_copy_dir_to_existing_directory_dirs_exist_ok(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' target.mkdir() target.joinpath('dirD').mkdir() - source.copytree(target, dirs_exist_ok=True) + source.copy(target, dirs_exist_ok=True) self.assertTrue(target.is_dir()) self.assertTrue(target.joinpath('dirD').is_dir()) self.assertTrue(target.joinpath('dirD', 'fileD').is_file()) @@ -1906,22 +1899,16 @@ def test_copytree_to_existing_directory_dirs_exist_ok(self): self.assertTrue(target.joinpath('fileC').read_text(), "this is file C\n") - def test_copytree_file(self): + def test_copy_missing_on_error(self): base = self.cls(self.base) - source = base / 'fileA' - target = base / 'copyA' - self.assertRaises(NotADirectoryError, source.copytree, target) - - def test_copytree_file_on_error(self): - base = self.cls(self.base) - source = base / 'fileA' + source = base / 'foo' target = base / 'copyA' errors = [] - source.copytree(target, on_error=errors.append) + source.copy(target, on_error=errors.append) self.assertEqual(len(errors), 1) - self.assertIsInstance(errors[0], NotADirectoryError) + self.assertIsInstance(errors[0], FileNotFoundError) - def test_copytree_ignore_false(self): + def test_copy_dir_ignore_false(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' @@ -1929,8 +1916,9 @@ def test_copytree_ignore_false(self): def ignore_false(path): ignores.append(path) return False - source.copytree(target, ignore=ignore_false) + source.copy(target, ignore=ignore_false) self.assertEqual(set(ignores), { + source, source / 'dirD', source / 'dirD' / 'fileD', source / 'fileC', @@ -1945,15 +1933,16 @@ def ignore_false(path): self.assertTrue(target.joinpath('fileC').read_text(), "this is file C\n") - def test_copytree_ignore_true(self): + def test_copy_dir_ignore_true(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' ignores = [] def ignore_true(path): - ignores.append(path) - return True - source.copytree(target, ignore=ignore_true) + if path != source: + ignores.append(path) + return True + source.copy(target, ignore=ignore_true) self.assertEqual(set(ignores), { source / 'dirD', source / 'fileC', @@ -1965,7 +1954,7 @@ def ignore_true(path): self.assertFalse(target.joinpath('novel.txt').exists()) @needs_symlinks - def test_copytree_dangling_symlink(self): + def test_copy_dangling_symlink(self): base = self.cls(self.base) source = base / 'source' target = base / 'target' @@ -1973,10 +1962,10 @@ def test_copytree_dangling_symlink(self): source.mkdir() source.joinpath('link').symlink_to('nonexistent') - self.assertRaises(FileNotFoundError, source.copytree, target) + self.assertRaises(FileNotFoundError, source.copy, target) target2 = base / 'target2' - source.copytree(target2, follow_symlinks=False) + source.copy(target2, follow_symlinks=False) self.assertTrue(target2.joinpath('link').is_symlink()) self.assertEqual(target2.joinpath('link').readlink(), self.cls('nonexistent')) 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 index c2953c65b2720f..d8e3bdf59ed092 100644 --- 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 @@ -1,2 +1 @@ -Add :meth:`pathlib.Path.copy`, which copies the content of one file to another, -like :func:`shutil.copyfile`. +Add :meth:`pathlib.Path.copy`, which copies a file or directory to another. 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 deleted file mode 100644 index 60a1b68d5bb1a8..00000000000000 --- a/Misc/NEWS.d/next/Library/2024-06-19-03-09-11.gh-issue-73991.lU_jK9.rst +++ /dev/null @@ -1 +0,0 @@ -Add :meth:`pathlib.Path.copytree`, which recursively copies a directory. From 76cf8936210969a3cd7e4f24aa16e7110c472535 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 00:50:40 +0100 Subject: [PATCH 02/17] Simplify handling of symlinks --- Lib/pathlib/__init__.py | 4 +- Lib/pathlib/_abc.py | 39 +++++++++---------- Lib/pathlib/_local.py | 21 ++++------ Lib/pathlib/_os.py | 47 ++--------------------- Lib/test/test_pathlib/test_pathlib_abc.py | 9 ++--- 5 files changed, 35 insertions(+), 85 deletions(-) diff --git a/Lib/pathlib/__init__.py b/Lib/pathlib/__init__.py index 2298a249529460..4b3edf535a61aa 100644 --- a/Lib/pathlib/__init__.py +++ b/Lib/pathlib/__init__.py @@ -5,8 +5,8 @@ operating systems. """ -from ._os import * +from ._abc import * from ._local import * -__all__ = (_os.__all__ + +__all__ = (_abc.__all__ + _local.__all__) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index adcd2c94a77baf..b28f79934fdb6e 100644 --- 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 ._os import copyfileobj + + +__all__ = ["UnsupportedOperation"] + + +class UnsupportedOperation(NotImplementedError): + """An exception that is raised when an unsupported operation is attempted. + """ + pass @functools.cache @@ -806,21 +815,14 @@ 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_file(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,8 +834,6 @@ def _copy_file(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 copy(self, target, *, follow_symlinks=True, preserve_metadata=False, dirs_exist_ok=False, ignore=None, on_error=None): @@ -848,20 +848,19 @@ def on_error(err): stack = [(self, target)] while stack: source, target = stack.pop() - if ignore and ignore(source): - continue try: if source.is_dir(follow_symlinks=follow_symlinks): children = source.iterdir() target.mkdir(exist_ok=dirs_exist_ok) - if preserve_metadata: - source._copy_metadata(target) for child in children: - stack.append((child, target.joinpath(child.name))) + if not (ignore and ignore(child)): + stack.append((child, target.joinpath(child.name))) + elif not follow_symlinks and source.is_symlink(): + target.symlink_to(source.readlink()) else: - source._copy_file(target, - follow_symlinks=follow_symlinks, - preserve_metadata=preserve_metadata) + source._copy_file(target) + if preserve_metadata: + source._copy_metadata(target, follow_symlinks=follow_symlinks) except OSError as err: on_error(err) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index bf663ae785179f..8c6d085918596f 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -17,9 +17,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 ._os import (copyfile, file_metadata_keys, read_file_metadata, + write_file_metadata) +from ._abc import UnsupportedOperation, PurePathBase, PathBase __all__ = [ @@ -787,7 +787,7 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): _write_metadata = write_file_metadata if copyfile: - def _copy_file(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 @@ -796,16 +796,11 @@ def _copy_file(self, target, *, follow_symlinks=True, preserve_metadata=False): try: target = os.fspath(target) except TypeError: - if not isinstance(target, PathBase): - raise + if isinstance(target, PathBase): + return PathBase._copy_file(self, target) + raise else: - try: - copyfile(os.fspath(self), target, follow_symlinks) - return - except UnsupportedOperation: - pass # Fall through to generic code. - PathBase._copy_file(self, target, follow_symlinks=follow_symlinks, - preserve_metadata=preserve_metadata) + copyfile(os.fspath(self), target) def chmod(self, mode, *, follow_symlinks=True): """ diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index 164ee8e9034427..f680c21eb86654 100644 --- 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: + 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_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index a9d0090f683cc9..0026377b1cc7ed 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -5,8 +5,7 @@ import stat import unittest -from pathlib._os import UnsupportedOperation -from pathlib._abc import ParserBase, PurePathBase, PathBase +from pathlib._abc import UnsupportedOperation, ParserBase, PurePathBase, PathBase import posixpath from test.support import is_wasi @@ -1918,7 +1917,6 @@ def ignore_false(path): return False source.copy(target, ignore=ignore_false) self.assertEqual(set(ignores), { - source, source / 'dirD', source / 'dirD' / 'fileD', source / 'fileC', @@ -1939,9 +1937,8 @@ def test_copy_dir_ignore_true(self): target = base / 'copyC' ignores = [] def ignore_true(path): - if path != source: - ignores.append(path) - return True + ignores.append(path) + return True source.copy(target, ignore=ignore_true) self.assertEqual(set(ignores), { source / 'dirD', From b10a3b5dd838040fa1455926c89a3d005f868824 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 01:06:16 +0100 Subject: [PATCH 03/17] Inline _copy_metadata() --- Lib/pathlib/_abc.py | 22 +++++++--------------- Lib/pathlib/_local.py | 8 +++----- 2 files changed, 10 insertions(+), 20 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index b28f79934fdb6e..5b64ea2d376edf 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -805,22 +805,10 @@ def _write_metadata(self, metadata, *, follow_symlinks=True): """ raise UnsupportedOperation(self._unsupported_msg('_write_metadata()')) - def _copy_metadata(self, target, *, follow_symlinks=True): - """ - Copies metadata (permissions, timestamps, etc) from this path to target. - """ - # Metadata types supported by both source and target. - keys = self._readable_metadata & target._writable_metadata - if keys: - metadata = self._read_metadata(keys, follow_symlinks=follow_symlinks) - target._write_metadata(metadata, follow_symlinks=follow_symlinks) - - def _copy_file(self, target): + def _copy_data(self, 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") with self.open('rb') as source_f: @@ -858,9 +846,13 @@ def on_error(err): elif not follow_symlinks and source.is_symlink(): target.symlink_to(source.readlink()) else: - source._copy_file(target) + source._copy_data(target) if preserve_metadata: - source._copy_metadata(target, follow_symlinks=follow_symlinks) + # Metadata types supported by both source and target. + keys = source._readable_metadata & target._writable_metadata + if keys: + metadata = source._read_metadata(keys, follow_symlinks=follow_symlinks) + target._write_metadata(metadata, follow_symlinks=follow_symlinks) except OSError as err: on_error(err) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 8c6d085918596f..a7e80824e7abf1 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -787,17 +787,15 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): _write_metadata = write_file_metadata if copyfile: - def _copy_file(self, target): + def _copy_data(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 isinstance(target, PathBase): - return PathBase._copy_file(self, target) + return PathBase._copy_data(self, target) raise else: copyfile(os.fspath(self), target) From a67f5a7d93151d66ca37bdc2a9caf8861bad91b7 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 01:40:24 +0100 Subject: [PATCH 04/17] Further symlink tweaks --- Lib/pathlib/_abc.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 5b64ea2d376edf..86423da9c0945e 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -837,14 +837,14 @@ def on_error(err): while stack: source, target = stack.pop() try: - if source.is_dir(follow_symlinks=follow_symlinks): + if not follow_symlinks and source.is_symlink(): + target.symlink_to(source.readlink()) + elif source.is_dir(): children = source.iterdir() target.mkdir(exist_ok=dirs_exist_ok) for child in children: if not (ignore and ignore(child)): stack.append((child, target.joinpath(child.name))) - elif not follow_symlinks and source.is_symlink(): - target.symlink_to(source.readlink()) else: source._copy_data(target) if preserve_metadata: From 017dd864b2d0ff2eaefa37ff20d8e14131377622 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 03:09:13 +0100 Subject: [PATCH 05/17] Fix copying metadata to non-symlinks --- Lib/pathlib/_abc.py | 24 +++++++++++++++++------- Lib/pathlib/_local.py | 6 +++--- 2 files changed, 20 insertions(+), 10 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 86423da9c0945e..745c2328c96411 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -805,6 +805,16 @@ def _write_metadata(self, metadata, *, follow_symlinks=True): """ raise UnsupportedOperation(self._unsupported_msg('_write_metadata()')) + def _copy_metadata(self, target, *, follow_symlinks=True): + """ + Copies metadata (permissions, timestamps, etc) from this path to target. + """ + # Metadata types supported by both source and target. + keys = self._readable_metadata & target._writable_metadata + if keys: + metadata = self._read_metadata(keys, follow_symlinks=follow_symlinks) + target._write_metadata(metadata, follow_symlinks=follow_symlinks) + def _copy_data(self, target): """ Copy the contents of this file to the given target. @@ -839,20 +849,20 @@ def on_error(err): try: if not follow_symlinks and source.is_symlink(): target.symlink_to(source.readlink()) - elif source.is_dir(): + if preserve_metadata: + source._copy_metadata(target, follow_symlinks=False) + elif source.is_dir(follow_symlinks=follow_symlinks): children = source.iterdir() target.mkdir(exist_ok=dirs_exist_ok) for child in children: if not (ignore and ignore(child)): stack.append((child, target.joinpath(child.name))) + if preserve_metadata: + source._copy_metadata(target) else: source._copy_data(target) - if preserve_metadata: - # Metadata types supported by both source and target. - keys = source._readable_metadata & target._writable_metadata - if keys: - metadata = source._read_metadata(keys, follow_symlinks=follow_symlinks) - target._write_metadata(metadata, follow_symlinks=follow_symlinks) + if preserve_metadata: + source._copy_metadata(target) except OSError as err: on_error(err) diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index a7e80824e7abf1..1e3b0fd5204d00 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -794,9 +794,9 @@ def _copy_data(self, target): try: target = os.fspath(target) except TypeError: - if isinstance(target, PathBase): - return PathBase._copy_data(self, target) - raise + if not isinstance(target, PathBase): + raise + PathBase._copy_data(self, target) else: copyfile(os.fspath(self), target) From fcc8af1581d894416a2ed7468609c163db09dfb4 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 03:42:52 +0100 Subject: [PATCH 06/17] Specify target_is_directory when symlinking. --- Lib/pathlib/_abc.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 745c2328c96411..771c70532263e7 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -848,7 +848,9 @@ def on_error(err): source, target = stack.pop() try: if not follow_symlinks and source.is_symlink(): - target.symlink_to(source.readlink()) + target.symlink_to( + source.readlink(), + target_is_directory=source.is_dir()) if preserve_metadata: source._copy_metadata(target, follow_symlinks=False) elif source.is_dir(follow_symlinks=follow_symlinks): From 57efe1f6f26fea3ddb77c7865da8392015e724f6 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 28 Jul 2024 20:21:39 +0100 Subject: [PATCH 07/17] Tiny simplification. --- Lib/pathlib/_abc.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 771c70532263e7..9ca898e2e875b2 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -853,7 +853,7 @@ def on_error(err): target_is_directory=source.is_dir()) if preserve_metadata: source._copy_metadata(target, follow_symlinks=False) - elif source.is_dir(follow_symlinks=follow_symlinks): + elif source.is_dir(): children = source.iterdir() target.mkdir(exist_ok=dirs_exist_ok) for child in children: From 8bef16b484e412d141d86f1313651bbec68575e1 Mon Sep 17 00:00:00 2001 From: barneygale Date: Mon, 29 Jul 2024 03:27:16 +0100 Subject: [PATCH 08/17] Skip unnecessary is_dir() when copying symlinks on POSIX. --- Lib/pathlib/_abc.py | 11 ++++++++--- Lib/pathlib/_local.py | 8 ++++++++ 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 9ca898e2e875b2..5cc0f19b6ac5ff 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -770,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*. @@ -848,9 +855,7 @@ def on_error(err): source, target = stack.pop() try: if not follow_symlinks and source.is_symlink(): - target.symlink_to( - source.readlink(), - target_is_directory=source.is_dir()) + target._symlink_to_target_of(source) if preserve_metadata: source._copy_metadata(target, follow_symlinks=False) elif source.is_dir(): diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 1e3b0fd5204d00..e658930cf663e5 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -876,6 +876,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): """ From a51ce636eb91c4dc0d30d588e7aa08596fa587ba Mon Sep 17 00:00:00 2001 From: barneygale Date: Tue, 30 Jul 2024 18:21:09 +0100 Subject: [PATCH 09/17] Docs tweaks --- Doc/library/pathlib.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index ff34b0715b13e7..8e7b77df70ee6d 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1543,7 +1543,8 @@ Copying, renaming and deleting preserve_metadata=False, dirs_exist_ok=False, \ ignore=None, on_error=None) - Recursively copy this file or directory tree to the given destination. + Copy this file or directory tree to the given *target*. If both paths are + existing files, the target file will be replaced. If a symlink is encountered and *follow_symlinks* is true (the default), the symlink's target is copied. Otherwise, the symlink is recreated at the @@ -1556,8 +1557,8 @@ Copying, renaming and deleting This argument has no effect on Windows, where metadata is always preserved when copying. - If the source and destination are existing directories and *dirs_exist_ok* - is false (the default), a :exc:`FileExistsError` is raised. Otherwise, the + If the source and target are existing directories 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. From 51b1e6d6a7a7e3e990887e55b3150efb99db23f8 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 4 Aug 2024 14:48:01 +0100 Subject: [PATCH 10/17] Fix CopyFile2 check --- Lib/pathlib/_os.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/pathlib/_os.py b/Lib/pathlib/_os.py index f680c21eb86654..63dbe131baea51 100644 --- a/Lib/pathlib/_os.py +++ b/Lib/pathlib/_os.py @@ -92,7 +92,7 @@ def copyfd(source_fd, target_fd): copyfd = None -if _winapi: +if _winapi and hasattr(_winapi, 'CopyFile2'): def copyfile(source, target): """ Copy from one file to another using CopyFile2 (Windows only). From 023b3a41ecd610867507360eac9078d89cc1ff7b Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 4 Aug 2024 15:29:12 +0100 Subject: [PATCH 11/17] Docs tweak --- Doc/library/pathlib.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 8e7b77df70ee6d..7cf7a9ad6cc495 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1554,8 +1554,8 @@ Copying, renaming and deleting 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. + This argument has no effect when copying files on Windows (metadata is + always preserved in this case.) If the source and target are existing directories and *dirs_exist_ok* is false (the default), a :exc:`FileExistsError` is raised. Otherwise, the From 4d6d2dcc777002dae1f81698a969619a5891edce Mon Sep 17 00:00:00 2001 From: barneygale Date: Sun, 4 Aug 2024 16:53:32 +0100 Subject: [PATCH 12/17] Address review feedback --- Doc/library/pathlib.rst | 27 +++++++++++++-------------- Lib/pathlib/_abc.py | 4 ++-- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 7cf7a9ad6cc495..6713589700ab2a 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1539,16 +1539,21 @@ Creating files and directories Copying, renaming and deleting ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -.. method:: Path.copy(target, *, follow_symlinks=True, \ - preserve_metadata=False, dirs_exist_ok=False, \ - ignore=None, on_error=None) +.. method:: Path.copy(target, *, follow_symlinks=True, dirs_exist_ok=False, \ + preserve_metadata=False, ignore=None, on_error=None) - Copy this file or directory tree to the given *target*. If both paths are - existing files, the target file will be replaced. + Copy this file or directory tree to the given *target*. - If a symlink is encountered and *follow_symlinks* is true (the default), - the symlink's target is copied. Otherwise, the symlink is recreated at the - destination. + 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 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 continue if it + encounters existing directories, and files within the destination tree will + be overwritten by corresponding files from the source tree. If *preserve_metadata* is false (the default), only directory structures and file data are guaranteed to be copied. Set *preserve_metadata* to true @@ -1557,12 +1562,6 @@ Copying, renaming and deleting This argument has no effect when copying files on Windows (metadata is always preserved in this case.) - If the source and target are existing directories 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. - If *ignore* is given, it should be a callable accepting one argument: a source file or directory path. The callable may return true to suppress copying of the path. diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 5cc0f19b6ac5ff..b453b84f028fca 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -840,8 +840,8 @@ def _copy_data(self, target): else: raise - def copy(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 file or directory tree to the given destination. """ From 5a945256f9f02d8d39c5cfd2a23c4043271e07fb Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 7 Aug 2024 03:18:12 +0100 Subject: [PATCH 13/17] Return the target as a Path object. --- Doc/library/pathlib.rst | 3 +- Lib/pathlib/_abc.py | 23 ++++++------ Lib/test/test_pathlib/test_pathlib_abc.py | 45 +++++++++++++++-------- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index 6bf4b1fae9016d..bf3fefb628d852 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1542,7 +1542,8 @@ Copying, renaming and deleting .. method:: Path.copy(target, *, follow_symlinks=True, dirs_exist_ok=False, \ preserve_metadata=False, ignore=None, on_error=None) - Copy this file or directory tree to the given *target*. + Copy this file or directory tree to the given *target*, and return a new + :class:`!Path` instance pointing to *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 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index 8ea6aab84a8baf..ca91e1c30fdb9d 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -852,26 +852,27 @@ def on_error(err): raise err stack = [(self, target)] while stack: - source, target = stack.pop() + src, dst = stack.pop() try: - if not follow_symlinks and source.is_symlink(): - target._symlink_to_target_of(source) + if not follow_symlinks and src.is_symlink(): + dst._symlink_to_target_of(src) if preserve_metadata: - source._copy_metadata(target, follow_symlinks=False) - elif source.is_dir(): - children = source.iterdir() - target.mkdir(exist_ok=dirs_exist_ok) + 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, target.joinpath(child.name))) + stack.append((child, dst.joinpath(child.name))) if preserve_metadata: - source._copy_metadata(target) + src._copy_metadata(dst) else: - source._copy_data(target) + src._copy_data(dst) if preserve_metadata: - source._copy_metadata(target) + src._copy_metadata(dst) except OSError as err: on_error(err) + return target def rename(self, target): """ diff --git a/Lib/test/test_pathlib/test_pathlib_abc.py b/Lib/test/test_pathlib/test_pathlib_abc.py index 4cad9864c22b79..629a1d4bdeb4de 100644 --- a/Lib/test/test_pathlib/test_pathlib_abc.py +++ b/Lib/test/test_pathlib/test_pathlib_abc.py @@ -1731,7 +1731,8 @@ def test_copy_file(self): base = self.cls(self.base) source = base / 'fileA' target = base / 'copyA' - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertEqual(source.read_text(), target.read_text()) @@ -1740,7 +1741,8 @@ def test_copy_symlink_follow_symlinks_true(self): base = self.cls(self.base) source = base / 'linkA' target = base / 'copyA' - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertFalse(target.is_symlink()) self.assertEqual(source.read_text(), target.read_text()) @@ -1750,7 +1752,8 @@ def test_copy_symlink_follow_symlinks_false(self): base = self.cls(self.base) source = base / 'linkA' target = base / 'copyA' - source.copy(target, follow_symlinks=False) + result = source.copy(target, follow_symlinks=False) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertTrue(target.is_symlink()) self.assertEqual(source.readlink(), target.readlink()) @@ -1760,7 +1763,8 @@ def test_copy_directory_symlink_follow_symlinks_false(self): base = self.cls(self.base) source = base / 'linkB' target = base / 'copyA' - source.copy(target, follow_symlinks=False) + result = source.copy(target, follow_symlinks=False) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertTrue(target.is_symlink()) self.assertEqual(source.readlink(), target.readlink()) @@ -1769,7 +1773,8 @@ def test_copy_file_to_existing_file(self): base = self.cls(self.base) source = base / 'fileA' target = base / 'dirB' / 'fileB' - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertEqual(source.read_text(), target.read_text()) @@ -1786,7 +1791,8 @@ def test_copy_file_to_existing_symlink(self): source = base / 'dirB' / 'fileB' target = base / 'linkA' real_target = base / 'fileA' - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertTrue(target.is_symlink()) self.assertTrue(real_target.exists()) @@ -1799,7 +1805,8 @@ def test_copy_file_to_existing_symlink_follow_symlinks_false(self): source = base / 'dirB' / 'fileB' target = base / 'linkA' real_target = base / 'fileA' - source.copy(target, follow_symlinks=False) + result = source.copy(target, follow_symlinks=False) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertTrue(target.is_symlink()) self.assertTrue(real_target.exists()) @@ -1811,7 +1818,8 @@ def test_copy_file_empty(self): source = base / 'empty' target = base / 'copyA' source.write_bytes(b'') - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.exists()) self.assertEqual(target.read_bytes(), b'') @@ -1819,7 +1827,8 @@ def test_copy_dir_simple(self): base = self.cls(self.base) source = base / 'dirC' target = base / 'copyC' - source.copy(target) + result = source.copy(target) + self.assertEqual(result, target) self.assertTrue(target.is_dir()) self.assertTrue(target.joinpath('dirD').is_dir()) self.assertTrue(target.joinpath('dirD', 'fileD').is_file()) @@ -1845,7 +1854,8 @@ def ordered_walk(path): # Perform the copy target = base / 'copyC' - source.copy(target, follow_symlinks=follow_symlinks) + result = source.copy(target, follow_symlinks=follow_symlinks) + self.assertEqual(result, target) # Compare the source and target trees source_walk = ordered_walk(source) @@ -1888,7 +1898,8 @@ def test_copy_dir_to_existing_directory_dirs_exist_ok(self): target = base / 'copyC' target.mkdir() target.joinpath('dirD').mkdir() - source.copy(target, dirs_exist_ok=True) + result = source.copy(target, dirs_exist_ok=True) + self.assertEqual(result, target) self.assertTrue(target.is_dir()) self.assertTrue(target.joinpath('dirD').is_dir()) self.assertTrue(target.joinpath('dirD', 'fileD').is_file()) @@ -1903,7 +1914,8 @@ def test_copy_missing_on_error(self): source = base / 'foo' target = base / 'copyA' errors = [] - source.copy(target, on_error=errors.append) + result = source.copy(target, on_error=errors.append) + self.assertEqual(result, target) self.assertEqual(len(errors), 1) self.assertIsInstance(errors[0], FileNotFoundError) @@ -1915,7 +1927,8 @@ def test_copy_dir_ignore_false(self): def ignore_false(path): ignores.append(path) return False - source.copy(target, ignore=ignore_false) + result = source.copy(target, ignore=ignore_false) + self.assertEqual(result, target) self.assertEqual(set(ignores), { source / 'dirD', source / 'dirD' / 'fileD', @@ -1939,7 +1952,8 @@ def test_copy_dir_ignore_true(self): def ignore_true(path): ignores.append(path) return True - source.copy(target, ignore=ignore_true) + result = source.copy(target, ignore=ignore_true) + self.assertEqual(result, target) self.assertEqual(set(ignores), { source / 'dirD', source / 'fileC', @@ -1962,7 +1976,8 @@ def test_copy_dangling_symlink(self): self.assertRaises(FileNotFoundError, source.copy, target) target2 = base / 'target2' - source.copy(target2, follow_symlinks=False) + result = source.copy(target2, follow_symlinks=False) + self.assertEqual(result, target2) self.assertTrue(target2.joinpath('link').is_symlink()) self.assertEqual(target2.joinpath('link').readlink(), self.cls('nonexistent')) From 3b6405bbc1444b483e2c5cb44d42a9e19fca3607 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Wed, 7 Aug 2024 16:11:31 +0100 Subject: [PATCH 14/17] Apply suggestions from code review Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com> --- Doc/library/pathlib.rst | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index bf3fefb628d852..e1cec7bb358b96 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1560,8 +1560,8 @@ Copying, renaming and deleting 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 when copying files on Windows (metadata is - always preserved in this case.) + 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 source file or directory path. The callable may return true to suppress From 03f9b7f398057cb9270e6484fed9b52dc2004701 Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 7 Aug 2024 16:30:12 +0100 Subject: [PATCH 15/17] Address some review feedback --- Doc/library/pathlib.rst | 8 ++++---- Lib/pathlib/_abc.py | 4 ++-- Lib/pathlib/_local.py | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Doc/library/pathlib.rst b/Doc/library/pathlib.rst index e1cec7bb358b96..4df4164e84ef34 100644 --- a/Doc/library/pathlib.rst +++ b/Doc/library/pathlib.rst @@ -1552,15 +1552,15 @@ Copying, renaming and deleting 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 continue if it - encounters existing directories, and files within the destination tree will - be overwritten by corresponding files from the source tree. + If *dirs_exists_ok* is true, the copying operation will overwrite + existing files within the destination tree with corresponding files + from the source tree. 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 when copying files on Windows (where + 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 diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index ca91e1c30fdb9d..ab69369b6b3afc 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -822,7 +822,7 @@ 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_data(self, target): + def _copy_file(self, target): """ Copy the contents of this file to the given target. """ @@ -867,7 +867,7 @@ def on_error(err): if preserve_metadata: src._copy_metadata(dst) else: - src._copy_data(dst) + src._copy_file(dst) if preserve_metadata: src._copy_metadata(dst) except OSError as err: diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index e459527c92cc7c..19a0c22b4a3320 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -788,7 +788,7 @@ def mkdir(self, mode=0o777, parents=False, exist_ok=False): _write_metadata = write_file_metadata if copyfile: - def _copy_data(self, target): + def _copy_file(self, target): """ Copy the contents of this file to the given target. """ @@ -797,7 +797,7 @@ def _copy_data(self, target): except TypeError: if not isinstance(target, PathBase): raise - PathBase._copy_data(self, target) + PathBase._copy_file(self, target) else: copyfile(os.fspath(self), target) From 02e92378f2436c043c0aae8d511311d33740d6ad Mon Sep 17 00:00:00 2001 From: barneygale Date: Wed, 7 Aug 2024 16:49:37 +0100 Subject: [PATCH 16/17] Simplify tracebacks --- Lib/pathlib/_abc.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index ab69369b6b3afc..fa6c2bd71ebd58 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -847,9 +847,6 @@ def copy(self, target, *, follow_symlinks=True, dirs_exist_ok=False, """ 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: src, dst = stack.pop() @@ -871,6 +868,8 @@ def on_error(err): if preserve_metadata: src._copy_metadata(dst) except OSError as err: + if on_error is None: + raise on_error(err) return target From fed93fca949572b5171ddcb883c162fadca92102 Mon Sep 17 00:00:00 2001 From: barneygale Date: Sat, 10 Aug 2024 20:43:33 +0100 Subject: [PATCH 17/17] Use absolute imports --- Lib/pathlib/__init__.py | 4 ++-- Lib/pathlib/_abc.py | 2 +- Lib/pathlib/_local.py | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Lib/pathlib/__init__.py b/Lib/pathlib/__init__.py index 4b3edf535a61aa..5da3acd31997e5 100644 --- a/Lib/pathlib/__init__.py +++ b/Lib/pathlib/__init__.py @@ -5,8 +5,8 @@ operating systems. """ -from ._abc import * -from ._local import * +from pathlib._abc import * +from pathlib._local import * __all__ = (_abc.__all__ + _local.__all__) diff --git a/Lib/pathlib/_abc.py b/Lib/pathlib/_abc.py index fa6c2bd71ebd58..500846d19cf811 100644 --- a/Lib/pathlib/_abc.py +++ b/Lib/pathlib/_abc.py @@ -16,7 +16,7 @@ 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 copyfileobj +from pathlib._os import copyfileobj __all__ = ["UnsupportedOperation"] diff --git a/Lib/pathlib/_local.py b/Lib/pathlib/_local.py index 19a0c22b4a3320..8f5c58c16ef0d0 100644 --- a/Lib/pathlib/_local.py +++ b/Lib/pathlib/_local.py @@ -18,9 +18,9 @@ except ImportError: grp = None -from ._os import (copyfile, file_metadata_keys, read_file_metadata, - write_file_metadata) -from ._abc import UnsupportedOperation, PurePathBase, PathBase +from pathlib._os import (copyfile, file_metadata_keys, read_file_metadata, + write_file_metadata) +from pathlib._abc import UnsupportedOperation, PurePathBase, PathBase __all__ = [