From b76a5691c9592da8e3608157a2dcdcd3fc53860c Mon Sep 17 00:00:00 2001 From: Ari Porad Date: Sat, 20 Feb 2016 16:37:28 -0800 Subject: [PATCH 1/2] refactor(ls): greatly simplify ls implimentation --- shell.js | 2 +- src/find.js | 10 ++--- src/ls.js | 124 +++++++++++++--------------------------------------- test/ls.js | 3 +- 4 files changed, 37 insertions(+), 102 deletions(-) diff --git a/shell.js b/shell.js index 51ff397..208da25 100644 --- a/shell.js +++ b/shell.js @@ -23,7 +23,7 @@ exports.pwd = common.wrap('pwd', _pwd); //@include ./src/ls var _ls = require('./src/ls'); -exports.ls = common.wrap('ls', _ls, {idx: 1}); +exports.ls = common.wrap('ls', _ls, {noGlob: true}); //@include ./src/find var _find = require('./src/find'); diff --git a/src/find.js b/src/find.js index c96fb2f..4465e52 100644 --- a/src/find.js +++ b/src/find.js @@ -1,4 +1,5 @@ var fs = require('fs'); +var path = require('path'); var common = require('./common'); var _ls = require('./ls'); @@ -20,16 +21,15 @@ var _ls = require('./ls'); function _find(options, paths) { if (!paths) common.error('no path specified'); - else if (typeof paths === 'object') - paths = paths; // assume array else if (typeof paths === 'string') paths = [].slice.call(arguments, 1); var list = []; function pushFile(file) { - if (common.platform === 'win') + if (common.platform === 'win') { file = file.replace(/\\/g, '/'); + } list.push(file); } @@ -40,8 +40,8 @@ function _find(options, paths) { pushFile(file); if (fs.statSync(file).isDirectory()) { - _ls('-RA', file+'/*').forEach(function(subfile) { - pushFile(subfile); + _ls('-RA', file).forEach(function(subfile) { + pushFile(path.join(file, subfile)); }); } }); diff --git a/src/ls.js b/src/ls.js index 71953b4..2b6203e 100644 --- a/src/ls.js +++ b/src/ls.js @@ -1,8 +1,10 @@ var path = require('path'); var fs = require('fs'); var common = require('./common'); -var _cd = require('./cd'); -var _pwd = require('./pwd'); +var glob = require('glob'); + +var globPatternAll = path.sep + '*'; +var globPatternRecrusive = path.sep + '**' + globPatternAll; //@ //@ ### ls([options,] [path, ...]) @@ -46,119 +48,51 @@ function _ls(options, paths) { if (!paths) paths = ['.']; - else if (typeof paths === 'object') - paths = paths; // assume array else if (typeof paths === 'string') paths = [].slice.call(arguments, 1); + paths = common.expand(paths, { dot: options.all }); + var list = []; - // Conditionally pushes file to list - returns true if pushed, false otherwise - // (e.g. prevents hidden files to be included unless explicitly told so) - function pushFile(file, query) { - var name = file.name || file; - // hidden file? - if (path.basename(name)[0] === '.') { - // not explicitly asking for hidden files? - if (!options.all && !(path.basename(query)[0] === '.' && path.basename(query).length > 1)) - return false; - } - - if (common.platform === 'win') - name = name.replace(/\\/g, '/'); - - if (file.name) { - file.name = name; + function pushFile(file, rel, stat) { + stat = stat || fs.lstatSync(file); + if (process.platform === 'win32') file = file.replace(/\\/, '/'); + if (options.long) { + list.push(ls_stat(file, stat)); } else { - file = name; + list.push(path.relative(rel || '.', file)); } - list.push(file); - return true; } paths.forEach(function(p) { - if (fs.existsSync(p)) { - var stats = ls_stat(p); - // Simple file? - if (stats.isFile()) { - if (options.long) { - pushFile(stats, p); - } else { - pushFile(p, p); - } - return; // continue - } + var stat; - // Simple dir? - if (options.directory) { - pushFile(p, p); - return; - } else if (stats.isDirectory()) { - // Iterate over p contents - fs.readdirSync(p).forEach(function(file) { - var orig_file = file; - if (options.long) - file = ls_stat(path.join(p, file)); - if (!pushFile(file, p)) - return; + try { + stat = fs.lstatSync(p); + } catch (e) { + common.error('no such file or directory: ' + p, true); + } - // Recursive? - if (options.recursive) { - var oldDir = _pwd().toString(); - _cd('', p); - if (fs.statSync(orig_file).isDirectory()) - list = list.concat(_ls('-R'+(options.all?'A':''), orig_file+'/*')); - _cd('', oldDir); - } + // If the stat failed. + if (stat) { + if (!options.directory && stat.isDirectory()) { + var pathWithGlob = p + (options.recursive ? globPatternRecrusive : globPatternAll); + + glob.sync(pathWithGlob, { dot: options.all }).forEach(function (item) { + pushFile(item, p); }); - return; // continue + } else { + pushFile(p, null, stat); } } - - // p does not exist - possible wildcard present - - var basename = path.basename(p); - var dirname = path.dirname(p); - // Wildcard present on an existing dir? (e.g. '/tmp/*.js') - if (basename.search(/\*/) > -1 && fs.existsSync(dirname) && fs.statSync(dirname).isDirectory) { - // Escape special regular expression chars - var regexp = basename.replace(/(\^|\$|\(|\)|<|>|\[|\]|\{|\}|\.|\+|\?)/g, '\\$1'); - // Translates wildcard into regex - regexp = '^' + regexp.replace(/\*/g, '.*') + '$'; - // Iterate over directory contents - fs.readdirSync(dirname).forEach(function(file) { - if (file.match(new RegExp(regexp))) { - var file_path = path.join(dirname, file); - file_path = options.long ? ls_stat(file_path) : file_path; - if (file_path.name) - file_path.name = path.normalize(file_path.name); - else - file_path = path.normalize(file_path); - if (!pushFile(file_path, basename)) - return; - - // Recursive? - if (options.recursive) { - var pp = dirname + '/' + file; - if (fs.lstatSync(pp).isDirectory()) - list = list.concat(_ls('-R'+(options.all?'A':''), pp+'/*')); - } // recursive - } // if file matches - }); // forEach - return; - } - - common.error('no such file or directory: ' + p, true); }); // Add methods, to make this more compatible with ShellStrings return new common.ShellString(list, common.state.error); } -module.exports = _ls; - -function ls_stat(path) { - var stats = fs.statSync(path); +function ls_stat(path, stats) { // Note: this object will contain more information than .toString() returns stats.name = path; stats.toString = function() { @@ -167,3 +101,5 @@ function ls_stat(path) { }; return stats; } + +module.exports = _ls; diff --git a/test/ls.js b/test/ls.js index 7f3e580..87ed399 100644 --- a/test/ls.js +++ b/test/ls.js @@ -144,7 +144,7 @@ assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); // wildcard, should not do partial matches var result = shell.ls('resources/ls/*.j'); // shouldn't get .js -assert.equal(shell.error(), null); +assert.ok(shell.error()); assert.equal(result.length, 0); // wildcard, all files with extension @@ -249,7 +249,6 @@ assert.equal(result.length, 6); // long option, single file var result = shell.ls('-l', 'resources/ls/file1')[0]; assert.equal(shell.error(), null); -assert.equal(result.name, 'resources/ls/file1'); assert.equal(result.nlink, 1); assert.equal(result.size, 5); assert.ok(result.mode); // check that these keys exist From 003a800dda1e8834f34dcd081bea0cebbd72ae98 Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Mon, 29 Feb 2016 00:40:16 -0800 Subject: [PATCH 2/2] Perf improvement for ls, and fix for Windows --- shell.js | 2 +- src/ls.js | 49 +++++++++++++++---------- test/ls.js | 102 +++++++++++++++++++++++++++++++---------------------- 3 files changed, 90 insertions(+), 63 deletions(-) diff --git a/shell.js b/shell.js index 208da25..51ff397 100644 --- a/shell.js +++ b/shell.js @@ -23,7 +23,7 @@ exports.pwd = common.wrap('pwd', _pwd); //@include ./src/ls var _ls = require('./src/ls'); -exports.ls = common.wrap('ls', _ls, {noGlob: true}); +exports.ls = common.wrap('ls', _ls, {idx: 1}); //@include ./src/find var _find = require('./src/find'); diff --git a/src/ls.js b/src/ls.js index 2b6203e..8088df7 100644 --- a/src/ls.js +++ b/src/ls.js @@ -3,8 +3,7 @@ var fs = require('fs'); var common = require('./common'); var glob = require('glob'); -var globPatternAll = path.sep + '*'; -var globPatternRecrusive = path.sep + '**' + globPatternAll; +var globPatternRecursive = path.sep + '**' + path.sep + '*'; //@ //@ ### ls([options,] [path, ...]) @@ -48,20 +47,20 @@ function _ls(options, paths) { if (!paths) paths = ['.']; - else if (typeof paths === 'string') + else paths = [].slice.call(arguments, 1); - paths = common.expand(paths, { dot: options.all }); - var list = []; - function pushFile(file, rel, stat) { - stat = stat || fs.lstatSync(file); - if (process.platform === 'win32') file = file.replace(/\\/, '/'); + function pushFile(abs, relName, stat) { + if (process.platform === 'win32') + relName = relName.replace(/\\/g, '/'); if (options.long) { - list.push(ls_stat(file, stat)); + stat = stat || fs.lstatSync(abs); + list.push(addLsAttributes(relName, stat)); } else { - list.push(path.relative(rel || '.', file)); + // list.push(path.relative(rel || '.', file)); + list.push(relName); } } @@ -72,19 +71,31 @@ function _ls(options, paths) { stat = fs.lstatSync(p); } catch (e) { common.error('no such file or directory: ' + p, true); + return; } - // If the stat failed. - if (stat) { - if (!options.directory && stat.isDirectory()) { - var pathWithGlob = p + (options.recursive ? globPatternRecrusive : globPatternAll); - - glob.sync(pathWithGlob, { dot: options.all }).forEach(function (item) { - pushFile(item, p); + // If the stat succeeded + if (stat.isDirectory() && !options.directory) { + if (options.recursive) { + // use glob, because it's simple + glob.sync(p + globPatternRecursive, { dot: options.all }) + .forEach(function (item) { + pushFile(item, path.relative(p, item)); + }); + } else if (options.all) { + // use fs.readdirSync, because it's fast + fs.readdirSync(p).forEach(function (item) { + pushFile(path.join(p, item), item); }); } else { - pushFile(p, null, stat); + // use fs.readdirSync and then filter out secret files + fs.readdirSync(p).forEach(function (item) { + if (item[0] !== '.') + pushFile(path.join(p, item), item); + }); } + } else { + pushFile(p, p, stat); } }); @@ -92,7 +103,7 @@ function _ls(options, paths) { return new common.ShellString(list, common.state.error); } -function ls_stat(path, stats) { +function addLsAttributes(path, stats) { // Note: this object will contain more information than .toString() returns stats.name = path; stats.toString = function() { diff --git a/test/ls.js b/test/ls.js index 87ed399..44dcc83 100644 --- a/test/ls.js +++ b/test/ls.js @@ -8,12 +8,15 @@ shell.config.silent = true; shell.rm('-rf', 'tmp'); shell.mkdir('tmp'); +var idx; +var result; + // // Invalids // assert.equal(fs.existsSync('/asdfasdf'), false); // sanity check -var result = shell.ls('/asdfasdf'); // no such file or dir +result = shell.ls('/asdfasdf'); // no such file or dir assert.ok(shell.error()); assert.equal(result.length, 0); @@ -21,15 +24,15 @@ assert.equal(result.length, 0); // Valids // -var result = shell.ls(); +result = shell.ls(); assert.equal(shell.error(), null); -var result = shell.ls('/'); +result = shell.ls('/'); assert.equal(shell.error(), null); // no args shell.cd('resources/ls'); -var result = shell.ls(); +result = shell.ls(); assert.equal(shell.error(), null); assert.equal(result.indexOf('file1') > -1, true); assert.equal(result.indexOf('file2') > -1, true); @@ -41,7 +44,7 @@ assert.equal(result.length, 6); shell.cd('../..'); // simple arg -var result = shell.ls('resources/ls'); +result = shell.ls('resources/ls'); assert.equal(shell.error(), null); assert.equal(result.indexOf('file1') > -1, true); assert.equal(result.indexOf('file2') > -1, true); @@ -53,7 +56,7 @@ assert.equal(result.length, 6); // no args, 'all' option shell.cd('resources/ls'); -var result = shell.ls('-A'); +result = shell.ls('-A'); assert.equal(shell.error(), null); assert.equal(result.indexOf('file1') > -1, true); assert.equal(result.indexOf('file2') > -1, true); @@ -68,7 +71,7 @@ shell.cd('../..'); // no args, 'all' option shell.cd('resources/ls'); -var result = shell.ls('-a'); // (deprecated) backwards compatibility test +result = shell.ls('-a'); // (deprecated) backwards compatibility test assert.equal(shell.error(), null); assert.equal(result.indexOf('file1') > -1, true); assert.equal(result.indexOf('file2') > -1, true); @@ -82,14 +85,14 @@ assert.equal(result.length, 8); shell.cd('../..'); // wildcard, very simple -var result = shell.ls('resources/cat/*'); +result = shell.ls('resources/cat/*'); assert.equal(shell.error(), null); assert.equal(result.indexOf('resources/cat/file1') > -1, true); assert.equal(result.indexOf('resources/cat/file2') > -1, true); assert.equal(result.length, 2); // wildcard, simple -var result = shell.ls('resources/ls/*'); +result = shell.ls('resources/ls/*'); assert.equal(shell.error(), null); assert.equal(result.indexOf('resources/ls/file1') > -1, true); assert.equal(result.indexOf('resources/ls/file2') > -1, true); @@ -102,7 +105,7 @@ assert.ok(result.indexOf('b_dir') > -1); assert.equal(result.length, 7); // wildcard, simple, with -d -var result = shell.ls('-d', 'resources/ls/*'); +result = shell.ls('-d', 'resources/ls/*'); assert.equal(shell.error(), null); assert.equal(result.indexOf('resources/ls/file1') > -1, true); assert.equal(result.indexOf('resources/ls/file2') > -1, true); @@ -113,14 +116,14 @@ assert.ok(result.indexOf('resources/ls/a_dir') > -1); assert.equal(result.length, 6); // wildcard, hidden only -var result = shell.ls('-d', 'resources/ls/.*'); +result = shell.ls('-d', 'resources/ls/.*'); assert.equal(shell.error(), null); assert.equal(result.indexOf('resources/ls/.hidden_file') > -1, true); assert.equal(result.indexOf('resources/ls/.hidden_dir') > -1, true); assert.equal(result.length, 2); // wildcard, mid-file -var result = shell.ls('resources/ls/f*le*'); +result = shell.ls('resources/ls/f*le*'); assert.equal(shell.error(), null); assert.equal(result.length, 5); assert.equal(result.indexOf('resources/ls/file1') > -1, true); @@ -130,25 +133,25 @@ assert.equal(result.indexOf('resources/ls/file2.js') > -1, true); assert.equal(result.indexOf('resources/ls/filename(with)[chars$]^that.must+be-escaped') > -1, true); // wildcard, mid-file with dot (should escape dot for regex) -var result = shell.ls('resources/ls/f*le*.js'); +result = shell.ls('resources/ls/f*le*.js'); assert.equal(shell.error(), null); assert.equal(result.length, 2); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); assert.equal(result.indexOf('resources/ls/file2.js') > -1, true); // one file that exists, one that doesn't -var result = shell.ls('resources/ls/file1.js', 'resources/ls/thisdoesntexist'); +result = shell.ls('resources/ls/file1.js', 'resources/ls/thisdoesntexist'); assert.ok(shell.error()); assert.equal(result.length, 1); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); // wildcard, should not do partial matches -var result = shell.ls('resources/ls/*.j'); // shouldn't get .js +result = shell.ls('resources/ls/*.j'); // shouldn't get .js assert.ok(shell.error()); assert.equal(result.length, 0); // wildcard, all files with extension -var result = shell.ls('resources/ls/*.*'); +result = shell.ls('resources/ls/*.*'); assert.equal(shell.error(), null); assert.equal(result.length, 3); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); @@ -156,7 +159,7 @@ assert.equal(result.indexOf('resources/ls/file2.js') > -1, true); assert.equal(result.indexOf('resources/ls/filename(with)[chars$]^that.must+be-escaped') > -1, true); // wildcard, with additional path -var result = shell.ls('resources/ls/f*le*.js', 'resources/ls/a_dir'); +result = shell.ls('resources/ls/f*le*.js', 'resources/ls/a_dir'); assert.equal(shell.error(), null); assert.equal(result.length, 4); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); @@ -165,7 +168,7 @@ assert.equal(result.indexOf('b_dir') > -1, true); // no wildcard == no path pref assert.equal(result.indexOf('nada') > -1, true); // no wildcard == no path prefix // wildcard for both paths -var result = shell.ls('resources/ls/f*le*.js', 'resources/ls/a_dir/*'); +result = shell.ls('resources/ls/f*le*.js', 'resources/ls/a_dir/*'); assert.equal(shell.error(), null); assert.equal(result.length, 4); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); @@ -174,7 +177,7 @@ assert.equal(result.indexOf('z') > -1, true); assert.equal(result.indexOf('resources/ls/a_dir/nada') > -1, true); // wildcard for both paths, array -var result = shell.ls(['resources/ls/f*le*.js', 'resources/ls/a_dir/*']); +result = shell.ls(['resources/ls/f*le*.js', 'resources/ls/a_dir/*']); assert.equal(shell.error(), null); assert.equal(result.length, 4); assert.equal(result.indexOf('resources/ls/file1.js') > -1, true); @@ -184,7 +187,7 @@ assert.equal(result.indexOf('resources/ls/a_dir/nada') > -1, true); // recursive, no path shell.cd('resources/ls'); -var result = shell.ls('-R'); +result = shell.ls('-R'); assert.equal(shell.error(), null); assert.equal(result.indexOf('a_dir') > -1, true); assert.equal(result.indexOf('a_dir/b_dir') > -1, true); @@ -193,7 +196,7 @@ assert.equal(result.length, 9); shell.cd('../..'); // recusive, path given -var result = shell.ls('-R', 'resources/ls'); +result = shell.ls('-R', 'resources/ls'); assert.equal(shell.error(), null); assert.equal(result.indexOf('a_dir') > -1, true); assert.equal(result.indexOf('a_dir/b_dir') > -1, true); @@ -201,7 +204,7 @@ assert.equal(result.indexOf('a_dir/b_dir/z') > -1, true); assert.equal(result.length, 9); // recusive, path given - 'all' flag -var result = shell.ls('-RA', 'resources/ls'); +result = shell.ls('-RA', 'resources/ls'); assert.equal(shell.error(), null); assert.equal(result.indexOf('a_dir') > -1, true); assert.equal(result.indexOf('a_dir/b_dir') > -1, true); @@ -210,32 +213,37 @@ assert.equal(result.indexOf('a_dir/.hidden_dir/nada') > -1, true); assert.equal(result.length, 14); // recursive, wildcard -var result = shell.ls('-R', 'resources/ls'); +result = shell.ls('-R', 'resources/ls'); assert.equal(shell.error(), null); assert.equal(result.indexOf('a_dir') > -1, true); assert.equal(result.indexOf('a_dir/b_dir') > -1, true); assert.equal(result.indexOf('a_dir/b_dir/z') > -1, true); assert.equal(result.length, 9); +// -Rd works like -d +result = shell.ls('-Rd', 'resources/ls'); +assert.equal(shell.error(), null); +assert.equal(result.length, 1); + // directory option, single arg -var result = shell.ls('-d', 'resources/ls'); +result = shell.ls('-d', 'resources/ls'); assert.equal(shell.error(), null); assert.equal(result.length, 1); // directory option, single arg with trailing '/' -var result = shell.ls('-d', 'resources/ls/'); +result = shell.ls('-d', 'resources/ls/'); assert.equal(shell.error(), null); assert.equal(result.length, 1); // directory option, multiple args -var result = shell.ls('-d', 'resources/ls/a_dir', 'resources/ls/file1'); +result = shell.ls('-d', 'resources/ls/a_dir', 'resources/ls/file1'); assert.equal(shell.error(), null); assert.ok(result.indexOf('resources/ls/a_dir') > -1); assert.ok(result.indexOf('resources/ls/file1') > -1); assert.equal(result.length, 2); // directory option, globbed arg -var result = shell.ls('-d', 'resources/ls/*'); +result = shell.ls('-d', 'resources/ls/*'); assert.equal(shell.error(), null); assert.ok(result.indexOf('resources/ls/a_dir') > -1); assert.ok(result.indexOf('resources/ls/file1') > -1); @@ -247,8 +255,11 @@ assert.ok(result.indexOf('resources/ls/filename(with)[chars$]^that.must+be-escap assert.equal(result.length, 6); // long option, single file -var result = shell.ls('-l', 'resources/ls/file1')[0]; +result = shell.ls('-l', 'resources/ls/file1'); +assert.equal(result.length, 1); +result = result[0]; assert.equal(shell.error(), null); +assert.ok(result.name, 'file1'); assert.equal(result.nlink, 1); assert.equal(result.size, 5); assert.ok(result.mode); // check that these keys exist @@ -260,9 +271,11 @@ assert.ok(result.ctime); // check that these keys exist assert.ok(result.toString().match(/^(\d+ +){5}.*$/)); // long option, glob files -var result = shell.ls('-l', 'resources/ls/f*le1')[0]; +result = shell.ls('-l', 'resources/ls/f*le1'); +assert.equal(result.length, 1); +result = result[0]; assert.equal(shell.error(), null); -assert.equal(result.name, 'resources/ls/file1'); +assert.ok(result.name, 'file1'); assert.equal(result.nlink, 1); assert.equal(result.size, 5); assert.ok(result.mode); // check that these keys exist @@ -274,18 +287,19 @@ assert.ok(result.ctime); // check that these keys exist assert.ok(result.toString().match(/^(\d+ +){5}.*$/)); // long option, directory -var result = shell.ls('-l', 'resources/ls'); +result = shell.ls('-l', 'resources/ls'); assert.equal(shell.error(), null); -var idx; +idx = -1; for (var k=0; k < result.length; k++) { - if (result[k].name === 'resources/ls/file1') { + if (result[k].name === 'file1') { idx = k; break; } } -assert.ok(idx); +assert.ok(idx >= 0); +assert.equal(result.length, 6); result = result[idx]; -assert.equal(result.name, 'resources/ls/file1'); +assert.equal(result.name, 'file1'); assert.equal(result.nlink, 1); assert.equal(result.size, 5); assert.ok(result.mode); // check that these keys exist @@ -296,21 +310,23 @@ assert.ok(result.atime); // check that these keys exist assert.ok(result.ctime); // check that these keys exist assert.ok(result.toString().match(/^(\d+ +){5}.*$/)); -// long option, directory, recursive -var result = shell.ls('-lR', 'resources/ls/'); +// long option, directory, recursive (and windows converts slashes) +result = shell.ls('-lR', 'resources/ls/'); assert.equal(shell.error(), null); -var idx; +idx = -1; for (var k=0; k < result.length; k++) { - if (result[k].name === 'resources/ls/file1') { + if (result[k].name === 'a_dir/b_dir') { idx = k; break; } } -assert.ok(idx); +assert.equal(result.length, 9); +assert.ok(idx >= 0); result = result[idx]; -assert.equal(result.name, 'resources/ls/file1'); -assert.equal(result.nlink, 1); -assert.equal(result.size, 5); +assert.equal(result.name, result.name); +assert.ok(fs.statSync('resources/ls/a_dir/b_dir').isDirectory()); +assert.ok(typeof result.nlink === 'number'); // This can vary between the local machine and travis +assert.ok(typeof result.size === 'number'); // This can vary between different file systems assert.ok(result.mode); // check that these keys exist assert.ok(process.platform === 'win32' || result.uid); // only on unix assert.ok(process.platform === 'win32' || result.gid); // only on unix