fs: add Buffer support in fs methods

[jasnell]

Pull Request check-list

Please make sure to review and check all of these items:

• Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
  this change (including linting)?
• Is the commit message formatted according to [CONTRIBUTING.md][0]?
• If this change fixes a bug (or a performance problem), is a regression
  test (or a benchmark) included?
• Is a documentation update included (if this change modifies
  existing APIs, or introduces new ones)?

Affected core subsystem(s)

fs (/cc @trevnorris @bnoordhuis )

Description of change

Updated: Reworked the implementation based on @trevnorris feedback. This now does several things:

• Buffer accepted as Path on all fs methods that accept a Path
• {encoding: '...'} option accepted on fs.readdir, fs.readdirSync, fs.readlink, fs.readlinkSync, and fs.watch
• Documentation updates

Fixes: #2088
Ref: #3519
See Also: #3401

[jasnell]

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

[jasnell]

Sigh... CI is red all over. Suspected as much. I'll fix up the test case. Definitely not ready to land yet.

[silverwind]

I'd list the possible values, or link to some other place that has them.

[seishun]

Why are you singling out fs.watch? Either add this option to every fs function, or don't add it anywhere.

[bnoordhuis]

I agree with @seishun. I think it should work like this:

1. Functions that take path strings should also accept buffers. Examples: fs.open, fs.stat.
2. Functions that produce path strings should accept an encoding (and encoding='buffer' should produce buffers). Examples: fs.watch, fs.readdir.

Point 1 can be simulated using path = Buffer(path, encoding).toString('binary') so it's not strictly necessary.

[jasnell]

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

@seishun @bnoordhuis ... actually I've already starting looking at the expanded set. I definitely agree that we need to address them all but I'd like to separate it out across multiple PRs... or at least multiple commits within this PR.

[jasnell]

@bnoordhuis ... on point 1... if path is passed as a Buffer then it would always be buf.toString('binary') internally?

const buf = Buffer('/Users/jasnell/tmp/testing/新建文夹件.txt', 'utf8');
fs.openSync(buf, 'r');
// would attempt to open a file with the name: '/Users/jasnell/tmp/testing/æ°å»ºæ夹件.txt'

Or is the encoding passed to toString() going to be dependent on OS platform?

[jasnell]

@bnoordhuis @seishun ... ok, PTAL. Started moving this in the direction of rounding out the Buffer support for all of fs... still in a work in progress.

[seishun]

I'm still curious to know what actual real-world problem this solves that #3401 doesn't.

[jasnell]

@seishun ... as I understand it, #3401 assumes that the filenames are always utf8 which is not always the case. This approach, while a bit more complicated, provides quite a bit more flexibility and does not change the current default behavior the way #3401 would. Either way, I think I can live with either approach. I implemented this because the issue has been fairly long standing and an alternative fix hasn't yet landed.

[jasnell]

One other point... even if we land #3401, there is still value, I think, in allowing a dev to specify which encoding they want the filename to be in at the API level.

[seishun]

which is not always the case

Is there evidence that this has ever not been the case for any Node.js user?

and does not change the current default behavior the way #3401 would

The default would need to change anyway, since currently fs.watch is inexplicably inconsistent with other fs functions.

[jasnell]

Is there evidence that this has ever not been the case for any Node.js user?

Yes. See nodejs/node-v0.x-archive#2387 for example

[jasnell]

What would likely be an even better fix would be to have readdir and watch not perform any decoding on the filename at all and instead have it return the filename as Buffer instances. Then in the fs code we can toString() as utf8 by default but allow the encoding option to override.

[bjouhier]

It feels strange to expose filenames as buffers through the fs API.

Conceptually, filenames are strings and code which uses the fs API should only see strings. If filenames are incorrectly decoded by specific calls (fs.watch), the problem should be fixed at the fs level or below, it should not surface above.

Of course, this assumes that the fs layer can determine the encoding that the system is using. But if it cannot do it, why would the higher layers know better?

[jasnell]

@bjouhier ... because on certain systems it's impossible to know for sure because filenames don't inherently have an encoding. They're just byte sequences. On such systems, the filenames aren't simply strings.

[jasnell]

After talking it over with @trevnorris a bit, I'm going to back up and refactor this more. Will update when it's closer to being done!

[trevnorris]

@bjouhier libuv can't make assumptions about the multi-byte-ness of the read filename. hence why:

require('fs').watch('/tmp/ab\u0222', function(event, filename) {
  console.log(filename);
});

prints abȢ instead of abȢ. meaning you'll have to do Buffer(filename, 'binary').toString() to get it back as expected anyway.

[bjouhier]

@jasnell If filenames are just byte streams and the system does not impose their encoding then two applications that create files may assume different encodings. Then filenames created by app1 will look weird in app2, and vice versa. This is a mess and you need extra info (where?) to associate encodings with filenames.

In OSX the encoding is imposed by the system (UTF8 encoding of unicode in fully decomposed form - which has its own problems BTW), so there is no ambiguity. Same story for Windows/NTFS (UTF16 pre-composed this time). Apparently the mess is on UNIX/Linux (I did not know it, just found out - http://unix.stackexchange.com/questions/39175/understanding-unix-file-name-encoding). Given that, yes, node should expose a buffer alternative, but for all fs calls, not just fs.watch.

[seishun]

Yes. See nodejs/node-v0.x-archive#2387 for example

It seems that user's filesystem is UTF-8 overall but has individual filenames encoded in a different encoding. I don't see how this PR would help them, unless they are going to use Buffers in all of fs API. In that case, I don't see any point in the encoding option - it's sufficient to just have a "Buffer/decode-UTF8" switch.

because on certain systems it's impossible to know for sure because filenames don't inherently have an encoding. They're just byte sequences. On such systems, the filenames aren't simply strings.

This is could be true only on Linux. On OS X, this API is completely useless, because filenames are guaranteed to be encoded in UTF-8. On Windows, it's effectively going to lie, because filenames are already decoded as UTF-16 and re-encoded as UTF-8 by libuv.

Node.js shouldn't add APIs that only work on a single platform.

[trevnorris]

On Windows, it's effectively going to lie, because filenames are already decoded as UTF-16 and re-encoded as UTF-8 by libuv.

This is not true. libuv never makes assumptions about a filename. It simply returns the bit stream it was handed by the OS. For example:

fs.watch('path/\u0222-foo.txt', function(e, filename) {
  console.log(filename);
});

Output:

È¢-foo.txt

It's node that makes an assumption in that particular API.

[jasnell]

@trevnorris @bnoordhuis @seishun @nodejs/ctc ... Ok, just pushed a significantly reworked update on this based on conversations with @trevnorris around the implementation. PTAL. @seishun, the reworked version assumes a default encoding of UTF8 so in the most common case, if encoding is not specified, filenames come through as UTF8.

[jasnell]

Changing to semver-major since this includes changes to the error handling and default encoding for fs.watch results.

[jasnell]

Landed in 4d4f353, 060e5f0, and 53a95a5

[jasnell]

Thanks @trevnorris and @bnoordhuis !