fs: report source and destination files in error messages

[caitp]

In some cases, only reporting the source or destination file makes sense.
There is a potential compat risk if modules are post-processing the error message in some way.

[piscisaureus]

Ok, that's a good start. But at a quick glance:

• This code doesn't work for sync calls.
• There should be no need to strcmp() to figure out what the syscall was.
• It leaves dead code behind.

[caitp]

How are you coming to the conclusion that it doesn't work for sync calls?

> require("fs").renameSync("biz", "baz")
Error: ENOENT, rename 'biz' -> 'baz'
at Error (native)
at Object.fs.renameSync (fs.js:636:18)
at repl:1:15
at REPLServer.defaultEval (repl.js:135:27)
at bound (domain.js:275:14)
at REPLServer.runBound [as eval] (domain.js:288:12)
at REPLServer.<anonymous> (repl.js:282:12)
at REPLServer.emit (events.js:116:17)
at REPLServer.Interface._onLine (readline.js:215:10)
at REPLServer.Interface._line (readline.js:554:8)

Unless you're referring to synchronous failures in ASYNC_DEST_CALL, in which case the error is handled by the preceding block in After() and the new stuff doesn't matter

If there is dead code left behind (I assume you mean the branch just underneath which tests for conditions where errors should be reported re: the destination path), a smart compiler will remove them --- however, I don't expect that this is dead code, just yet, and that branch should probably take precedence over the new one.

There should be no need to strcmp() to figure out what the syscall was.

There are lots of approaches. Adding a flag to the macros and ReqWrap objects indicating the call type (big change), using a hash (not worth it for the small number of cases that matter), just testing against an enumerated number with something like func ## _command in the macros (and also passing this to the req wrapper). I'm not sure extending ReqWrap objects is worth it for this, but you're welcome to make a case regarding this, explaining clearly what the issue with strcmp() is.

[vkurchatkin]

There should be no need to strcmp() to figure out what the syscall was.

I think checking req_wrap->dest_len() > 0 is enough

[caitp]

I think checking req_wrap->dest_len() > 0 is enough

Okay, well doing that will definitely create some dead code, and I'm of the opinion that the code which is removed is generally more useful (because, a caller is able to deduce the specific file which is problematic more easily).

I'll change it to do that I guess

[caitp]

I guess the UV request has a way to figure out request commands, but if people are happy with dest_len > 0 then it's all good

[piscisaureus]

How are you coming to the conclusion that it doesn't work for sync calls?

You're right, I was wrong. It does work for sync calls.

If there is dead code left behind (I assume you mean the branch just underneath which tests for conditions where errors should be reported re: the destination path),

Indeed. That branch makes the error include the "dest" path instead of the "source" path in certain cases. Not in a way that makes any sense. That's the part I'd like you to improve.

I guess the UV request has a way to figure out request commands, but if people are happy with dest_len

There's a type field can can be UV_FS_STAT, UV_FS_RENAME etc.

[caitp]

@piscisaureus yeah --- I changed it to the simpler approach, which removes the dead code.

Personal opinion, the now-dead code was useful. It makes sense to report the file which was the cause of the signal. The extra information is good too, but maybe it would make sense to put it in parentheses after the single-file error message, so you get both more helpful information and less compat-risk.

If that sounds good, I could do a followup or just change it in the PR when I have some time

[piscisaureus]

Personal opinion, the now-dead code was useful.

I disagree. The now-dead code was misleading; if you get EPERM in a rename operation that can mean many things, some of them related to the old filename (not allowed to rename), others may be related to the new filename (the new name already exists and you're not allowed to overwrite the file).

For symlinks, it almost never makes any sense to put the symlink contents (../foo/bar) in the error message since the contents are not validated, thus can't generate an error. The only exception is windows when you're creating a junction.

That's why I suggested to put both paths in the filename for rename() and link(), and include the symlink contents only on windows if it's a junction.

[caitp]

There are definitely cases where you do want the single-file error though, depending on the command and the signal, and it's nice to be specific about this.

Whatever you like though

[piscisaureus]

Thanks

[caitp]

so what's going on here? this was as-asked-for 9 days ago =)

[Qard]

LGTM

@piscisaureus Can you take another look? Both paths are there, so your concerns should be dealt with, afaict.

[piscisaureus]

@bnoordhuis@indutny WDYT, I'm unsure about the use of std::string.

[bnoordhuis]

@piscisaureus I don't mind. I've discussed with @indutny before that I'd be alright with moving more of our homegrown code over to STL.

Frivolous string appending is something I would avoid in general because of the potential O(n) number of allocations but here it's probably alright. A simple snprintf() works too, of course.

[piscisaureus]

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/132/

[piscisaureus]

It's nothing personal, but I'm not going to land this. A lot more needed to be cleaned up (notice how UVException just accidentally works because node_file.cc passes the syscall in the msg parameter position, and passes nullptr as the syscall).

#675