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

Glob sync: trailing slash fix #159

Closed
wants to merge 3 commits into from
Closed

Glob sync: trailing slash fix #159

wants to merge 3 commits into from

Conversation

meyer
Copy link

@meyer meyer commented Feb 6, 2015

This PR fixes #158.

GlobSync.prototype._processSimple

  • Line 331 Renamed exists to prefixStat so that it’d make sense when I’d refer to it later.
  • Line 367: abs was being built with path.resolve, but path.resolve trims the trailing slash off files and folder paths, which means any trailing slash check on abs will always be negative. I switched it to path.join instead.
  • Lines 364-367: The absolute path wasn’t being “absoluteified” in a few cases, so I fixed that.

GlobSync.prototype._stat

  • Line 386: fs.statSync comes undone if a file path ends in a slash but isn’t a directory, so I’m running path.resolve on it before fs.statSync.
  • Line 395-397: _stat was returning false if abs ended with a slash but wasn’t a directory, but hey, that’s not its job, that’s _processSimple’s job, so I moved it up there. Also, since the trailing slash was being resolved away that particular blob of code would never ever run. Poor lil guy.

Hope this all makes sense 😰

Ran tests with npm test, everything looks good.

meyer added 3 commits February 5, 2015 16:16
Fix:
* `_stat` should always return `DIR` or `FILE` on paths that exist.

Opinion:
* `_processSimple` should be the function that deals with non-DIR
trailing slashes in file paths.
@isaacs
Copy link
Owner

isaacs commented Feb 7, 2015

Thanks for getting this started!

Before this can be accepted, though, there are two things that need to be done.

First, it needs a test added (something that fails before the patch, and passes with the patch). You can either modify an existing test to add the new case, or add a new one. (Note that if you do add a new test, you'll have to run npm run test-regen on a Mac or Linux box to re-generate the bash comparison test cases.)

Second, I'm pretty sure that all of these glob.sync errors are also errors in the async case. If you look through the code, you'll see that they follow one another pretty closely. So, each of these changes in the sync flow have to have the corresponding change made in the async flow.

Let me know if you are/aren't interested in doing these bits, or if you need any help with them. Eventually, I'm sure I or someone else will get to it if you don't find the time, but it might take a bit longer to get to.

@isaacs
Copy link
Owner

isaacs commented Mar 6, 2015

Fixed by 0b729c8. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trailing slash in pattern is matching files as well
2 participants