fix(path): handle lowercasing that changes path length in windows relative() - #7337
Open
Punisheroot wants to merge 3 commits into
Open
Punisheroot wants to merge 3 commits into
Punisheroot wants to merge 3 commits into
Conversation
…ative()
`relative()` lowercases both paths to compare them case-insensitively, then slices the original `to` path using indexes found in the lowercased strings. Some characters change length when lowercased ("İ" becomes "i̇", which is two code units), shifting those indexes and dropping leading characters from the result.
Fall back to comparing path segments when lowercasing changes the length of either path, mirroring the fix merged for nodejs/node#53991. Regression cases are added to path/relative_test.ts.
Fixes denoland#7336
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7337 +/- ##
========================================
Coverage 95.04% 95.04%
========================================
Files 619 619
Lines 52012 51788 -224
Branches 9450 9414 -36
========================================
- Hits 49434 49223 -211
+ Misses 2031 2022 -9
+ Partials 547 543 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
|
This looks good. I noticed that Patch coverage is 67.74% Could you investigate raising it? (ideally over 95% if practically possible) |
Author
|
@tomas-zijdemans done! |
This branch has not been deployed
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.
Summary
relative()from@std/path/windowsdropping leading characters when apath contains a character that changes length when lowercased, such as
İ(U+0130).
length of either resolved path.
Context
relative()lowercases both paths to compare them case-insensitively, thenslices the original
topath using indexes found in the lowercased strings."İ".toLowerCase()is"i̇", which is two code units, so each such character inthe common prefix shifts those indexes and the result loses one leading
character per occurrence:
This also affects ESLint on Windows:
@eslint/config-arrayuses this functionto match ignore patterns against paths relative to the config folder, so
projects under a directory containing
İstop matching**/node_modules/andend up linting all of
node_modules.The fallback mirrors the fix merged upstream for
path.win32.relative()innodejs/node#53991.
Fixes #7336
Changes
path/windows/relative.ts: whenfromOrig/toOrigchange length aftertoLowerCase(), compare path segments instead of using indexes computed onthe lowercased strings.
path/relative_test.ts: add regression cases withİported frompath: fix relative on Windows nodejs/node#53991 plus the scenarios from path:
relative()on Windows drops leading characters when a path contains "İ" #7336, and aßcontrol case.Validation
deno test -A path/: PASS (91 passed, 0 failed)deno test path/relative_test.ts: PASS (fails before the fix withode_modules\\zod\\index.ts)deno check path/windows/relative.ts path/relative_test.ts: PASSdeno lint path/windows/relative.ts path/relative_test.ts: PASSdeno fmt --check: PASSdeno doc --lint path/windows/relative.ts: PASSdeno check --config browser-compat.tsconfig.json path/windows/relative.ts:PASS
deno task testrun locally on Windows: 7424 passed, 156 ignored; the35 failures are all
fs/*symlink tests failing with os error 1314 (symlinkprivileges unavailable on this machine), unrelated to this change.
deno task lint:docsanddeno task test:browsercan't run as-is on thisWindows machine (command-line length limit and missing grep/xargs); the
targeted equivalents above pass and CI covers the full tasks.