Fix issue with globbing exclusions against relative filepaths

[sterpe]

So, was having an issue with grunt-contrib-watch not properly globbing the following exclusion:

//Gruntfile.js
        watch: {                                                               
            js: {                                                              
                files: [                                                       
                    '<%= yeoman.app %>/**/*.js',                               
                    '!**/node_modules/**'                                      
                ],                                                             
                tasks: ['dev']                                                 
            }                                                                  
        },                                                                     

        yeoman: {                                                              
            app: './app',                                                      
            build: './.build',                                                 
            dist: './dist'                                                     
        }             

Tracing the problem through grunt-contrib-watch -> gaze -> [email protected] reveals that neither [email protected] or @2.0.0 properly handles an exclusion glob when some relative filepath is used -- i.e., it is unable to understand that the following paths 'match':

app/js/foo.js //returned from minimatch('**/js/**') in globule#processPatterns
./app/js/foo.js //the filepath that should be matched and excluded. (in results [])

This patch fixes the issue by using path.resolve to normalize to absolute paths when running a comparison against the exclusion. Next steps would be to file bug with gaze to update it's dependency and then grunt-contrib-watch to update gaze.

Unit tests of issue added as well.

Thanks

[cowboy]

Why don't you just do this?

//Gruntfile.js
        watch: {                                                               
            js: {                                                              
                files: [                                                       
                    '<%= yeoman.app %>/**/*.js',                               
                    '!<%= yeoman.app %>/**/node_modules/**'                                      
                ],                                                             
                tasks: ['dev']                                                 
            }                                                                  
        },                                                                     

        yeoman: {                                                              
            app: './app',                                                      
            build: './.build',                                                 
            dist: './dist'                                                     
        }

Also, FWIW, because of the way exclusions (have to) work, requiring everything and then excluding node_modules is going to be VERY slow. Don't do this. Only include the files you need to!

[sterpe]

@cowboy It kind of feels like a principle / functional purity thing here:

./app/js/**/*.js
app/js/**/*.js 
/home/me/foo/app/js/**/*.js
./../foo/app/js/**/*.js

are equivalent filepaths and it seems like globule#processPatterns should return the same results array when processing the same globs against any equivalent path on the file system. Currently it doesn't since the paths it compares are not normalized.

I'm not sure what the argument is for not making this fix?

The issue of what the most efficient globbing sequence is feels like a separate issue. You are probably right but it seems like the correctness of results should in no ways depend on whether someone specified their inclusive glob as

app/js/**/*.js

vice

./app/js/**/*.js

I would ask that you please reconsider my pull request.

[sterpe]

Just as a final thought on this, FWIW, Ben. I think reasonable people would agree that when executed in the same cwd:

globule#processPatterns(['./app/js/**/*.js', '!**/node_modules/**'], ...)

globule#processPatterns(['app/js/**/*.js', '!**/node_modules/**'], ...)

should return the same results set. They do not.

Guaranteeing this only involves making a call out to path.resolve() to normalize the paths for comparison -- a trivial fix that doesn't even change the run-time of this function...if you are familiar with run-time evaluations and notations.

In any event it's probably a bit difficult to for me (or anyone for that matter, maybe even you) to justify using a utility library such as this in a codebase if the maintainers of that library are going to out of hand reject valid bugs fixes that people using it take the time to find and fix and share back to the community of other users.

Anyway, thanks for your time, Ben.

[cowboy]

The main issue is that even though these are all "equivalent" filepaths, globule is just doing very basic string matching using the minimatch lib, which helps keep performance as fast as possible. The instant you add the path module in, operations get significantly more expensive.

I filed an issue upstream to see if this can be fixed in minimatch.

[cowboy]

I'm going to leave this open until I know what's up with isaacs/minimatch#30. If he fixes the issue, I'll pull it in. If not, I'll re-close this issue.

[ghost]

@cowboy Did you found out whats up with isaacs/minimatch#30 ?