Repository navigation
Fix path assignment losing absoluteness - #202
Closed
afonsojanu wants to merge 1 commit into
Closed
afonsojanu wants to merge 1 commit into
afonsojanu wants to merge 1 commit into
Conversation
Reassigning a Path back onto furl.path, like `f.path = f.path / 'c'`, was silently turning an absolute path relative. The culprit is in Path.load(): when it's handed a Path instance rather than a string, it reuses that Path's .segments to figure out whether the result should be absolute, checking for a leading empty segment. That check only works for strings, though, since a Path's .segments never keeps that leading '' around (it gets popped off once the path is parsed and only gets reinserted temporarily inside __str__ for rendering). So a Path built from an already-absolute path would come back through load() as if it had been relative all along. The fix reads .isabsolute straight off the source Path instead of trying to reconstruct it from segments that don't carry that information for this input type. Added a test covering both the absolute and relative round trip through this exact assignment pattern, plus the file:// reproducer from the original report.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #144.
Reassigning a
Pathback ontofurl.path, likef.path = f.path / 'c', silently turns an absolute path relative:The bug is in
Path.load(). When it's handed aPathinstance (rather than a string), it tries to inferisabsoluteby checking whethersegments[0] == '', which is how a parsed path string signals absoluteness. APathobject's own.segmentsnever carries that leading empty entry though, it gets popped off during parsing and only gets reinserted temporarily inside__str__for rendering. So loading from aPathinstance always computedisabsoluteasFalse, regardless of what the source path actually was.The fix reads
.isabsolutedirectly off the sourcePathwhen that's what's being loaded, instead of trying to reconstruct it from segments that don't encode it for this input type. String and list inputs are untouched, same behavior as before.Added a test covering the exact assignment pattern from the issue (both the absolute
file:///a/bcase and a relative one), and confirmed it fails on master and passes with the fix.Ran the full suite locally: 76 passed, plus this new one, and 2 pre-existing IPv6-related failures unrelated to this change (already tracked in #182, happens on master too regardless of this patch).