fs: fix rmSync to handle non-ASCII characters #56117
Conversation
nodejs-github-bot
added
c++
Yeaseen
force-pushed
the
fix-non-ascii-filepath-issue
branch
from
0486aea to
f4a6ab5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56117 +/- ##
==========================================
+ Coverage 87.99% 88.00% +0.01%
==========================================
Files 656 656
Lines 188929 189137 +208
Branches 35968 36013 +45
==========================================
+ Hits 166253 166456 +203
- Misses 15831 15841 +10
+ Partials 6845 6840 -5
|
|
Can you please update your first commit message to follow the guideline, i.e. each line needs to be below 72 chars. Also there are some linting issues in your js test. |
Update fs.rmSync to properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names. Add a test in test/parallel to ensure that files with non-ASCII characters can be deleted without issues. This covers cases that previously caused unexpected behavior or crashes on certain file systems. Fixes: nodejs#56049
Yeaseen
force-pushed
the
fix-non-ascii-filepath-issue
branch
from
423c7d3 to
f434191
Compare
|
@jazelly I made the updates for the first commit message and linter issues. I think it should be fine now. |
Yeaseen
force-pushed
the
fix-non-ascii-filepath-issue
branch
from
3931460 to
2572f57
Compare
|
@jazelly This time a linter issue: I didn't put a newline at the end of the test file. I am learning new things! Thanks for your understanding. |
|
@jazelly @joyeecheung It's updated based on the review. |
juanarbol
added
the
author ready
This comment was marked as duplicate.
This comment was marked as duplicate.
src/node_file.cc
Outdated
| @@ -1637,11 +1686,13 @@ static void RmSync(const FunctionCallbackInfo<Value>& args) { | |||
| int recursive = args[2]->IsTrue(); | |||
| int retryDelay = args[3].As<Int32>()->Value(); | |||
|
|
|||
| auto file_path_as_str = PathToString(file_path); | |||
There was a problem hiding this comment.
This can be moved into the branch below to avoid excessive conversion costs.
|
|
||
| // Ensure rmSync throws an error when trying to remove a directory without recursive | ||
| assert.throws(() => { | ||
| fs.rmSync(dirPath, { recursive: false }); |
There was a problem hiding this comment.
This only tests the errors from one special path, there are more errors using the string without converting to UTF8...can you add a test to check the errors thrown from line 1747-1764 in node_file.cc?
There was a problem hiding this comment.
Yeah. The conversion wasn't done previously. I added the conversion using PathToString macro. Also, I added a test case in the js for checking not_a_directory error case.
|
@joyeecheung Sorry for late update. It was a long winter vacation and my marriage ceremony. Hence the hiatus. I updated the changes you mentioned. |
This is my first pull request here and I read the contribution guide and commit guide.
Update
fs.rmSyncinsrc/node_file.ccto properly handle file paths that include non-ASCII characters. This change prevents crashes and errors when attempting to delete files with international or special characters in their names.Add a test in
test/parallel/test-fs-rmSync-special-char.jsto ensure that files with non-ASCII characters can be deleted without issues, covering cases that previously led to unexpected behavior or crashes on certain file systems.Fixes: #56049
For building the node and running the tests, I used: