CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
path,win: fix bug in resolve and normalize #55623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
path,win: fix bug in resolve and normalize #55623
Conversation
assert.strictEqual(path.win32.toNamespacedPath('//foo//bar'), | ||
'\\\\?\\UNC\\foo\\bar\\'); | ||
assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo\\'); | ||
assert.strictEqual(path.win32.toNamespacedPath('\\\\?\\foo'), '\\\\?\\foo'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is stripping the / okay? We wouldn't want to repeat that happened with npm 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be OK since this PR doesn't change anything other than this particular path format. But yes, it would be useful to see the citgm test results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, a CITGM would be nice. I've applied the labels. If possible, could you also run npm's test suite? The CITGM doesn't cover all of it, and IMO we should be extra cautious JIC.
Although, it seems like it should be fine. Just want to confirm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the test in the npm repo locally and didn't encounter any problems.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #55623 +/- ##
=======================================
Coverage 88.43% 88.43%
=======================================
Files 654 654
Lines 187698 187729 +31
Branches 36126 36135 +9
=======================================
+ Hits 165986 166024 +38
Misses 14953 14953
+ Partials 6759 6752 -7
🚀 New features to boost your workflow:
|
CITGM ( SUCCESS: No new failures in 3501 compared to 3499 🎉 |
Landed in 7f68e54 |
Fixes: nodejs#54025 PR-URL: nodejs#55623 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
I had to drop it from 22.14.0 proposal as the following test was failing:
|
Fixes: nodejs#54025 PR-URL: nodejs#55623 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
This fixes local flash (and all other flashes) on windows machines after a bugfix in a issue coming from nodejs See: nodejs/node#55623 See: #2959 Change-type: patch
This PR fixes resolving device paths like
\\.\PHYSICALDRIVE
.The
resolve
andnormalize
functions were adding a trailing backslash, considering that this device path was a UNC path. However, device paths are different than UNC paths.In addition to fixing this issue, the previous attempt also fixed other inconsistencies. So, this PR only fixes the issue itself.
Previous PR: #54224
Fixes: #54025