gh-105002: [pathlib] Fix relative_to with walk_up=True using ".."

[kukovecz]

For absolute paths, this was working as intended, only new test cases were added.

For relative paths it makes sense to raise an Error because ".." can not be resolved and the current working directory is unknown.

• Issue: pathlib.PurePath.relative_to(walk_up=True) mishandles '..' components #105002

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[ghost]

All commit authors signed the Contributor License Agreement.

[kukovecz]

CC: @pablogsal

[barneygale]

Thanks very much for taking a look at this

I think the exception message is wrong here:

>>> PurePath("a/b/c").relative_to("a/b/..", walk_up=False)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/barney/projects/cpython/Lib/pathlib.py", line 637, in relative_to
    raise ValueError(f"'..' segment in {str(other)!r} cannot be walked")
ValueError: '..' segment in 'a/b/..' cannot be walked

When we set walk_up=False (the default), we shouldn't get an error about the '..' segment not being walkable, because we haven't asked to walk it.

Something like this might work better:

diff --git a/Lib/pathlib.py b/Lib/pathlib.py
index 8ff4d4ea19..c83cf3d2ef 100644
--- a/Lib/pathlib.py
+++ b/Lib/pathlib.py
@@ -633,10 +633,12 @@ def relative_to(self, other, /, *_deprecated, walk_up=False):
         for step, path in enumerate([other] + list(other.parents)):
             if self.is_relative_to(path):
                 break
+            elif not walk_up:
+                raise ValueError(f"{str(self)!r} is not in the subpath of {str(other)!r}")
+            elif path.name == '..':
+                raise ValueError(f"'..' segment in {str(other)!r} cannot be walked")
         else:
             raise ValueError(f"{str(self)!r} and {str(other)!r} have different anchors")
-        if step and not walk_up:
-            raise ValueError(f"{str(self)!r} is not in the subpath of {str(other)!r}")
         parts = ['..'] * step + self._tail[len(path._tail):]
         return self.with_segments(*parts)
 

[kukovecz]

Thank you @barneygale for the fast reponse! Absolutely makes sense, I have applied your requested changes.

[barneygale]

Thanks for making that change!

The bug seems to be reproducible with absolute paths too:

>>> PurePath('/a/b').relative_to('/a/..', walk_up=True)
PurePosixPath('../b')

So it would be good to add a test case for that, and adjust the news entry to remove the mention of relative paths.

Also, the bug only occurs when users set walk_up=True. I think that's important to mention in the news entry.

[kukovecz]

Thank you, I appreciate these suggestions!

I have modified the news entry to better reflect the changes.

Also, for the testing: You were right, sadly I always used other as relative path in the new test asserts. Now maybe a little bit overdone because for both relative and absolute test cases the I have done some asserts for the other to be relative, absolute, with and without walk_up=True, but hopefully now all things are covered.

[barneygale]

Looks good, thank you!

[miss-islington]

Thanks @kukovecz for the PR, and @barneygale for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

[bedevere-bot]

GH-107315 is a backport of this pull request to the 3.12 branch.

[barneygale]

@kukovecz thanks again for this. By the way, we always squash PRs on merge, so there's no need to force-push. It can make PRs a little harder to follow for reviewers.

[kukovecz]

Thanks @barneygale for the quick round of reviews and suggestions along the way.

I was not sure of your settings / policies about updating the PR so I was trying to keep the history acceptable, manually. Maybe I should have read the contributor guide more carefully. I will take extra care next time not make the review harder that it needs to be.

[barneygale]

No worries, it didn't make this review any harder! Just a tip for any future PRs you might make :-)