Skip to content

Suggested fix for #97, review of implications required #107

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.

Sign up for GitHub

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

Open
nilsvu wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Suggested fix for #97, review of implications required #107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c682113
Suggested fix for #97, review of implications required
nilsvu Jan 17, 2018
6d08219
attempt to implement test
nilsvu Jan 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion lib/glob.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,14 @@ function glob( globs, options ) {
var negatives = [];
globs.forEach( function ( glob ) {

var mm = new Minimatch( self.join( '/', glob ), options );
var negate = new Minimatch( glob, options ).negate;
glob = glob.replace(/^[!]+/g, '');
glob = self.join( '/', glob );
if (negate) {
glob = self.join( '!', glob );
}

var mm = new Minimatch( glob, options );

@nilsvu nilsvu Jan 28, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should properly handle leading negation characters ! while keeping the behaviour of prepending a slash. However, I don't quite see why prepending a slash is necessary or even useful - it may even lead to problems where the user doesn't expect this behaviour.
I'd also suggest moving to the glob parsing of vinyl-fs, as proposed above, but I don't have enough knowledge of the code to implement this.

mm.base = mmBase( mm );

if ( mm.negate ) negatives.push( mm );
Expand Down
30 changes: 22 additions & 8 deletions test/clean.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,17 @@ describe( 'clean', function () {

it( '(preparing: create files to clean up later)', function ( done ) {

VinylFs.src( 'test/fixtures/**' )
.pipe( VinylFs.dest( 'test/fixtures/cleaning' ) )
VinylFs.src( 'test/fixtures/index.html' )
.pipe( VinylFs.dest( 'test/fixtures/cleanthis' ) )
.on( 'error', done )
.on( 'end', done );

} );

it( '(preparing: create files to ignore in cleanup)', function ( done ) {

VinylFs.src( 'test/fixtures/index.html' )
.pipe( VinylFs.dest( 'test/fixtures/dontcleanthis' ) )
.on( 'error', done )
.on( 'end', done );

Expand All @@ -31,22 +40,27 @@ describe( 'clean', function () {

it( '(preparing: remove files to clean)', function ( done ) {

rmdir( 'test/fixtures/cleaning', done );
rmdir( 'test/fixtures/cleanthis', done );

} );

it( '(preparing: remove files to ignore in cleanup)', function ( done ) {

rmdir( 'test/fixtures/dontcleanthis', done );

} );

it( 'should clean up extra remote files', function ( done ) {
it( 'should clean up extra remote files but repect ignore rules', function ( done ) {

done = suite.done( done );

suite.vftp.clean( 'test/clean/**', 'test/fixtures' )
suite.vftp.clean( [ 'test/clean/**/*', '!test/clean/dontcleanthis/**/*' ], 'test/fixtures' )

@nilsvu nilsvu Jan 28, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This operation doesn't quite work yet, and the problem may lie with the implementation of the clean function. Skimming its implementation, it appears a directory can be deleted even though one or more of its containing files is excluded from the clean operation. I don't have enough insight into the code to fix this quickly, so could use some help here.

.on( 'error', done )

@nilsvu nilsvu Jan 28, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This practice of calling done on error, as adopted in the other tests as well, appears problematic to me. For example the operation may fail to delete a file, because its containing directory has already been deleted. This throws an error which is handled here by calling done, essentially marking the test as successful and bypassing the checks below. Try adding a check assert.ok(false) below - it will still succeed if an error occurs.

.on( 'end', check );

function check() {

assert.ok( suite.log.match( /DEL/ ) );
assert.ok( suite.log.match( /RMDIR/ ) );
assert.ok( suite.log.match( /RMDIR(.)*test\/clean\/cleanthis/ ) );
assert.equal( suite.log.match( /RMDIR(.)*test\/clean\/dontcleanthis/ ), null );
done();

}
Expand Down