Repository navigation
Conversation
Avoids computing the same path more than once
|
@freya022 One of the things I'd be concerned with is when the system is running under JRT:// and not JAR:// I imagine these are where the tests are failing looking at the changes |
|
This test may need updating if you added sorting to the jar scan (which may add read times, this library is optimized for speed) Error: Failures: Would be curious as to why they are being sorted now |
Not sure what your concern is, but this PR does not change any behavior, it only memoizes the result and optimizes finding out if a path needs to be sanitized (as most of them don't), but not how they are sanitized. The fail at |
|
(Comment added by Claude) Thanks for this. The PR is in two halves, and they need to be split. The The path.contains("//") || path.contains("!!") || path.contains(".!") || path.contains("./")misses trailing
Those paths would then be compared and cached as-is, so two spellings of the same entry stop matching. Could you resubmit the |
…tPath() call Splits out the sound half of #935. The scanPathRecursively() loop has already computed FastPathResolver.resolve(classpathEltPath.relativize(subPath)) as subPathRelativeStr in order to run the accept/reject checks, and Resource#getPath() then recomputed the identical expression on every call. Pass the resolved string through instead. The other half of #935 (rewriting FileUtils#sanitizeEntryPath to detect segments needing sanitization with a handful of String#contains calls) is not included: it misses trailing '.' and '..' segments, since those are not followed by a separator, so e.g. 'a/b/..' would no longer sanitize to 'a'. Thanks to freya022 for spotting this.
|
(Comment added by Claude) I have gone ahead and done the split myself, so you do not have to -- the The Thanks! |
|
…s char[] copy
sanitizeEntryPath() threw StringIndexOutOfBoundsException for any path that
normalizes away to nothing but slashes ("/..", "/.", "//..", "/a/.." etc.) when
both removeInitialSlash and removeFinalSlash were set: the leading-slash index
was computed first, and stripping the trailing slashes afterwards could shrink
the buffer below it. ScanResult.getResourcesWithPath() and
getResourcesWithPathIgnoringAccept() both call with both flags set, so this was
reachable from the public API. Strip the final slashes first.
Also removes the per-call "new char[path.length()]" copy from the segment scan,
reading through charAt() instead. The common case is that a path needs no
sanitizing at all, so the copy was pure overhead on every call.
The scan itself is deliberately left exact rather than replaced by a
contains()-based test for "./", "..", "//" and the like, as suggested in #935.
Such a test cannot be made equivalent: it misses a trailing "." or ".." segment,
which is not followed by a separator; it cannot tell a nested jar separator "!"
from an ordinary "!" in a filename without consulting nestedJarSepIdx (#903);
and over-triggering is not harmless, because the sanitizing branch also drops
empty segments, so a path like "a./b/" would silently lose its trailing slash.
Adds tests pinning the normalization behaviour: "." and ".." segments are
normalized away wherever they occur including as the final segment, excess ".."
segments cannot escape the top of the hierarchy or the preceding "!" section
marker, empty segments are collapsed, and dots inside a segment name are left
alone.
I've found that quite some time was spent on resolving/sanitizing paths, it did it 5 times for each .class, now this PR brings it to only once.
.jarpaths are still resolved multiple times but I didn't see an easy fix for it, and the impact seems to be minimal.The changes in
FileUtilsonly changes detecting when to sanitize, it should be equivalent to the previous code but much faster.No clue why CI is failing,
mvn clean testpasses on my machine. Also the MacOS and Ubuntu CIs don't show the same errors lol