path: fix bugs and inconsistencies

[huseyinacacak-janea]

This PR fixes resolving device paths like \\.\PHYSICALDRIVE.
The resolve function was adding a trailing backslash, considering that this device path was a UNC path. However, device paths are different than UNC paths.

This PR includes more changes than originally intended because fixing the bug caused inconsistencies between other functions and operating systems. Now, this PR fixes all these problems.

Fixes: #54025

[codecov]

Codecov Report

Attention: Patch coverage is 90.32258% with 12 lines in your changes missing coverage. Please review.

Project coverage is 87.89%. Comparing base (b4fd1fd) to head (34839d8).
Report is 441 commits behind head on main.

Files with missing lines	Patch %	Lines
src/path.cc	38.46%	6 Missing and 2 partials ⚠️
lib/path.js	95.91%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54224      +/-   ##
==========================================
+ Coverage   87.06%   87.89%   +0.83%     
==========================================
  Files         643      651       +8     
  Lines      181576   183425    +1849     
  Branches    34894    35744     +850     
==========================================
+ Hits       158088   161222    +3134     
+ Misses      16759    15476    -1283     
+ Partials     6729     6727       -2     

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	96.19% <100.00%> (+3.26%)	⬆️
lib/internal/url.js	98.70% <ø> (+3.93%)	⬆️
lib/path.js	97.27% <95.91%> (+1.33%)	⬆️
src/path.cc	67.39% <38.46%> (-1.66%)	⬇️

... and 206 files with indirect coverage changes

[lpinca]

I think it should also handle \\?\PHYSICALDRIVE0.

[huseyinacacak-janea]

Thanks for the review. Fixed.

[lpinca]

There are other inconsistencies:

path.resolve('\\\\?\\c:/')    // \\?\c:
path.toNamespacedPath('c:/')  // \\?\c:\

path.normalize('\\\\.\\foo')  // \\.\foo\
path.resolve('\\\\.\\foo\\')  // \\.\foo

[huseyinacacak-janea]

OK, I'll try to fix these inconsistencies. I have a question.
Node currently runs as follows:

path.resolve("C:\\")            // C:\\
path.resolve("//server/share")  // \\\\server\\share\\

But, I think it should be:

path.resolve("C:\\")            // C:
path.resolve("//server/share")  // \\\\server\\share

If I change these ones, resolve could return all the paths without any trailing backslash.
If I don't change them, the inconsistency below will still exist:

path.resolve("//server/share/folder")  // \\\\server\\share\\folder
path.resolve("//server/share")         // \\\\server\\share\\

What do you think?

[lpinca]

I think it should respect the original input. If there is a trailing slash, keep it, otherwise do not add it.

$ python
Python 3.12.4 (tags/v3.12.4:8e8a4ba, Jun  6 2024, 19:30:16) [MSC v.1940 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> from pathlib import PurePath
>>> PurePath('c:/')
PureWindowsPath('c:/')
>>> PurePath('c:')
PureWindowsPath('c:')
>>> PurePath('c:', '\\\\.\\foo')
PureWindowsPath('//./foo')
>>> PurePath('c:', '\\\\.\\foo\\')
PureWindowsPath('//./foo/')
>>>

[tniessen]

AFAIK \\?\... is not supposed to be subject to normalization, see this post.

[huseyinacacak-janea]

My latest commit fixes the inconsistencies in resolve, toNamespacedPath and normalize on Windows. I'm open to suggestions and waiting for your reviews.
If there are no objections, I'll fix the inconsistencies between Windows and Posix that occurred after the last commit.

[huseyinacacak-janea]

Is there anything else I can do to help this PR move forward?
Note that since this PR includes a lot of breaking changes, I think it would be better to label this as semver-major.

[lpinca]

@nodejs/path @nodejs/tsc

[lpinca]

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3474/

[BridgeAR]

LGTM, I just left a few suggestions to simplify the code

[BridgeAR]

I believe it is possible to simplify the code by changing this to a string that we always append to the output and that is either an empty string or the slash.

[BridgeAR]

Same as above

[BridgeAR]

Similar as above

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/54224
✔  Done loading data for nodejs/node/pull/54224
----------------------------------- PR info ------------------------------------
Title      path: fix bugs and inconsistencies (#54224)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     huseyinacacak-janea:huseyin-12382-physicaldrive-open-is-broken -> nodejs:main
Labels     c++, windows, semver-major, path, author ready, needs-citgm
Commits    3
 - path: fix bugs and inconsistencies
 - Update lib/path.js
 - Update src/path.cc
Committers 2
 - Hüseyin Açacak <[email protected]>
 - GitHub <[email protected]>
PR-URL: https://github.com/nodejs/node/pull/54224
Fixes: https://github.com/nodejs/node/issues/54025
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/54224
Fixes: https://github.com/nodejs/node/issues/54025
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Tue, 06 Aug 2024 12:03:37 GMT
   ✔  Approvals: 4
   ✔  - Yagiz Nizipli (@anonrig) (TSC): https://github.com/nodejs/node/pull/54224#pullrequestreview-2266954760
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/54224#pullrequestreview-2304547027
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/54224#pullrequestreview-2288586284
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/54224#pullrequestreview-2288982004
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-09-10T08:43:33Z: https://ci.nodejs.org/job/node-test-pull-request/62236/
   ℹ  Last CITGM CI on 2024-09-06T19:13:49Z: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3474/
- Querying data for job/node-test-pull-request/62236/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
   f468509cf6..c3e1c31baa  main       -> origin/main
✔  origin/main is now up-to-date
main is out of sync with origin/main. Mismatched commits:
 - 6043e9d16c build: upgrade clang-format to v18
 - c3e1c31baa build: upgrade clang-format to v18
--------------------------------------------------------------------------------
HEAD is now at c3e1c31baa build: upgrade clang-format to v18
   ✔  Reset to origin/main
- Downloading patch for 54224
From https://github.com/nodejs/node
 * branch                  refs/pull/54224/merge -> FETCH_HEAD
✔  Fetched commits as c3e1c31baa8f..34839d89e9bb
--------------------------------------------------------------------------------
Auto-merging lib/internal/modules/cjs/loader.js
Auto-merging lib/internal/url.js
Auto-merging lib/path.js
[main f0729d6eab] path: fix bugs and inconsistencies
 Author: Hüseyin Açacak <[email protected]>
 Date: Fri Aug 16 08:55:19 2024 +0300
 9 files changed, 187 insertions(+), 72 deletions(-)
Auto-merging lib/path.js
[main ed8f9f8ac4] Update lib/path.js
 Author: Hüseyin Açacak <[email protected]>
 Date: Mon Sep 9 14:20:38 2024 +0300
 1 file changed, 1 insertion(+), 1 deletion(-)
[main 43d3cbd84f] Update src/path.cc
 Author: Hüseyin Açacak <[email protected]>
 Date: Mon Sep 9 14:20:50 2024 +0300
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)
Executing: git node land --amend --yes

⚠  Found Fixes: #54025, skipping..

--------------------------------- New Message ----------------------------------

path: fix bugs and inconsistencies
Fixes: #54025

PR-URL: #54224

Reviewed-By: Yagiz Nizipli <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Ruben Bridgewater <[email protected]>
[detached HEAD aa222571b5] path: fix bugs and inconsistencies

Author: Hüseyin Açacak <[email protected]>

Date: Fri Aug 16 08:55:19 2024 +0300

9 files changed, 187 insertions(+), 72 deletions(-)

Rebasing (3/6)

Rebasing (4/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update lib/path.js
Co-authored-by: Ruben Bridgewater <[email protected]>

PR-URL: #54224

Fixes: #54025

Reviewed-By: Yagiz Nizipli <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Ruben Bridgewater <[email protected]>
[detached HEAD 6c049d49ab] Update lib/path.js

Author: Hüseyin Açacak <[email protected]>

Date: Mon Sep 9 14:20:38 2024 +0300

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (5/6)

Rebasing (6/6)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update src/path.cc
Co-authored-by: Ruben Bridgewater <[email protected]>

PR-URL: #54224

Fixes: #54025

Reviewed-By: Yagiz Nizipli <[email protected]>

Reviewed-By: Luigi Pinca <[email protected]>

Reviewed-By: James M Snell <[email protected]>

Reviewed-By: Ruben Bridgewater <[email protected]>
[detached HEAD 7a62960edc] Update src/path.cc

Author: Hüseyin Açacak <[email protected]>

Date: Mon Sep 9 14:20:50 2024 +0300

1 file changed, 1 insertion(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/10863285142

[nodejs-github-bot]

Landed in efbba60

[ShenHongFei]

@huseyinacacak-janea @anonrig @lpinca

This change modifies the return results of path.resolve and path.relative, which will cause many third-party libraries to suddenly stop working. Please carefully consider whether you really want to change this way.
I encountered an error with npm publish. The main reason for the impact is that npm's processing of paths depends on resolve and relative functions, which will automatically remove the trailing /

npm publish error log

0 verbose cli D:\1\nodejs\node.exe D:\1\nodejs\node_modules\npm\bin\npm-cli.js
1 info using [email protected]
2 info using [email protected]
3 silly config load:file:D:\1\nodejs\node_modules\npm\npmrc
4 silly config load:file:D:\1\xsh\.npmrc
5 silly config load:file:C:\Users\shf\.npmrc
6 silly config load:file:D:\1\nodejs\etc\npmrc
7 verbose title npm publish
8 verbose argv "publish" "--proxy" "http://127.0.0.1:10080" "--registry" "https://registry.npmjs.org/"
9 verbose logfile logs-max:10 dir:T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-
10 verbose logfile T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-debug-0.log
11 verbose publish [ '.' ]
12 silly logfile start cleaning logs, removing 1 files
13 silly logfile done cleaning log files
14 verbose stack Error: ENOENT: no such file or directory, open 'D:\1\README.md'
14 verbose stack     at async open (node:internal/fs/promises:639:25)
14 verbose stack     at async Object.readFile (node:internal/fs/promises:1239:14)
14 verbose stack     at async normalize (D:\1\nodejs\node_modules\npm\node_modules\@npmcli\package-json\lib\normalize.js:348:26)
14 verbose stack     at async PackageJson.prepare (D:\1\nodejs\node_modules\npm\node_modules\@npmcli\package-json\lib\index.js:266:5)
14 verbose stack     at async #getManifest (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:209:27)
14 verbose stack     at async #publish (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:93:20)
14 verbose stack     at async Publish.exec (D:\1\nodejs\node_modules\npm\lib\commands\publish.js:46:5)
14 verbose stack     at async Npm.exec (D:\1\nodejs\node_modules\npm\lib\npm.js:207:9)
14 verbose stack     at async module.exports (D:\1\nodejs\node_modules\npm\lib\cli\entry.js:74:5)
15 error code ENOENT
16 error syscall open
17 error path D:\1\README.md
18 error errno -4058
19 error enoent ENOENT: no such file or directory, open 'D:\1\README.md'
20 error enoent This is related to npm not being able to find a file.
20 error enoent
21 verbose cwd D:\1\xsh
22 verbose os Windows_NT 10.0.22631
23 verbose node v23.0.0
24 verbose npm  v10.8.3
25 verbose exit -4058
26 verbose code -4058
27 error A complete log of this run can be found in: T:\t\npm-cache\_logs\2024-09-19T16_52_55_988Z-debug-0.log

The relevant code for the error in npm is as follows:

https://github.com/npm/npm-package-arg/blob/a8a9bddc726802fb5ed30f6b113d57f0655bbd51/lib/npa.js#L296

The reason for the error is that the following function returns inconsistent results:

// main branch
path.resolve('D:\\1\\xsh\\') === 'D:\\1\\xsh\\'
path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === '..\\xsh'

// node.js v22.9.0
path.resolve('D:\\1\\xsh\\') === 'D:\\1\\xsh'
path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === ''

[lpinca]

Ok, if it too breaking of a change, I think we should revert efbba60 and reopen #54025.

path.relative('D:\\1\\xsh', 'D:\\1\\xsh\\') === '..\\xsh'

The above result is weird.

[lpinca]

Maybe we can limit the scope and go back to b40e78c.