Repository navigation
fix: LocalPath.IsPrefixOf for root paths - #223
Merged
Merged
Conversation
ForNeVeR
self-requested a review
September 19, 2026 13:14
A root path like 'A:\' (Windows) or '/' (Unix) ends with the directory separator, so the next character in the child path is not a separator and the old check failed. Adds the same 'value ends with separator' clause that AbsolutePath.IsPrefixOf already has, plus the rooted-path test from the issue and a Unix InlineData row. Closes ForNeVeR#221
ForNeVeR
force-pushed
the
fix/isprefixof-root
branch
from
September 19, 2026 19:34
502f72d to
da11cb6
Compare
An empty path is the normalized form of "", "." and "a/..", and is also the parent of any single-segment relative path, so it reaches IsPrefixOf easily. The trailing-separator check added by the first commit of PR ForNeVeR#223 indexed Value[-1] on it and threw IndexOutOfRangeException. It now answers true: the empty path is the current directory, and so a prefix of every relative path.
An absolute path is never a prefix of a relative one, nor the other way round: deciding that would mean resolving the relative path against the current directory, which this library never does. Previously the answer fell out of an incidental string comparison, so "." was reported as a prefix of "/foo" (every string starts with the empty one). The IsAbsolute check is only an approximation of the four kinds of path a LocalPath can hold; see the TODOs for ForNeVeR#224. The ordinal comparison it guards disagrees with Equals on Windows and macOS: TODO for ForNeVeR#225.
Both types implemented StartsWith as a raw string test, so it ignored
segment boundaries: "/foo1".StartsWith("/foo") was true while
"/foo".IsPrefixOf("/foo1") was false. Issue ForNeVeR#43 asked for the two to be
mirrors of each other, so express one through the other.
BREAKING: StartsWith now compares whole path segments on both LocalPath
and AbsolutePath.
AbsolutePath already wraps a LocalPath and delegates Value, Parent and FileName to it, but carried a character-for-character copy of the prefix algorithm, so every fix had to be made twice. Delegate instead. The empty-path and absoluteness branches are unreachable here, since an AbsolutePath is always rooted and never empty. IsPrefixOfShouldBeEquivalentToStartsWith compared against the string method rather than the path one, which is the very semantics that just changed; it now tests the property its name claims, on both platforms.
The empty path is the current directory, but the arm added for it in this PR answered true for any relative path, so "" (equally ".", "a/.." or the parent of any single-segment relative path) was reported as a prefix of "../evil", which sits outside the current directory. A caller using IsPrefixOf or StartsWith as a containment check accepted the escape. Reported as ../evil rather than found by the tests, none of which exercised a ".." path.
The single-argument string.StartsWith overload compares under the current culture, which ignores collation-ignorable characters, so "foo" was accepted as a prefix of "fooo/bar" - and likewise for AbsolutePath, which now delegates here. This also makes the TODO for ForNeVeR#225 true as written: what remains is the mismatch between this ordinal comparison and the platform-default one that Equals uses.
Once StartsWith is defined as other.IsPrefixOf(this), comparing a.IsPrefixOf(b) with b.StartsWith(a) compares a value with itself, so the test could not fail for any input. Give every row an expected result, and assert both directions against it: the equivalence is still stated, but it is now falsifiable.
ForNeVeR
force-pushed
the
fix/isprefixof-root
branch
from
September 19, 2026 23:40
de56dcb to
fe14595
Compare
ForNeVeR
approved these changes
Sep 19, 2026
ForNeVeR
left a comment
ForNeVeR
left a comment
Owner
There was a problem hiding this comment.
Thanks! I found a whole can of worms in various components during review, and decided to address some bits in this branch.
ForNeVeR
added a commit
that referenced
this pull request
Sep 19, 2026
An empty path is the normalized form of "", "." and "a/..", and is also the parent of any single-segment relative path, so it reaches IsPrefixOf easily. The trailing-separator check added by the first commit of PR #223 indexed Value[-1] on it and threw IndexOutOfRangeException. It now answers true: the empty path is the current directory, and so a prefix of every relative path.
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
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.
Problem
LocalPath.IsPrefixOfreturnedfalsefor a root prefix:new LocalPath("/").IsPrefixOf(new LocalPath("/frob"))— because for a root path the character after the prefix is not a separator.Fix
Adds the same
Value[Value.Length - 1] == Separatorclause thatAbsolutePath.IsPrefixOfalready uses, plus the rooted-path test from the issue and a UnixInlineData("/", "/frob", true)row.Note: I couldn't run the .NET suite locally (SDK pin 10.0.401 vs installed 10.0.201 on this machine) — relying on CI for verification.
Closes #221