Fix for removal of trailing slashes

[natewallace]

The calls to the path.resolve function are stripping trailing slashes from glob patterns passed into the create function. This is resulting in files coming back in search results when only folders should be for certain glob patterns.

For example if I had the following folders and files: /test/folder1/ /test/file1.js /test/file2.js

And I passed in the following glob pattern: /test/*/

I should only get back /test/folder1/ in the results, however because the trailing slash is removed and the pattern /test/* is applied I get back /test/folder1, /test/file1.js, and /test/file2.js in the results.

This pull request contains an update that restores the trailing slash when this scenario occurs so the expected results will be returned for glob patterns that target folders only.

[yocontra]

Is there an alternate path function (in core that is) that will keep the trailing slash?

[natewallace]

It looks like calls to path.resolve can be replaced with a combination of calls to path.normalize and path.isAbsolute. path.normalize will preserve the trailing slash and path.isAbsolute is used to determine when to ignore the cwd value as path.resolve would normally do when an absolute path is given.

However, path.isAbsolute is not defined in Node v0.10 so this functionality has to be duplicated in order to work on this version of Node.

My latest push contains these updates.

[phated]

The correct glob for directories is ** not a single star. I don't think this needs to be changed.

[natewallace]

I don't believe the double asterisk is used to search for folders but is rather used to specify that any and all sub folders from the given path should be searched through.

For example if I had the following folders and files: /test/folderParent/ /test/folderParent/folderChild/ /test/folderParent/folderChild/test.js

And I used the following glob pattern: /test/**

I will get the following results back: /test /test/folderParent /test/folderParent/folderChild /test/folderParent/folderChild/test.js

Based on the following documentation for the options.nodir parameter from the node-glob library:

Do not match directories, only files. (Note: to match only directories, simply put a / at the end of the pattern.)

a trailing slash is needed in order to search specifically for folders.

[phated]

@natewallace my mistake. I think path.resolve is made for files instead of folders. Let me ask about this with some of node core.

[phated]

@jonschlinkert could we solve this with https://github.com/jonschlinkert/resolve-glob? Also, I'd like to have a longer form discussion about your globbing libraries/utils that I found through this.

[jonschlinkert]

could we solve this with https://github.com/jonschlinkert/resolve-glob? Also, I'd like to have a longer form discussion about your globbing libraries/utils that I found through this.

possibly, I'll look over the issue when I get back from lunch.

I'd like to have a longer form discussion about your globbing libraries/utils that I found through this.

awesome, anytime. let me know what works best for you.

[jonschlinkert]

sorry long lunch lol. if the only thing we really need to know is if the last character is a slash, can we just get away with checking to see if the last character in the pattern is a slash, and if so, let path.resolve do it's thing then just restore the slash before passing it to glob? (sorry if I missed other nuances about the issue)

[jonschlinkert]

if it's needed to check for absolute paths, maybe take a look at https://www.npmjs.com/package/is-absolute. it's been pretty heavily vetted

[phated]

That can solve the issue but the only thing we use path.resolve for is to make globs absolute. If your library handled this case, maybe we should drop our custom code.

[phated]

@jonschlinkert we make all globs absolute, I think. This is probably due to node-glob handling relative paths weird

[jonschlinkert]

the resolve-glob lib actually calls node-glob and just ensures that the returned path is absolute - rather than relative to the given cwd - so it's not really useful here. but I had this same challenge with making globs absolute with matched, so thought it might make sense to externalize a function that makes globs absolute, using the logic in glob-stream as a starting point so it would be more unit testable and generalized.

I ran it against all unit tests in glob stream, and added unit tests for this specific issue. everything passes. I'd be happy to do a pr if you want - referencing @natewallace for bringing it to our attention and this issue of course.

[phated]

:+1: for a reusable module

[phated]

@natewallace thanks for bringing this to our attention and sending this PR with a fix. I deferred to the reusable module. This should be fixed now.

[phated]

Due to publishing problems, this has been released as 5.2.0. Sorry about that.