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.

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

remove*() and empty*(): remove contents as Buffers to handle non-UTF-8 filenames on Linux #612

Closed
wants to merge 1 commit into from

Conversation

rossj
Copy link

@rossj rossj commented Aug 5, 2018

This is the first part of fixing issue #575 that deals with non-UTF-8 filenames. I've modified remove(), removeSync(), emptyDir() and emptyDirSync() to use {encoding: 'buffer'} when calling fs.readdir*, in order to more reliably remove contents that may not be convertible to UTF-8. This situation may only be possible on Linux file systems, but the changes and additional unit tests are working on Windows NTFS and Apple APFS.

Further work is required to update copy*() (and therefore move*()) to close #575, but it was becoming a bit of a rabbit hole, so I wanted to get this part squared away first.

Additionally, as a side effect of these changes, it is also possible to pass a Buffer input path to remove() and removeSync() to remove a directory directly that may have an invalid UTF-8 name. This is not yet possible with the emptyDir* functions because that first requires Buffer support in mkdirs.

Please advise if you would be willing to review + merge this PR, or if you would like to wait for the additional changes mentioned above.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 86.313% when pulling 2ddf1d5 on rossj:master into fb3dda7 on jprichardson:master.

@RyanZim
Copy link
Collaborator

RyanZim commented Aug 11, 2018

@manidlou how do you feel about blocking the v8 release until @rossj can get #575 completely fixed?

Code here looks good BTW.

@manidlou
Copy link
Collaborator

@RyanZim yep I am totally fine with that!

@rossj can you please give us a rough estimate of how much work you need to do for this to be complete like when do you think you can have it ready?

Code LGTM too!

@manidlou
Copy link
Collaborator

manidlou commented Sep 9, 2018

@rossj the work on copy*() and move*() is done on branch v8-dev. We'll be waiting for your work to be finished before releasing v8.

@rossj
Copy link
Author

rossj commented Sep 14, 2018

@manidlou thank you, I should have time to work on this over the weekend

Edit: Working on it this week!

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 7, 2018

@rossj ping?

@rossj
Copy link
Author

rossj commented Nov 7, 2018

@RyanZim sorry for the silence... I kind of went down a rabbit hole and got suck in some upstream issues and scope creap.

For example, #575 is about expanding Buffer path input to all fs-extra methods, including mkdirs. The problem is that mkdirs uses path.resolve and path.normalize and path.dirname, but the path module has no equivalents for dealing with Buffers, which lead to nodejs/node#23722, and the conclusion that a Buffer-based path module would be a good user-land module. I have slowly been working on one, but it will take some more time.

Furthermore, I discovered nodejs/node#23735 which prevents fs-extra from being able to copy / delete / do anything to certain files on Windows. The main purpose of this PR is to make remove & empty work with any files on any system, regardless of encoding, but that isn't actually possible at this point (on Windows).

My suggestion is to either leave this Buffer stuff behind and continue to v8 without it, or take the Buffer improvements as they are now with updates only to remove and empty, which do actually improve / fix some issues on Linux/Unix systems, as fully addressing #575 requires some more free time to implement the buffer-path module.

@RyanZim
Copy link
Collaborator

RyanZim commented Nov 7, 2018

I'm leaning towards going ahead with v8 without these changes; @manidlou?

@manidlou
Copy link
Collaborator

manidlou commented Nov 8, 2018

@RyanZim yeah I am fine with releasing v8 without these changes.

@RyanZim
Copy link
Collaborator

RyanZim commented May 22, 2019

@rossj How's it going?

@RyanZim
Copy link
Collaborator

RyanZim commented Sep 21, 2021

Closing this PR as abandoned.

@RyanZim RyanZim closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems when dealing with invalidly-encoded filenames
4 participants