Skip to content
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.

Sign up for GitHub

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

Jump to bottom

path: update win32 toNamespacedPath to support device namespace paths #54367

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

path: update win32 toNamespacedPath to support device namespace paths #54367

wants to merge 1 commit into from
+176 −4

Conversation

EarlyRiser42
Copy link
Contributor

@EarlyRiser42 EarlyRiser42 commented Aug 14, 2024 •
edited
Loading

Updated the win32 toNamespacedPath function to support device namespace paths as per #54180 and #54161.
url.pathToFileURL was not modified since device namespace paths are not valid file URIs.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels Aug 14, 2024
@codecov Codecov
Copy link

codecov bot commented Aug 14, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 85.00000% with 9 lines in your changes missing coverage. Please review.

Project coverage is 88.23%. Comparing base (880c446) to head (9eb494a).
Report is 477 commits behind head on main.

Files with missing lines Patch % Lines
src/path.cc 57.14% 6 Missing and 3 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54367      +/-   ##
==========================================
+ Coverage   87.09%   88.23%   +1.14%     
==========================================
  Files         648      651       +3     
  Lines      182216   183895    +1679     
  Branches    34965    35847     +882     
==========================================
+ Hits       158704   162267    +3563     
+ Misses      16785    14913    -1872     
+ Partials     6727     6715      -12
Files with missing lines Coverage Δ
lib/path.js 95.61% <100.00%> (+0.44%) ⬆️
src/path.cc 67.39% <57.14%> (-1.66%) ⬇️

... and 248 files with indirect coverage changes

@avivkeller avivkeller added the windows Issues and PRs related to the Windows platform. label Aug 14, 2024
avivkeller
avivkeller reviewed Aug 14, 2024
View reviewed changes
lib/path.js Outdated
@@ -687,6 +707,19 @@ const win32 = {
if (typeof path !== 'string' || path.length === 0)
return path;

// Check if the path matches any device pattern
if (ObjectValues(windowDevicePatterns).some((pattern) => pattern.test(path))) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be faster if the object was an array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you're correct. My local benchmarks confirm that using an array is indeed faster than using an object. Thank you very much for your thorough review. I will take this and apply the changes accordingly

@EarlyRiser42 EarlyRiser42 force-pushed the namedspace branch 2 times, most recently from 5000736 to d347f33 Compare August 14, 2024 12:29
@avivkeller avivkeller added the needs-benchmark-ci PR that need a benchmark CI run. label Aug 14, 2024
avivkeller
avivkeller reviewed Aug 14, 2024
View reviewed changes
lib/path.js Outdated
@@ -180,6 +180,25 @@ function glob(path, pattern, windows) {
});
}

// Regular expressions to identify special device names in Windows.
// Ref: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea
// COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.
//
// COM to AUX (e.g., COM1, LPT1, NUL, CON, PRN, AUX) are reserved OS device names.

This might help to separate the ideas

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're absolutely right. Moreover, the code I recently added is currently checking all regular expressions in the device namespace, which has significantly reduced performance. As you suggested, I will try to improve performance by distinguishing between special devices and reserved devices during the inspection. Thank you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments to differentiate between reserved and non-reserved device names, explaining the differences between regular expressions like /([\\/])?(COM\d+)$/i and /^(PHYSICALDRIVE\d+)$/i. While I tried to maintain performance while addressing the issues, I couldn't find a way to do so effectively because reserved device names can also appear in paths (e.g., C:/path/COM1). If anyone has a better solution that maintains performance while resolving this, I welcome suggestions.

@targos
Copy link
Member

targos commented Aug 15, 2024

I believe this is already being fixed in #54224

@EarlyRiser42
Copy link
Contributor Author

Thank you for let me know sir. I'll close this PR

@EarlyRiser42 EarlyRiser42 deleted the namedspace branch August 15, 2024 07:23
@EarlyRiser42
Copy link
Contributor Author

sir Im bit confused that #54224 seems to be focused on fixing an issue where a trailing backslash was incorrectly added in the resolve function, whereas my PR is specifically aimed at converting file paths to namespace paths in the tonamespaced function. It seems these are addressing different concerns..?

@huseyinacacak-janea
Copy link
Contributor

I would work on this issue after finishing my current work. So, I reviewed this PR.

This PR fixes a different problem than #54224 and LGTM.

I think the author can continue working on this by reopening it.

@huseyinacacak-janea
Copy link
Contributor

huseyinacacak-janea commented Aug 15, 2024 •
edited
Loading

I believe this PR could fix #54161 as well. The conout$ and conin$ could be added as regular expressions. What do you think?
Refs: https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file
Refs: https://learn.microsoft.com/en-us/windows/console/console-handles

@EarlyRiser42
Copy link
Contributor Author

Thank you for your valuable feedback. I initially included only CON as a regular expression, but I completely agree that conout$ and conin$ should also be added as regular expressions. I will reopen this PR tomorrow. I appreciate your guidance.

@EarlyRiser42 EarlyRiser42 restored the namedspace branch August 16, 2024 01:05
@EarlyRiser42 EarlyRiser42 reopened this Aug 16, 2024
@EarlyRiser42 EarlyRiser42 force-pushed the namedspace branch 2 times, most recently from 9b397f6 to aed9a78 Compare August 16, 2024 01:45
@targos
Copy link
Member

targos commented Aug 16, 2024

Note that toNamespacedPath also has a C++ implementation that would have to be updated as well.

@EarlyRiser42
Copy link
Contributor Author

Thank you for let me know. I just updated C++ implementation as well.

@EarlyRiser42 EarlyRiser42 force-pushed the namedspace branch 2 times, most recently from 09f9091 to 0f8415f Compare August 19, 2024 01:55
@EarlyRiser42
Copy link
Contributor Author

@targos I made the changes based on your feedback. Could you review this PR when you have a moment?

@EarlyRiser42 EarlyRiser42 mentioned this pull request Aug 28, 2024
Open
2 tasks
@daeyeon daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 29, 2024
@nodejs-github-bot
Copy link
Collaborator

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

daeyeon
daeyeon reviewed Aug 31, 2024
View reviewed changes
lib/path.js Outdated Show resolved Hide resolved
src/path.cc Outdated Show resolved Hide resolved
@daeyeon daeyeon added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Sep 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 1, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@daeyeon daeyeon removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 2, 2024
@EarlyRiser42 EarlyRiser42 force-pushed the namedspace branch 4 times, most recently from 19f95e2 to fb05e6e Compare September 24, 2024 09:51
@daeyeon
Copy link
Member

daeyeon commented Sep 25, 2024

Could you update the doc also?

@daeyeon daeyeon added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Sep 25, 2024
@EarlyRiser42
Copy link
Contributor Author

Of course. Thank you for let me know. I updated the doc also according to your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avivkeller avivkeller avivkeller left review comments

@daeyeon daeyeon daeyeon approved these changes

Assignees
No one assigned
Labels
needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. semver-minor PRs that contain new features and should be released in the next minor version. windows Issues and PRs related to the Windows platform.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@EarlyRiser42 @targos @huseyinacacak-janea @nodejs-github-bot @daeyeon @avivkeller