fix(which): address @nfischer comments (#657)

shelljs · Jul 14, 2018 · 51fd66a · 51fd66a
1 parent 644e632
commit 51fd66a
Show file tree

Hide file tree

Showing 2 changed files with 15 additions and 8 deletions.
diff --git a/src/which.js b/src/which.js
@@ -16,13 +16,18 @@ var XP_DEFAULT_PATHEXT = '.com;.exe;.bat;.cmd;.vbs;.vbe;.js;.jse;.wsf;.wsh';
 // For earlier versions of NodeJS that doesn't have a list of constants (< v6)
 var FILE_EXECUTABLE_MODE = 1;
 
+function isWindowsPlatform() {
+  return process.platform === 'win32';
+}
+
 // Cross-platform method for splitting environment `PATH` variables
 function splitPath(p) {
   return p ? p.split(path.delimiter) : [];
 }
 
 function isExecutable(pathName) {
   try {
+    // TODO(node-support): replace with fs.constants.X_OK once remove support for node < v6
     fs.accessSync(pathName, FILE_EXECUTABLE_MODE);
   } catch (err) {
     return false;
@@ -31,7 +36,8 @@ function isExecutable(pathName) {
 }
 
 function checkPath(pathName) {
-  return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory() && isExecutable(pathName);
+  return fs.existsSync(pathName) && !common.statFollowLinks(pathName).isDirectory()
+    && (isWindowsPlatform() || isExecutable(pathName));
 }
 
 //@
@@ -49,9 +55,8 @@ function checkPath(pathName) {
 function _which(options, cmd) {
   if (!cmd) common.error('must specify command');
 
-  var isWindows = process.platform === 'win32';
-  var pathEnv = process.env.path || process.env.Path || process.env.PATH;
-  var pathArray = splitPath(pathEnv);
+  var isWindows = isWindowsPlatform();
+  var pathArray = splitPath(process.env.PATH);
 
   var queryMatches = [];
 

diff --git a/test/which.js b/test/which.js
@@ -75,20 +75,22 @@ test('Searching with -a flag returns an array with first item equals to the regu
 
 test('None executable files does not appear in the result list', t => {
   const commandName = 'node'; // Should be an existing command
-  const pathEnv = process.env.path || process.env.Path || process.env.PATH;
   const extraPath = path.resolve(__dirname, 'resources', 'which');
   const matchingFile = path.resolve(extraPath, commandName);
+  const pathEnv = process.env.PATH;
 
   // make sure that file is exists (will throw error otherwise)
-  fs.statSync(matchingFile);
+  t.truthy(fs.existsSync(matchingFile));
 
-  process.env.path = process.env.Path = process.env.PATH = extraPath + path.delimiter + pathEnv;
+  process.env.PATH = extraPath + path.delimiter + process.env.PATH;
   const resultForWhich = shell.which(commandName);
   const resultForWhichA = shell.which('-a', commandName);
   t.falsy(shell.error());
   t.truthy(resultForWhich);
   t.truthy(resultForWhichA);
   t.truthy(resultForWhichA.length);
   t.not(resultForWhich.toString(), matchingFile);
-  t.not(resultForWhichA[0], matchingFile);
+  t.falsy(resultForWhichA.includes(matchingFile));
+
+  process.env.PATH = pathEnv;
 });