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

fix: normalize win32 paths to use on glob expressions #209

Merged
merged 1 commit into from
May 18, 2023

Conversation

supita
Copy link
Contributor

@supita supita commented May 18, 2023

What

While testing cacache.verify I found that on Windows the stats returned by it are not properly reported. Moreover, I found that cacache.verify fails to delete files if no cache entries refer to them.

cacache.rm.all also fails to work on Windows: the cache is not cleared after calling this function.

Why

The root of the problem is the same: globify function is not replacing to forward-slashes as glob library is expecting to handle Windows paths.

I found that originally globify was at lib/verify.js, the replacement of the forward-slashes was made correctly.

const globify = pattern => pattern.split('\\').join('/')

But when created the file lib/util/glob.js and moved globify there, it was changed to

const globify = pattern => pattern.split('//').join('/')

which is causing problems when glob is used for obtaining stats and cleanup in the case of verify and delete the cache with rm.all

Change details

Replace backslash by forward-slashes if they are present on a path before calling glob.

Initially I was going to rollback the change to the first implementation of globify, but I though it would be more explicit to use path.sep.

Issues reported

[BUG] rm.all doesn't delete anything on Windows #165

@supita supita requested a review from a team as a code owner May 18, 2023 02:00
@wraithgar
Copy link
Member

Yep good catch. I think this is a typo back from when glob had that semver major change to stop accepting windows paths.

In other repos we got it right, it looks like:
https://github.com/npm/map-workspaces/blob/cc0580d8e8486816eb8dea8efe01e3734605a996/lib/index.js#L57
https://github.com/npm/cli/blob/9202c7d7c4058deb618e1a74fdc97b11f2845af7/lib/workspaces/get-workspaces.js#L7

Initially I was going to rollback the change to the first implementation of globify, but I though it would be more explicit to use path.sep.

Yeah when we initially fixed this we did do it how globify did. Your choice is a good one though, and would have prevented this very bug.

@wraithgar wraithgar merged commit a0a5e58 into npm:main May 18, 2023
@github-actions github-actions bot mentioned this pull request May 18, 2023
@supita supita deleted the fix/normalize-win32-path branch May 18, 2023 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants