Repository navigation
Conversation
|
Hi @LuisDulo! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
I have signed the CLA with this GitHub account (LuisDulo). Thanks for taking a look. |
Metro review: Looks goodRisk: low · Commit: On Windows, readdir reports every reparse point as a link, including cloud placeholders. With this change the node crawler records an entry as a symlink only if lstat also says it is one. Entries that lstat calls regular files are kept only when their extension is listed, and entries it calls directories are crawled. The change is small and uses the lstat call the crawler already makes, so it adds no filesystem calls. It also brings the crawler in line with FallbackWatcher and NativeWatcher, which already classify entries by lstat. The new test fails on the base and passes on the head. Findings
Verified
Not run: No benchmark: the change adds no filesystem calls and only a few boolean checks per symlink entry in the crawler. No Windows run either. A hosted VM can't easily produce a cloud-filter reparse point that isn't a symlink (no OneDrive). The libuv behavior the PR relies on was checked against libuv's source instead. An early background run of the integration tests on head and base hit its time limit and was stopped. A later head run covered the same tests, but the integration tests were never run on base. Automated review by Claude. Advisory only: it never approves or blocks, and a maintainer makes the call. |
| // including cloud placeholders (OneDrive, Dropbox) that lstat | ||
| // reports as regular files or directories, and that readlink | ||
| // rejects with EINVAL. Trust lstat over the dirent. | ||
| const isLink = isSymbolicLink && stat.isSymbolicLink(); |
There was a problem hiding this comment.
nit · PR description has no Changelog line
The description has a test plan but no Changelog: line. Something like Changelog: [Fix] metro-file-map: Don't treat Windows cloud placeholder files (OneDrive, Dropbox) as symlinks when crawling would do.
Evidence: Read pr.md: it has no Changelog: line. · id 65618bea
Summary
On Windows,
fs.readdir(..., {withFileTypes: true})reports every reparse point as a symbolic link. That includes cloud placeholders such as OneDrive and Dropbox files, whichlstatcorrectly reports as regular files or directories. libuv's directory listing maps the reparse-point attribute toUV_DIRENT_LINK, whilelstatonly treats real symlinks and junctions as links.The node crawler in
metro-file-maptook the dirent's word for it. With symlinks enabled, it marked those files withH.SYMLINK.#maybeReadLinkthen calledreadlinkon them, which fails withEINVAL, and anything other thanENOENT/EACCESSthere is fatal:This is easy to hit when a project lives under OneDrive's Documents folder, the Windows default. In one Expo app's
node_modulesI found 57 files thatreaddircalled symlinks;lstatsaid none were, andreadlinkfailed on every one.The crawler already calls
lstaton every entry it records, so this change trusts that result:lstatagrees,lstatsays is a regular file is recorded as a file, but only if its extension is listed,lstatsays is a directory is crawled like any other directory.It adds no filesystem calls, so the existing tests' call counts and mtimes are unchanged.
Not changed here: with
includeSymlinksoff, entries thatreaddirmisreports as links are still skipped beforelstatruns, so such placeholders are silently left out of the map. Fixing that needs an extralstatfor each skipped link. I'm happy to follow up if you'd like that.Possibly related: #1146 ("None of these files exist" on Windows, fixed by re-saving files, which hydrates placeholders), though I have not confirmed it.
Test plan
New test in
crawlers/__tests__/node-test.js. It mocksreaddirentries that all claim to be links:.jsfile, expected to be recorded as a plain file.mdfile, expected to be ignored because.mdis not a listed extensionI wasn't able to run the full suite locally, so I'm relying on CI here.