Skip to content

bpo-30177: pathlib: include the full path in resolve(strict=False) #1893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 7, 2017
Merged

bpo-30177: pathlib: include the full path in resolve(strict=False) #1893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 8 additions & 11 deletions Lib/pathlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,19 +183,18 @@ def resolve(self, path, strict=False):
if strict:
return self._ext_to_normal(_getfinalpathname(s))
else:
tail_parts = [] # End of the path after the first one not found
while True:
try:
s = self._ext_to_normal(_getfinalpathname(s))
except FileNotFoundError:
previous_s = s
s = os.path.dirname(s)
s, tail = os.path.split(s)
Copy link
Member

Choose a reason for hiding this comment

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

pathlib never uses os.path.split or os.path.join as they might not return the right results.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So os.path.split doesn't work but os.path.dirname and os.path.basename can be used?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm... You're right that it's incoherent. Did you test resolving weird paths such as UNC paths? Because AFAIR that's where the issues can lie.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code I'm replacing was already using os.path.dirname so I'm not sure whether the code was already wrong before or if this function works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I haven't tried myself but I've seen some tests that did things like that for resolve()... I'll check later.

Copy link
Contributor Author

@seirl seirl Jun 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@pitrou I did some tests and I'm really not sure what the problem is with UNC paths... I think if there is a problem, it won't be introduced by this patch anyway as this code already uses os.path.dirname, so it's probably irrelevant?

Since the goal is to backport that fix, I thought it would be best to keep it as simple as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for testing. You're right, if we don't risk a regression then your strategy is ok.

Copy link
Member

Choose a reason for hiding this comment

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

Though for clarity, I'm fairly sure I introduced the call to dirname without knowing that we should avoid it. But it should be safer than split and join because I don't think those necessarily handle drive/machine names properly while dirname certainly should.

tail_parts.append(tail)
if previous_s == s:
return path
else:
if previous_s is None:
return s
else:
return s + os.path.sep + os.path.basename(previous_s)
return os.path.join(s, *reversed(tail_parts))
# Means fallback on absolute
return None

Expand Down Expand Up @@ -326,12 +325,10 @@ def _resolve(path, rest):
try:
target = accessor.readlink(newpath)
except OSError as e:
if e.errno != EINVAL:
if strict:
raise
else:
return newpath
# Not a symlink
if e.errno != EINVAL and strict:
raise
# Not a symlink, or non-strict mode. We just leave the path
# untouched.
path = newpath
else:
seen[newpath] = None # not resolved symlink
Expand Down
21 changes: 12 additions & 9 deletions Lib/test/test_pathlib.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1492,10 +1492,10 @@ def test_resolve_common(self):
os.path.join(BASE, 'foo'))
p = P(BASE, 'foo', 'in', 'spam')
self.assertEqual(str(p.resolve(strict=False)),
os.path.join(BASE, 'foo'))
os.path.join(BASE, 'foo', 'in', 'spam'))
p = P(BASE, '..', 'foo', 'in', 'spam')
self.assertEqual(str(p.resolve(strict=False)),
os.path.abspath(os.path.join('foo')))
os.path.abspath(os.path.join('foo', 'in', 'spam')))
# These are all relative symlinks
p = P(BASE, 'dirB', 'fileB')
self._check_resolve_relative(p, p)
Expand All @@ -1507,16 +1507,18 @@ def test_resolve_common(self):
self._check_resolve_relative(p, P(BASE, 'dirB', 'fileB'))
# Non-strict
p = P(BASE, 'dirA', 'linkC', 'fileB', 'foo', 'in', 'spam')
self._check_resolve_relative(p, P(BASE, 'dirB', 'fileB', 'foo'), False)
self._check_resolve_relative(p, P(BASE, 'dirB', 'fileB', 'foo', 'in',
'spam'), False)
p = P(BASE, 'dirA', 'linkC', '..', 'foo', 'in', 'spam')
if os.name == 'nt':
# In Windows, if linkY points to dirB, 'dirA\linkY\..'
# resolves to 'dirA' without resolving linkY first.
self._check_resolve_relative(p, P(BASE, 'dirA', 'foo'), False)
self._check_resolve_relative(p, P(BASE, 'dirA', 'foo', 'in',
'spam'), False)
else:
# In Posix, if linkY points to dirB, 'dirA/linkY/..'
# resolves to 'dirB/..' first before resolving to parent of dirB.
self._check_resolve_relative(p, P(BASE, 'foo'), False)
self._check_resolve_relative(p, P(BASE, 'foo', 'in', 'spam'), False)
# Now create absolute symlinks
d = tempfile.mkdtemp(suffix='-dirD')
self.addCleanup(support.rmtree, d)
Expand All @@ -1526,16 +1528,17 @@ def test_resolve_common(self):
self._check_resolve_absolute(p, P(BASE, 'dirB', 'fileB'))
# Non-strict
p = P(BASE, 'dirA', 'linkX', 'linkY', 'foo', 'in', 'spam')
self._check_resolve_relative(p, P(BASE, 'dirB', 'foo'), False)
self._check_resolve_relative(p, P(BASE, 'dirB', 'foo', 'in', 'spam'),
False)
p = P(BASE, 'dirA', 'linkX', 'linkY', '..', 'foo', 'in', 'spam')
if os.name == 'nt':
# In Windows, if linkY points to dirB, 'dirA\linkY\..'
# resolves to 'dirA' without resolving linkY first.
self._check_resolve_relative(p, P(d, 'foo'), False)
self._check_resolve_relative(p, P(d, 'foo', 'in', 'spam'), False)
else:
# In Posix, if linkY points to dirB, 'dirA/linkY/..'
# resolves to 'dirB/..' first before resolving to parent of dirB.
self._check_resolve_relative(p, P(BASE, 'foo'), False)
self._check_resolve_relative(p, P(BASE, 'foo', 'in', 'spam'), False)

@support.skip_unless_symlink
def test_resolve_dot(self):
Expand All @@ -1549,7 +1552,7 @@ def test_resolve_dot(self):
r = q / '3' / '4'
self.assertRaises(FileNotFoundError, r.resolve, strict=True)
# Non-strict
self.assertEqual(r.resolve(strict=False), p / '3')
self.assertEqual(r.resolve(strict=False), p / '3' / '4')

def test_with(self):
p = self.cls(BASE)
Expand Down
1 change: 1 addition & 0 deletions Misc/ACKS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,7 @@ Steve Piercy
Jim St. Pierre
Dan Pierson
Martijn Pieters
Antoine Pietri
Anand B. Pillai
François Pinard
Tom Pinckney
Expand Down