path: fix win32.relative() for some Unicode paths #27662

mscdex · 2019-05-12T20:45:29Z

This is an alternative to #27644 that avoids any performance regressions in cases where lowercased versions of paths do not differ in length to the original.

The only minor issue (in practice it may not really matter) is that return values are always returned normalized using canonical composition if the lowercased version differed in length to the original, which may be unexpected if you passed in a path that was normalized using canonical decomposition for example.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Fixes: nodejs#27534

nodejs-github-bot · 2019-05-12T20:45:31Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3494

nodejs-github-bot · 2019-05-12T20:46:15Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23069/

mscdex · 2019-05-12T21:40:28Z

Perhaps for builds without intl we could utilize a polyfill, such as unorm.

mscdex · 2019-05-12T21:47:59Z

Alternatively we could ignore additional code points and only compare base symbols, but I'm not sure if that's how Windows/NTFS/etc. compares characters in paths.

jdalton

I'd rather see more work on the original PR as it's a first time contributor instead of jumping to this one. Let's hold off for the time being.

BridgeAR

This does not fix all edge cases. There could theoretically be a combination of the Russian I and the German ß. Those together could return the identical length after lowercasing the input and then still producing wrong results.
Update: I've mistaken.

mscdex · 2019-05-22T04:43:42Z

@BridgeAR do you have a specific test case in mind?

BridgeAR · 2019-05-22T09:46:35Z

@mscdex sorry I made a mistake while looking at this earlier. It should indeed work properly in all cases (if intl is available).
About the polyfill: we could also change the algorithm itself to better reflect what we want in case the length is not identical. Using that would also prevent the edge case mentioned above.

One of these implementations could e.g. look like: "loop over the original first argument and slice out parts, lowercase them and compare them with the other side in case you find a slash." That should be almost as fast as the generic implementation.

lundibundi · 2020-08-25T19:55:16Z

ping @mscdex @BridgeAR @jdalton is this ready basically but needs reviews?

mscdex · 2020-08-25T20:10:37Z

@lundibundi I guess?

lundibundi · 2020-09-02T12:26:30Z

@jdalton I think your request changes can now be dismissed?

ping @nodejs/path

nodejs-github-bot · 2020-09-02T12:27:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33003/

nodejs-github-bot · 2020-09-06T09:45:53Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33075/

whyboris · 2020-10-13T17:43:25Z

Seems like it would fix a crash in my app for users with İ in the file path:
whyboris/Video-Hub-App#533 (comment)

BridgeAR

LGTM

Please take another look. The alternative PR is long closed.

whyboris

I don't have enough background to confirm the code is 💯 but as far as I can tell it's good 👍

aduh95 · 2020-10-20T08:47:12Z

@mscdex The CI is failing consistently on very specific jobs, I'm not sure if this needs action or is just flaky CI.

nodejs-github-bot · 2020-10-22T18:27:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33803/

mscdex · 2020-10-22T22:25:56Z

@aduh95 The error in the intl-less build environment makes sense. I don't think we came to a consensus about how to handle that situation. I suggested using a pure JS polyfill for a consistent experience whether intl is available or not. @BridgeAR had a separate suggestion. Maybe someone else has additional input into this or is ok with one of these solutions?

github-actions · 2020-12-15T21:27:57Z

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

whyboris · 2020-12-15T21:42:39Z

@mscdex -- could you resolve the conflicting files (currently only lib/path.js)

@jdalton -- is there any reason to delay merging after the merge conflicts are resolved? 🙏

monoblaine · 2021-01-30T17:10:56Z

@whyboris I'll be happy to cherry-pick @mscdex's commit, resolve the conflicts and create another PR if they're not available at the moment.

I've been using a custom-built (using #27644) node for almost two years and I really want this officially fixed. 😐

mscdex · 2021-01-30T21:28:25Z

@monoblaine the issue about what to do about intl-less node builds hasn't been resolved yet, so resolving merge conflicts isn't worthwhile at this point

monoblaine · 2021-02-07T18:13:31Z

Oh, NOW I understand the problem here. (It took only two years)

A note to my future self and other fast learners like me:

Node uses ICU to implement internationalization support.
This significantly increases the file size.
Therefore node has the option to make a build wihout intl support.
The changes introduced in this PR is dependent on intl support's existence and that's a problem. (An alternative solution is proposed but not yet discussed.)

bnb · 2022-01-11T20:02:56Z

@mscdex is this PR still something that can/should land? Is there anything that's currently blocking?

mscdex · 2022-01-11T22:05:54Z

@bnb

is this PR still something that can/should land?

I'm not really the one to ask, I was just providing an alternative solution to an issue.

Is there anything that's currently blocking?

#27662 (comment)

Mifrill · 2023-11-03T21:15:32Z

lib/path.js

+      fromOrig = fromOrig.normalize('NFD');
+      from = fromOrig.toLowerCase();
+    }
+    let toNormalized;


⚙️ (optional) Let's use false rather than undefined since it's boolean type:

Suggested change

let toNormalized;

let toNormalized = false;

Mifrill · 2023-11-03T21:17:08Z

lib/path.js


    if (fromOrig === toOrig)
      return '';

    from = fromOrig.toLowerCase();
    to = toOrig.toLowerCase();

+    if (fromOrig.length !== from.length) {
+      fromOrig = fromOrig.normalize('NFD');


shouldn't be NFC?

BridgeAR · 2024-08-06T09:49:28Z

Superseded by another PR

path: fix win32.relative() for some Unicode paths

d02c703

Fixes: nodejs#27534

mscdex added the path Issues and PRs related to the path subsystem. label May 12, 2019

jdalton previously requested changes May 15, 2019

View reviewed changes

BridgeAR reviewed May 21, 2019

View reviewed changes

orta mentioned this pull request Aug 2, 2019

createGetCanonicalFileName yields non-existent paths on Windows if the path contains certain unicode letters microsoft/TypeScript#31819

Closed

Trott force-pushed the master branch from 1ecc406 to 49cf67e Compare September 17, 2019 16:51

devnexen force-pushed the master branch from e8a4568 to 5289f80 Compare December 26, 2019 19:46

BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19

jasnell added the stalled Issues and PRs that are stalled. label Jul 7, 2020

monoblaine mentioned this pull request Jul 12, 2020

path: fix unicode path problems in path.relative #27644

Closed

3 tasks

lundibundi approved these changes Sep 2, 2020

View reviewed changes

lundibundi added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2020

BridgeAR approved these changes Oct 13, 2020

View reviewed changes

whyboris approved these changes Oct 13, 2020

View reviewed changes

aduh95 removed the stalled Issues and PRs that are stalled. label Oct 19, 2020

aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 19, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 19, 2020

This comment has been minimized.

Sign in to view

whyboris mentioned this pull request Oct 22, 2020

Maximum call stack size exceeded whyboris/Video-Hub-App#533

Closed

aduh95 mentioned this pull request Nov 3, 2020

[Proposal] Deprecate --without-intl compilation flag #35942

Open

jasnell added stalled Issues and PRs that are stalled. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Dec 15, 2020

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

bpasero mentioned this pull request Feb 17, 2023

extUri.relativePath returns wrong value if the path has UTF characters microsoft/vscode#174693

Closed

Mifrill reviewed Nov 3, 2023

View reviewed changes

avivkeller mentioned this pull request Jul 22, 2024

path: fix relative on Windows #53991

Merged

BridgeAR closed this Aug 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

path: fix win32.relative() for some Unicode paths #27662

path: fix win32.relative() for some Unicode paths #27662

mscdex commented May 12, 2019

nodejs-github-bot commented May 12, 2019

nodejs-github-bot commented May 12, 2019

mscdex commented May 12, 2019

mscdex commented May 12, 2019

jdalton left a comment

BridgeAR left a comment •

edited

Loading

mscdex commented May 22, 2019

BridgeAR commented May 22, 2019

lundibundi commented Aug 25, 2020

mscdex commented Aug 25, 2020

lundibundi commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 6, 2020

whyboris commented Oct 13, 2020

BridgeAR left a comment

whyboris left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aduh95 commented Oct 20, 2020

nodejs-github-bot commented Oct 22, 2020

mscdex commented Oct 22, 2020 •

edited

Loading

github-actions bot commented Dec 15, 2020

whyboris commented Dec 15, 2020

monoblaine commented Jan 30, 2021

mscdex commented Jan 30, 2021

monoblaine commented Feb 7, 2021

bnb commented Jan 11, 2022

mscdex commented Jan 11, 2022

Mifrill Nov 3, 2023

Mifrill Nov 3, 2023

BridgeAR commented Aug 6, 2024

path: fix win32.relative() for some Unicode paths #27662

path: fix win32.relative() for some Unicode paths #27662

Conversation

mscdex commented May 12, 2019

Checklist

nodejs-github-bot commented May 12, 2019

nodejs-github-bot commented May 12, 2019

mscdex commented May 12, 2019

mscdex commented May 12, 2019

jdalton left a comment

Choose a reason for hiding this comment

BridgeAR left a comment • edited Loading

Choose a reason for hiding this comment

mscdex commented May 22, 2019

BridgeAR commented May 22, 2019

lundibundi commented Aug 25, 2020

mscdex commented Aug 25, 2020

lundibundi commented Sep 2, 2020

nodejs-github-bot commented Sep 2, 2020

nodejs-github-bot commented Sep 6, 2020

whyboris commented Oct 13, 2020

BridgeAR left a comment

Choose a reason for hiding this comment

whyboris left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aduh95 commented Oct 20, 2020

nodejs-github-bot commented Oct 22, 2020

mscdex commented Oct 22, 2020 • edited Loading

github-actions bot commented Dec 15, 2020

whyboris commented Dec 15, 2020

monoblaine commented Jan 30, 2021

mscdex commented Jan 30, 2021

monoblaine commented Feb 7, 2021

bnb commented Jan 11, 2022

mscdex commented Jan 11, 2022

Mifrill Nov 3, 2023

Choose a reason for hiding this comment

Mifrill Nov 3, 2023

Choose a reason for hiding this comment

BridgeAR commented Aug 6, 2024

BridgeAR left a comment •

edited

Loading

mscdex commented Oct 22, 2020 •

edited

Loading