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.

Sign up for GitHub

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

Jump to bottom

win: use long directory name for handle->dirw #1769

Closed
wants to merge 2 commits into from
Closed

win: use long directory name for handle->dirw #1769

wants to merge 2 commits into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Mar 8, 2018 •
edited

uv_relative_path assumes dir is a prefix of filename, which is not
the case when handle->dirw is a short path.

Fixes: nodejs/node#19170

@bzoz
Copy link
Member

bzoz commented Mar 8, 2018

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/748/

@seishun
Copy link
Contributor Author

seishun commented Mar 8, 2018

I see, it doesn't work because watching is recursive. We'll have to go with the other solution.

@seishun
win: use long directory name for handle->dirw
d750995 
`uv_relative_path` assumes `dir` is a prefix of `filename`, which is not
the case when `handle->dirw` is a short path.

Fixes: nodejs/node#19170
@seishun
Copy link
Contributor Author

seishun commented Mar 8, 2018

Let's try again: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/750/

@seishun seishun changed the title win: use uv_split_path to get filename from fs event win: use long directory name for handle->dirw Mar 8, 2018
bzoz
bzoz approved these changes Mar 12, 2018
View reviewed changes
Copy link
Member

@bzoz bzoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is green, change LGTM

@santigimeno
Copy link
Member

Would it be possible adding a test for this?

@seishun
Copy link
Contributor Author

seishun commented Mar 13, 2018

nodejs/node#19170 is reproducible only with user names longer than 8 characters, not sure how one could test such scenario.

@santigimeno
Copy link
Member

I don't know either 🤷‍♂️ . @bzoz do you think it can be possible?

@bzoz
Copy link
Member

bzoz commented Mar 13, 2018

Sure: use https://github.com/libuv/libuv/blob/v1.x/test/test-fs-event.c, add one more test case with watch_dir/very_long_dirname_that_is_very_long/, and use result of GetShortPathName to watch this directory.

The username is just a coincident, the original issue reproduces with any long directory name watched through its short name. Besides os.tmpdir() there is very low possibility that a Node program would encounter 8.3 short filename, so this is why we found the issue there.

@seishun
Copy link
Contributor Author

seishun commented Mar 13, 2018

Added a test.

@bzoz
Copy link
Member

bzoz commented Mar 13, 2018

ci: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/751/

@bzoz
Copy link
Member

bzoz commented Mar 19, 2018

The failures seem unrelated.

bzoz pushed a commit that referenced this pull request Mar 19, 2018
@seishun @bzoz
win: use long directory name for handle->dirw
7e865b6 
`uv_relative_path` assumes `dir` is a prefix of `filename`, which is not
the case when `handle->dirw` is a short path.

Refs: nodejs/node#19170
PR-URL: #1769
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
@bzoz
Copy link
Member

bzoz commented Mar 19, 2018

Landed in 7e865b6

@bzoz bzoz closed this Mar 19, 2018
@seishun seishun deleted the tmp-watch branch March 19, 2018 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bzoz bzoz bzoz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fs.watch - on change returns incorrect file name
3 participants
@seishun @bzoz @santigimeno