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

Grab file name from file_info for accuracy #4429

Closed
wants to merge 1 commit into from

Conversation

jsheely
Copy link

@jsheely jsheely commented Dec 26, 2015

The handle->filew out of ReadDirectoryChangesW grabs the closest match to the file but does not contain the actual file name

fs.watch('*/.jsx');
ex. /path/to/file/filename.jsx will match /path/to/file/filename.js if you use handle->filew

Using file_info->FileName it will return an event for filename.jsx or one for filename.js but at least in this case you will be able to properly filter the value in node since you have the correct file name to match against.

There is probably more we can do in order to ensure that the event doesn't leave libuv if it doesn't match the watched file path but at least this way we can accurately do something about it.

Tje handle->filew out of ReadDirectoryChangesW grabs the closest match to the file but does not contain the actaul file name

fs.watch('**/*.jsx');
ex. /path/to/file/filename.jsx will match /path/to/file/filename.js if you use handle->filew

Using file_info->FileName it will return an event for filename.jsx or one for filename.js but at least in this case you will be able to properly filter the value in node since you have the correct file name to match against.

There is probably more we can do in order to ensure that the event doesn't leave libuv if it doesn't match the watched file path but at least this way we can accurately do something about it.
@Trott
Copy link
Member

Trott commented Dec 26, 2015

I believe the right place to submit this PR is https://github.com/libuv/libuv.

This is a change to libuv, a Node.js dependency, but not part of Node.js core itself.

@mscdex mscdex added the libuv Issues and PRs related to the libuv dependency or the uv binding. label Dec 26, 2015
@GenuineRex
Copy link

in libuv src/win/fs-event.c ~ line 376

        if (handle->dirw ||
          _wcsnicmp(handle->filew, file_info->FileName,
            file_info->FileNameLength / sizeof(WCHAR)) == 0 ||
          _wcsnicmp(handle->short_filew, file_info->FileName,
            file_info->FileNameLength / sizeof(WCHAR)) == 0) {

seems the culprit is this _wcsnicmp function that limits out to the length of the file changed. any reason this is not just a straight string compare instead?

@ChALkeR
Copy link
Member

ChALkeR commented Dec 26, 2015

/cc @saghul

@cjihrig
Copy link
Contributor

cjihrig commented Dec 29, 2015

Closing, as this should go in libuv first (as previously mentioned).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libuv Issues and PRs related to the libuv dependency or the uv binding.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants