GeezerVison

A couple of suggestions:

• I don’t think the native binding to the libuv realpath needs to be removed. It can be, but it might be nice to retain it for comparing results with the JS implementation later.
• You could add the benchmark/test commit from @trevnorris’ fs: fix long symlinks in realpath #7548 here.
• I’ll try to benchmark it, but removing the cache altogether sounds like something that won’t work out. I realize that it conflicts with the encoding: option, but that seems to be easily worked around by never adding an encoding: key to the cache? It also won’t be a likely problem in practice, because only path.resolve()d keys are added to the cache. People in the CTC meeting made some good points and removing the cache sounds okay.
• The encoding option should probably be used for all fs.* calls that are made from inside the realpath implementations?

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

At the last meeting, the CTC has expressed consensus with the approach in this PR: reverting, leaving the cache out, but keeping the encoding option.

To me it looks like this reverts too many tests. Could we keep the ones that still pass?

@addaleax :

• I think that since we are not using native realpath anymore there is no need to keep bindings around.
• Adding benchmarks from fs: fix long symlinks in realpath #7548 is a good idea, I'll do that.
• Inside we are using path.resolve, which expects utf-8 strings. So we stick to them when calling other fs functions, and only apply encoding option when returning result.

@ChALkeR: Those test are reverted by #7846 on which this PR depends.

I think that since we are not using native realpath anymore there is no need to keep bindings around.

@bzoz Your call.There seems to be agreement that the long-term plan is to use a fixed libuv implementation, though, and I think keeping the binding around might be useful for e.g. comparative benchmarks.

we are using path.resolve, which expects utf-8 strings

path.resolve expects strings, which means that it it isn’t concerned with encodings at all. But you’re right, this is not as easy as I though it would be, and it’s probably best to just stick with UTF-8 for all intermediate results (and accepting that that means that symlinks encountered along the way must be valid UTF-8).

In general this LGTM with a green CI. I definitely want to make sure this gets additional eyes on it to review, however. @nodejs/ctc

@bzoz I've just compiled your PR on Windows and tried the node.exe inside a Windows Container. Installed Node.js 6.3.1 from MSI inside the container and just replaced node.exe.

Running npm init, npm install express ... works fine again. 🐳
So this PR would fix #7044

Why does this PR also revert "fs: validate args of truncate functions in js" and "fs: make callback mandatory to all async functions" ? How are those related to the realpath problems? It makes is a lot harder to review this PR.

Those seem to be handled by #7846, btw.

@saghul This PR would otherwise create merge conflicts, that’s all; the intent here is to wait until #7846 is landed, if I understood @bzoz correctly.

@bzoz Many, many thanks for starting to add regression tests! Overall LGTM, too.

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

LGTM, thanks for doing the work behind this PR!

CI: https://ci.nodejs.org/job/node-test-commit/4374/

One Windows build bot failure, running CI again to be safe: https://ci.nodejs.org/job/node-test-commit/4376/

I had one running here https://ci.nodejs.org/job/node-test-commit/4373/, it is green.

Oh, sorry (but good to hear)! I’d say this is good to go once #7846 is resolved.

@bzoz What do you think of squashing all commits but the ones from #7846 to make reviewing changes easier? Currently one has to go through all these commits separately in order to focus on reviewing the changes that are actually related to this PR, which is a bit tedious.

@misterdjules OK, squashed

Commits:

1. Restore old js realpath implementation
2. Implement new API (i.e. options.encoding) and add a test
3. Add tests and benchmarks from fs: fix long symlinks in realpath #7548
4. Remove one test from fs: fix long symlinks in realpath #7548 which was failing

What was the result of the discussion about the substed realpath behavior thing? This PR adds a test thus making certain behavior "correct" which might not necessarily be the case. It depends on how substed drives are interpreted, IMHO.

On the maze of issues and notes I recall reading that some people where using substed drives to get shorter paths. Was that the case? What breaks if we would give them the long path?

I've added a note to the fs documentation.

LGTM, thank you.

CI is green. Let's give it another day before landing. @nodejs/ctc @nodejs/collaborators ... please take one final look.

Landed in 08996fd and b9832eb

@bzoz @addaleax one thing I really liked from the behaviour of native fs.realpath is that it would return the actual casing of a path as it exists on disk. E.g. if there is an actual path /foo/bar/some.txt, running fs.realpath("/foo/BAR/some.txt") would return /foo/bar/some.txt. Are there any plans to support this behaviour after going back to the old JS implementation? Or any plans to provide this functionality in a separate method?

@bpasero I guess you can open a separate issue for this? It sounds like something that would be okay to have.

@addaleax makes sense, opened #8715