fs: fix rmSync to handle non-ASCII characters #56934
Conversation
nodejs-github-bot
added
c++
|
@joyeecheung, thanks for your suggestion. I resolved the conflict with the latest main branch. But, due to the changes in the helper, in |
github-actions
bot
removed
the
request-ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56934 +/- ##
==========================================
+ Coverage 89.15% 89.17% +0.01%
==========================================
Files 665 665
Lines 192798 192800 +2
Branches 37130 37121 -9
==========================================
+ Hits 171886 171921 +35
+ Misses 13673 13670 -3
+ Partials 7239 7209 -30
|
|
@joyeecheung It looks like all checks passed except the first git commit message one, where I must explicitly put the GitHub link. Should I just amend the first commit message and push it here? |
|
Yes, you'll need to update it to pull a full GitHub link |
Yeaseen
force-pushed
the
fix-non-ascii-filepath-issue
branch
from
81955c8 to
100424e
Compare
|
@joyeecheung updated. |
|
Hmm, I think you'll need to rebase against the latest main branch and drop the other commits that you've pulled in somehow from the main branch. You can use use something like |
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 two tests 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
100424e to
50accca
Compare
|
@joyeecheung Thanks for the suggestion! It worked. Learned a lof things! |
| return env->ThrowErrnoException(ENOTDIR, "rm", message.c_str(), path_c_str); | ||
| } else if (error == std::errc::permission_denied) { | ||
| std::string message = "Permission denied: " + file_path_str; | ||
| std::string message = "Permission denied: "; |
There was a problem hiding this comment.
Why did you remove the paths from the error messages?
| @@ -41,8 +56,7 @@ Local<Value> ErrnoException(Isolate* isolate, | |||
|
|
|||
| Local<String> path_string; | |||
| if (path != nullptr) { | |||
| // FIXME(bnoordhuis) It's questionable to interpret the file path as UTF-8. | |||
| path_string = String::NewFromUtf8(isolate, path).ToLocalChecked(); | |||
| path_string = StringFromPath(isolate, path); | |||
There was a problem hiding this comment.
This FIXME should not be removed, isn't it?
This is a fresh version of PR #56117
Update
fs.rmSyncto 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 two tests 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: #56049
@lemire I had to close the previous PR due to some conflicts. Please review this. Thank you!