From 7cad93a21601a71e29720092f86627fa390c1677 Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Sun, 18 Feb 2024 01:04:02 -0800 Subject: [PATCH 1/2] deprecate config.globOptions This removes support for configuring `config.globOptions`. Exposing this variable makes it difficult to change (or upgrade) our glob library. It's best to consider our choice of glob library to be an implementation detail. As far as I know, this is not a commonly used option: https://github.com/shelljs/shelljs/issues?q=globOptions currently shows no GitHub issues of users using this option, and there was never really a motivation for why this needed to be exposed (#400 exposed the option just because we could, not because we should). This is one step toward supporting Issue #828. --- README.md | 11 ----------- shell.js | 12 ------------ src/common.js | 3 +-- test/config.js | 15 +-------------- 4 files changed, 2 insertions(+), 39 deletions(-) diff --git a/README.md b/README.md index 357e5a47..720b0c2c 100644 --- a/README.md +++ b/README.md @@ -854,16 +854,6 @@ rm -rf foo.txt bar.txt exec echo hello ``` -### config.globOptions - -Example: - -```javascript -config.globOptions = {nodir: true}; -``` - -Use this value for calls to `glob.sync()` instead of the default options. - ### config.reset() Example: @@ -882,7 +872,6 @@ Reset `shell.config` to the defaults: ```javascript { fatal: false, - globOptions: {}, maxdepth: 255, noglob: false, silent: false, diff --git a/shell.js b/shell.js index ad40740b..aa33bb6d 100644 --- a/shell.js +++ b/shell.js @@ -135,17 +135,6 @@ exports.config = common.config; //@ exec echo hello //@ ``` -//@ -//@ ### config.globOptions -//@ -//@ Example: -//@ -//@ ```javascript -//@ config.globOptions = {nodir: true}; -//@ ``` -//@ -//@ Use this value for calls to `glob.sync()` instead of the default options. - //@ //@ ### config.reset() //@ @@ -165,7 +154,6 @@ exports.config = common.config; //@ ```javascript //@ { //@ fatal: false, -//@ globOptions: {}, //@ maxdepth: 255, //@ noglob: false, //@ silent: false, diff --git a/src/common.js b/src/common.js index 0ec1f813..9b258927 100644 --- a/src/common.js +++ b/src/common.js @@ -19,7 +19,6 @@ var isElectron = Boolean(process.versions.electron); // Module globals (assume no execPath by default) var DEFAULT_CONFIG = { fatal: false, - globOptions: {}, maxdepth: 255, noglob: false, silent: false, @@ -263,7 +262,7 @@ function expand(list) { } else { var ret; try { - ret = glob.sync(listEl, config.globOptions); + ret = glob.sync(listEl, {}); // if nothing matched, interpret the string literally ret = ret.length > 0 ? ret : [listEl]; } catch (e) { diff --git a/test/config.js b/test/config.js index 743df356..34aa036c 100644 --- a/test/config.js +++ b/test/config.js @@ -48,7 +48,7 @@ test.cb('config.fatal = true', t => { }); // -// config.globOptions +// Default glob expansion behavior // test('Expands to directories by default', t => { @@ -60,16 +60,3 @@ test('Expands to directories by default', t => { t.truthy(result.indexOf('test/resources/head') > -1); t.truthy(result.indexOf('test/resources/external') > -1); }); - -test( - 'Check to make sure options get passed through (nodir is an example)', - t => { - shell.config.globOptions = { nodir: true }; - const result = common.expand(['test/resources/*a*']); - t.is(result.length, 2); - t.truthy(result.indexOf('test/resources/a.txt') > -1); - t.truthy(result.indexOf('test/resources/badlink') > -1); - t.truthy(result.indexOf('test/resources/cat') < 0); - t.truthy(result.indexOf('test/resources/external') < 0); - } -); From 30154157e4274042b254b824ab7782d08ae754f8 Mon Sep 17 00:00:00 2001 From: Nate Fischer Date: Sun, 18 Feb 2024 01:12:31 -0800 Subject: [PATCH 2/2] feat: switch to fast-glob This removes `node-glob` in favor of `fast-glob`. The main motivation for this is because `node-glob` has a security warning and I can't update to `node-glob@9` unless we drop compatibility for node v8. Switching to `fast-glob` seems to be fairly straightforward, although some options need to be changed by default for bash compatibility. Fixes #828 Fixes #1149 --- package.json | 2 +- src/common.js | 9 +++++++-- src/ls.js | 23 +++++++++++++++-------- 3 files changed, 23 insertions(+), 11 deletions(-) diff --git a/package.json b/package.json index 40619da4..77bc110b 100644 --- a/package.json +++ b/package.json @@ -55,7 +55,7 @@ }, "dependencies": { "execa": "^1.0.0", - "glob": "^7.0.0", + "fast-glob": "^3.3.2", "interpret": "^1.0.0", "rechoir": "^0.6.2" }, diff --git a/src/common.js b/src/common.js index 9b258927..828bcc06 100644 --- a/src/common.js +++ b/src/common.js @@ -6,7 +6,7 @@ var os = require('os'); var fs = require('fs'); -var glob = require('glob'); +var glob = require('fast-glob'); var shell = require('..'); var shellMethods = Object.create(shell); @@ -262,7 +262,12 @@ function expand(list) { } else { var ret; try { - ret = glob.sync(listEl, {}); + ret = glob.sync(listEl, { + // These options are just to make fast-glob be compatible with POSIX + // (bash) wildcard behavior. + onlyFiles: false, + followSymbolicLinks: false, + }); // if nothing matched, interpret the string literally ret = ret.length > 0 ? ret : [listEl]; } catch (e) { diff --git a/src/ls.js b/src/ls.js index 4faa30b0..57442015 100644 --- a/src/ls.js +++ b/src/ls.js @@ -1,7 +1,7 @@ var path = require('path'); var fs = require('fs'); var common = require('./common'); -var glob = require('glob'); +var glob = require('fast-glob'); var globPatternRecursive = path.sep + '**'; @@ -105,13 +105,20 @@ function _ls(options, paths) { if (stat.isDirectory() && !options.directory) { if (options.recursive) { // use glob, because it's simple - glob.sync(p + globPatternRecursive, { dot: options.all, follow: options.link }) - .forEach(function (item) { - // Glob pattern returns the directory itself and needs to be filtered out. - if (path.relative(p, item)) { - pushFile(item, path.relative(p, item)); - } - }); + glob.sync(p + globPatternRecursive, { + // These options are just to make fast-glob be compatible with POSIX + // (bash) wildcard behavior. + onlyFiles: false, + + // These options depend on the cmdOptions provided to ls. + dot: options.all, + followSymbolicLinks: options.link, + }).forEach(function (item) { + // Glob pattern returns the directory itself and needs to be filtered out. + if (path.relative(p, item)) { + pushFile(item, path.relative(p, item)); + } + }); } else if (options.all) { // use fs.readdirSync, because it's fast fs.readdirSync(p).forEach(function (item) {