windows: ensure fs_event always provides filename

[nitoyon]

uv_fs_event didn't provide filename argument on Windows when a file is deleted or renamed, although it does on Linux.

Fix this by ensuring that fs_event always provides filename argument on Windows.

---

Node.js API document says:

Providing filename argument in the callback is not supported on every platform (currently it's only supported on Linux and Windows). Even on supported platforms filename is not always guaranteed to be provided.

http://nodejs.org/docs/latest/api/fs.html#fs_filename_argument

But, it's better that we provide filename argument if we can, isn't it?

How to reproduce this bug

Use following script:

var fs = require('fs');

var monitor = fs.watch('./', function(changes, fn) {
  console.log("%s :  %s", changes, fn);
});

fs.writeFileSync('foo.txt', 'new contents');
fs.renameSync('foo.txt', 'bar.txt');
fs.unlinkSync('bar.txt');

setTimeout(function() {
  monitor.close();
}, 1000);

Linux

On CentOS 6.4 x64 (Node.js v0.10.29)

$ node test.js
rename :  foo.txt  # writeFile
change :  foo.txt  # writeFile
rename :  foo.txt  # rename (src)
rename :  bar.txt  # rename (dst)
rename :  bar.txt  # unlink

On Windows 8.1 x64 (Node.j v0.10.21)

> node test.js
rename :  foo.txt  # writeFile
change :  foo.txt  # writeFile
rename :  null     # rename (src) <--- FIX THIS
rename :  bar.txt  # rename (dst)
rename :  null     # unlink       <--- FIX THIS

[saghul]

@nitoyon is this related to any issue open in Node? I remember something similar to this being discussed there.

[nitoyon]

@saghul this PR fixes nodejs/node-v0.x-archive#8372 and #1479.

[saghul]

@nitoyon thanks for the heads up. Could you please add a test, or modify an existing one, which validates the fix?

[piscisaureus]

It's not that I necessarily disagree.
But this feels like a behavioral change dressed up as a bug fix.
Also if we are going to give the user short filenames, we should at least be consistent about it and also do it for the create and rename-newname events.

[saghul]

Thanks for chiming in @piscisaureus :-) On top of that, we also need to be consistent with what we do in other platforms (which I don't remember from the top of my head).

[nitoyon]

@saghul It looks hard to add a test because there is no test which assert that filename should be given (e.g. ASSERT(filename == NULL || strcmp(filename, "file1") == 0); in test-fs-event.c L117).

And the document for fs.watch() in Node.js says:

Providing filename argument in the callback is not supported on every platform (currently it's only supported on Linux and Windows). Even on supported platforms filename is not always guaranteed to be provided.

It would break unit test if we add a test which asserts filename should be given.

@piscisaureus We have already provided short filenames if Action isn't REMOVE or RENAMED_OLD_NAME, and GetLongPathNameW fails. For details, please read my table.

[jorangreef]

fs.watch is different to most Node functionality in that it does not promise to be the same cross-platform.

But this promise is still very helpful. It makes it much easier to watch for filesystem events without resorting to any native dependencies. And for anyone using fs.watch, they immediately need to start out by considering cross-platform differences anyway. They are probably going to already be aware of things like 8.3 short names and unicode and casing differences.

So it would be helpful to me if fs.watch gave as much information as possible according to each platform. In this specific case, even the 8.3 short name would be very useful. And if the callback could always provide the 8.3 short name as extra info on Windows for the other events that would be even better.