From bdfcc65abcb582c79681fb9c9c5c282f4f20fa64 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Wed, 2 Nov 2022 23:37:31 +0000 Subject: [PATCH 1/5] gh-99029: Fix handling of `PureWindowsPath('C:\').relative_to('C:')` `relative_to()` now treats naked drive paths as relative. This brings its behaviour in line with other parts of pathlib, and with `ntpath.relpath()`, and so allows us to factor out the pathlib-specific implementation. --- Lib/pathlib.py | 57 ++++--------------- Lib/test/test_pathlib.py | 12 +--- ...2-11-02-23-47-07.gh-issue-99029.7uCiIB.rst | 3 + 3 files changed, 18 insertions(+), 54 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 1498ce08be4012..6b545bbea651bf 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -634,57 +634,24 @@ def relative_to(self, *other, walk_up=False): The *walk_up* parameter controls whether `..` may be used to resolve the path. """ - # For the purpose of this method, drive and root are considered - # separate parts, i.e.: - # Path('c:/').relative_to('c:') gives Path('/') - # Path('c:/').relative_to('/') raise ValueError if not other: raise TypeError("need at least one argument") - parts = self._parts - drv = self._drv - root = self._root - if root: - abs_parts = [drv, root] + parts[1:] - else: - abs_parts = parts - other_drv, other_root, other_parts = self._parse_args(other) - if other_root: - other_abs_parts = [other_drv, other_root] + other_parts[1:] - else: - other_abs_parts = other_parts - num_parts = len(other_abs_parts) - casefold = self._flavour.casefold_parts - num_common_parts = 0 - for part, other_part in zip(casefold(abs_parts), casefold(other_abs_parts)): - if part != other_part: - break - num_common_parts += 1 - if walk_up: - failure = root != other_root - if drv or other_drv: - failure = casefold([drv]) != casefold([other_drv]) or (failure and num_parts > 1) - error_message = "{!r} is not on the same drive as {!r}" - up_parts = (num_parts-num_common_parts)*['..'] - else: - failure = (root or drv) if num_parts == 0 else num_common_parts != num_parts - error_message = "{!r} is not in the subpath of {!r}" - up_parts = [] - error_message += " OR one path is relative and the other is absolute." - if failure: - formatted = self._format_parsed_parts(other_drv, other_root, other_parts) - raise ValueError(error_message.format(str(self), str(formatted))) - path_parts = up_parts + abs_parts[num_common_parts:] - new_root = root if num_common_parts == 1 else '' - return self._from_parsed_parts('', new_root, path_parts) + path_cls = type(self) + other = path_cls(*other) + if path_cls(self.anchor) != path_cls(other.anchor): + raise ValueError("{!r} and {!r} have different anchors".format(str(self), str(other))) + result = path_cls(self._flavour.pathmod.relpath(self, other)) + if result.parts[:1] == ('..',) and not walk_up: + raise ValueError("{!r} is not in the subpath of {!r}".format(str(self), str(other))) + return result def is_relative_to(self, *other): """Return True if the path is relative to another path or False. """ - try: - self.relative_to(*other) - return True - except ValueError: - return False + if not other: + raise TypeError("need at least one argument") + other = type(self)(*other) + return other == self or other in self.parents @property def parts(self): diff --git a/Lib/test/test_pathlib.py b/Lib/test/test_pathlib.py index 3b1f302cc964ba..035725f3b2099b 100644 --- a/Lib/test/test_pathlib.py +++ b/Lib/test/test_pathlib.py @@ -1183,10 +1183,6 @@ def test_relative_to(self): self.assertRaises(ValueError, p.relative_to, P('/Foo'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('C:/Foo'), walk_up=True) p = P('C:/Foo/Bar') - self.assertEqual(p.relative_to(P('c:')), P('/Foo/Bar')) - self.assertEqual(p.relative_to('c:'), P('/Foo/Bar')) - self.assertEqual(str(p.relative_to(P('c:'))), '\\Foo\\Bar') - self.assertEqual(str(p.relative_to('c:')), '\\Foo\\Bar') self.assertEqual(p.relative_to(P('c:/')), P('Foo/Bar')) self.assertEqual(p.relative_to('c:/'), P('Foo/Bar')) self.assertEqual(p.relative_to(P('c:/foO')), P('Bar')) @@ -1194,10 +1190,6 @@ def test_relative_to(self): self.assertEqual(p.relative_to('c:/foO/'), P('Bar')) self.assertEqual(p.relative_to(P('c:/foO/baR')), P()) self.assertEqual(p.relative_to('c:/foO/baR'), P()) - self.assertEqual(p.relative_to(P('c:'), walk_up=True), P('/Foo/Bar')) - self.assertEqual(p.relative_to('c:', walk_up=True), P('/Foo/Bar')) - self.assertEqual(str(p.relative_to(P('c:'), walk_up=True)), '\\Foo\\Bar') - self.assertEqual(str(p.relative_to('c:', walk_up=True)), '\\Foo\\Bar') self.assertEqual(p.relative_to(P('c:/'), walk_up=True), P('Foo/Bar')) self.assertEqual(p.relative_to('c:/', walk_up=True), P('Foo/Bar')) self.assertEqual(p.relative_to(P('c:/foO'), walk_up=True), P('Bar')) @@ -1209,6 +1201,7 @@ def test_relative_to(self): self.assertEqual(p.relative_to('C:/Foo/Bar/Baz', walk_up=True), P('..')) self.assertEqual(p.relative_to('C:/Foo/Baz', walk_up=True), P('../Bar')) # Unrelated paths. + self.assertRaises(ValueError, p.relative_to, P('c:')) self.assertRaises(ValueError, p.relative_to, P('C:/Baz')) self.assertRaises(ValueError, p.relative_to, P('C:/Foo/Bar/Baz')) self.assertRaises(ValueError, p.relative_to, P('C:/Foo/Baz')) @@ -1218,6 +1211,7 @@ def test_relative_to(self): self.assertRaises(ValueError, p.relative_to, P('/')) self.assertRaises(ValueError, p.relative_to, P('/Foo')) self.assertRaises(ValueError, p.relative_to, P('//C/Foo')) + self.assertRaises(ValueError, p.relative_to, P('c:'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('C:Foo'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('d:'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('d:/'), walk_up=True) @@ -1275,13 +1269,13 @@ def test_is_relative_to(self): self.assertFalse(p.is_relative_to(P('C:Foo/Bar/Baz'))) self.assertFalse(p.is_relative_to(P('C:Foo/Baz'))) p = P('C:/Foo/Bar') - self.assertTrue(p.is_relative_to('c:')) self.assertTrue(p.is_relative_to(P('c:/'))) self.assertTrue(p.is_relative_to(P('c:/foO'))) self.assertTrue(p.is_relative_to('c:/foO/')) self.assertTrue(p.is_relative_to(P('c:/foO/baR'))) self.assertTrue(p.is_relative_to('c:/foO/baR')) # Unrelated paths. + self.assertFalse(p.is_relative_to('c:')) self.assertFalse(p.is_relative_to(P('C:/Baz'))) self.assertFalse(p.is_relative_to(P('C:/Foo/Bar/Baz'))) self.assertFalse(p.is_relative_to(P('C:/Foo/Baz'))) diff --git a/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst b/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst new file mode 100644 index 00000000000000..6043720233b3df --- /dev/null +++ b/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst @@ -0,0 +1,3 @@ +:meth:`pathlib.PurePath.relative_to()` now treats naked Windows drive paths +as relative. This brings its behaviour in line with other parts of pathlib, +and with :func:`os.path.relpath()`. From 4b190a1a11c09d108c8cbfc24054040daf33e1b7 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Thu, 3 Nov 2022 18:21:35 +0000 Subject: [PATCH 2/5] Restore deleted assertions --- Lib/test/test_pathlib.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_pathlib.py b/Lib/test/test_pathlib.py index 035725f3b2099b..4a6449feef8d24 100644 --- a/Lib/test/test_pathlib.py +++ b/Lib/test/test_pathlib.py @@ -1201,6 +1201,7 @@ def test_relative_to(self): self.assertEqual(p.relative_to('C:/Foo/Bar/Baz', walk_up=True), P('..')) self.assertEqual(p.relative_to('C:/Foo/Baz', walk_up=True), P('../Bar')) # Unrelated paths. + self.assertRaises(ValueError, p.relative_to, 'c:') self.assertRaises(ValueError, p.relative_to, P('c:')) self.assertRaises(ValueError, p.relative_to, P('C:/Baz')) self.assertRaises(ValueError, p.relative_to, P('C:/Foo/Bar/Baz')) @@ -1211,6 +1212,7 @@ def test_relative_to(self): self.assertRaises(ValueError, p.relative_to, P('/')) self.assertRaises(ValueError, p.relative_to, P('/Foo')) self.assertRaises(ValueError, p.relative_to, P('//C/Foo')) + self.assertRaises(ValueError, p.relative_to, 'c:', walk_up=True) self.assertRaises(ValueError, p.relative_to, P('c:'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('C:Foo'), walk_up=True) self.assertRaises(ValueError, p.relative_to, P('d:'), walk_up=True) From 4d13ac8c3965458a19ec6d6d24aad543a747461e Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Sat, 12 Nov 2022 06:01:06 +0000 Subject: [PATCH 3/5] Stop using `os.path.relpath()` Differences in handling of '..' parts still make these functions fundamentally incompatible. --- Lib/pathlib.py | 10 +++++++--- .../2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst | 3 +-- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 6b545bbea651bf..0154167679836a 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -640,10 +640,14 @@ def relative_to(self, *other, walk_up=False): other = path_cls(*other) if path_cls(self.anchor) != path_cls(other.anchor): raise ValueError("{!r} and {!r} have different anchors".format(str(self), str(other))) - result = path_cls(self._flavour.pathmod.relpath(self, other)) - if result.parts[:1] == ('..',) and not walk_up: + if not self.is_relative_to(other) and not walk_up: raise ValueError("{!r} is not in the subpath of {!r}".format(str(self), str(other))) - return result + walk_up_steps = 0 + while not self.is_relative_to(other): + other = other.parent + walk_up_steps += 1 + parts = ('..',) * walk_up_steps + self.parts[len(other.parts):] + return path_cls(*parts) def is_relative_to(self, *other): """Return True if the path is relative to another path or False. diff --git a/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst b/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst index 6043720233b3df..0bfba5e1e32662 100644 --- a/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst +++ b/Misc/NEWS.d/next/Library/2022-11-02-23-47-07.gh-issue-99029.7uCiIB.rst @@ -1,3 +1,2 @@ :meth:`pathlib.PurePath.relative_to()` now treats naked Windows drive paths -as relative. This brings its behaviour in line with other parts of pathlib, -and with :func:`os.path.relpath()`. +as relative. This brings its behaviour in line with other parts of pathlib. From 3b37a0d77f61adb20cc9c6e96b1895a4bf7764a4 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Sat, 12 Nov 2022 22:46:12 +0000 Subject: [PATCH 4/5] Simplify algorithm a little --- Lib/pathlib.py | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 0154167679836a..6383621136f402 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -638,15 +638,14 @@ def relative_to(self, *other, walk_up=False): raise TypeError("need at least one argument") path_cls = type(self) other = path_cls(*other) - if path_cls(self.anchor) != path_cls(other.anchor): + for step, path in enumerate([other] + list(other.parents)): + if self.is_relative_to(path): + break + else: raise ValueError("{!r} and {!r} have different anchors".format(str(self), str(other))) - if not self.is_relative_to(other) and not walk_up: + if step and not walk_up: raise ValueError("{!r} is not in the subpath of {!r}".format(str(self), str(other))) - walk_up_steps = 0 - while not self.is_relative_to(other): - other = other.parent - walk_up_steps += 1 - parts = ('..',) * walk_up_steps + self.parts[len(other.parts):] + parts = ('..',) * step + self.parts[len(path.parts):] return path_cls(*parts) def is_relative_to(self, *other): From a3e551768995ab01c991469e617e843b3c02c264 Mon Sep 17 00:00:00 2001 From: Barney Gale Date: Sat, 19 Nov 2022 01:11:21 +0000 Subject: [PATCH 5/5] Use f-strings for exception messages --- Lib/pathlib.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/pathlib.py b/Lib/pathlib.py index 6383621136f402..fc20a49b338374 100644 --- a/Lib/pathlib.py +++ b/Lib/pathlib.py @@ -642,9 +642,9 @@ def relative_to(self, *other, walk_up=False): if self.is_relative_to(path): break else: - raise ValueError("{!r} and {!r} have different anchors".format(str(self), str(other))) + raise ValueError(f"{str(self)!r} and {str(other)!r} have different anchors") if step and not walk_up: - raise ValueError("{!r} is not in the subpath of {!r}".format(str(self), str(other))) + raise ValueError(f"{str(self)!r} is not in the subpath of {str(other)!r}") parts = ('..',) * step + self.parts[len(path.parts):] return path_cls(*parts)