fix: only wait for stdin when appropriate (#98)

Don't deadlock waiting for stdin during situations when stdin is not expected.

Fixes #95

src/cli.js

@@ -1,5 +1,9 @@
 #!/usr/bin/env node
 import { shx } from './shx';
+import minimist from 'minimist';
+import { shouldReadStdin } from './config';
+
+const parsedArgs = minimist(process.argv.slice(2), { stopEarly: true, boolean: true });
 
 // `input` is null if we're running from a TTY, or a string of all stdin if
 // running from the right-hand side of a pipe
@@ -22,12 +26,12 @@ const run = (input) => {
 };
 
 // ShellJS doesn't support input streams, so we have to collect all input first
-if (process.stdin.isTTY) {
-  // There's no stdin, so we can immediately invoke the ShellJS function
-  run(null);
-} else {
+if (shouldReadStdin(parsedArgs._)) {
   // Read all stdin first, and then pass that onto ShellJS
   const chunks = [];
   process.stdin.on('data', data => chunks.push(data));
   process.stdin.on('end', () => run(chunks.join('')));
+} else {
+  // There's no stdin, so we can immediately invoke the ShellJS function
+  run(null);
 }

src/config.js

@@ -1,3 +1,5 @@
+import minimist from 'minimist';
+
 export const EXIT_CODES = {
   SHX_ERROR: 27, // https://xkcd.com/221/
   CMD_FAILED: 1, // TODO: Once shelljs/shelljs#269 lands, use `error()`
@@ -16,3 +18,35 @@ export const CMD_BLACKLIST = [
 ];
 
 export const CONFIG_FILE = '.shxrc.json';
+
+export const SHELLJS_PIPE_INFO = {
+  cat: { minArgs: 1 },
+  grep: { minArgs: 2 },
+  head: { minArgs: 1 },
+  sed: { minArgs: 2 },
+  sort: { minArgs: 1 },
+  tail: { minArgs: 1 },
+  uniq: { minArgs: 1 },
+};
+
+// All valid options
+const allOptionsList = 'abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ'
+  .split('');
+
+export const shouldReadStdin = (args) => {
+  const cmd = args[0];
+  const cmdInfo = SHELLJS_PIPE_INFO[cmd];
+  const parsedArgs = minimist(args.slice(1), {
+    stopEarly: true,
+    boolean: allOptionsList, // treat all short options as booleans
+  });
+  let requiredNumArgs = cmdInfo ? cmdInfo.minArgs : -1;
+
+  // If a non-boolean option is passed in, incrememnt the required argument
+  // count (this is the case for `-n` for `head` and `tail`)
+  if (parsedArgs.n && (cmd === 'head' || cmd === 'tail')) {
+    requiredNumArgs++;
+  }
+
+  return Boolean(!process.stdin.isTTY && parsedArgs._.length < requiredNumArgs);
+};

test/specs/cli.js

@@ -1,5 +1,5 @@
 import { shx } from '../../src/shx';
-import { EXIT_CODES, CONFIG_FILE } from '../../src/config';
+import { EXIT_CODES, CONFIG_FILE, shouldReadStdin } from '../../src/config';
 import * as mocks from '../mocks';
 import * as shell from 'shelljs';
 import fs from 'fs';
@@ -130,6 +130,63 @@ describe('cli', () => {
     output.code.should.equal(0);
   });
 
+  describe('stdin', () => {
+    const oldTTY = process.stdin.isTTY;
+    after(() => {
+      process.stdin.isTTY = oldTTY;
+    });
+
+    it('reads stdin for commands that accept stdin', () => {
+      process.stdin.isTTY = undefined;
+      shouldReadStdin(['cat']).should.equal(true);
+      shouldReadStdin(['head']).should.equal(true);
+      shouldReadStdin(['sed', 's/foo/bar/g']).should.equal(true);
+      shouldReadStdin(['grep', 'a.*z']).should.equal(true);
+    });
+
+    it('reads stdin if only options are given', () => {
+      process.stdin.isTTY = undefined;
+      shouldReadStdin(['head', '-n', '1']).should.equal(true);
+      shouldReadStdin(['grep', '-i', 'a.*z']).should.equal(true);
+    });
+
+    it('does not read stdin if process.stdin is a TTY', () => {
+      process.stdin.isTTY = true;
+      shouldReadStdin(['cat']).should.equal(false);
+      shouldReadStdin(['head']).should.equal(false);
+      shouldReadStdin(['sed', 's/foo/bar/g']).should.equal(false);
+      shouldReadStdin(['grep', 'a.*z']).should.equal(false);
+    });
+
+    it('does not read stdin if command does not accept stdin', () => {
+      process.stdin.isTTY = undefined;
+      // It shouldn't matter for this function if these have valid numbers of
+      // arguments or not, so let's test both
+      shouldReadStdin(['rm']).should.equal(false);
+      shouldReadStdin(['ls', 'dir']).should.equal(false);
+      shouldReadStdin(['cp', 'a']).should.equal(false);
+    });
+
+    it('does not read stdin if files are given as arguments', () => {
+      process.stdin.isTTY = undefined;
+      shouldReadStdin(['cat', 'file.txt']).should.equal(false);
+      shouldReadStdin(['head', 'file.txt']).should.equal(false);
+      shouldReadStdin(['sed', 's/foo/bar/g', 'file.txt']).should.equal(false);
+      shouldReadStdin(['grep', 'a.*z', 'file.txt']).should.equal(false);
+      // Lots of files
+      shouldReadStdin(['cat', 'file.txt', 'file2.txt', 'file3.txt'])
+        .should.equal(false);
+    });
+
+    it('does not read stdin if both options and files are given', () => {
+      process.stdin.isTTY = undefined;
+      shouldReadStdin(['head', '-n', '1', 'file.txt']).should.equal(false);
+      shouldReadStdin(['sed', '-i', 's/foo/bar/g', 'file.txt'])
+        .should.equal(false);
+      shouldReadStdin(['grep', '-i', 'a.*z', 'file.txt']).should.equal(false);
+    });
+  });
+
   describe('plugin', () => {
     afterEach(() => {
       shell.rm('-f', CONFIG_FILE);
@@ -174,7 +231,7 @@ describe('cli', () => {
       const output = cli('help');
       output.stderr.should.equal('');
       output.stdout.should.match(/Usage/); // make sure help is printed
-      output.stdout.should.match(/- open/); // make sure help includes new command
+      output.stdout.should.match(/- open/); // help should include new command
     });
   });
 
@@ -209,14 +266,17 @@ describe('cli', () => {
     beforeEach(() => {
       // create test files
       shell.touch(testFileName1);
-      shell.ShellString('foo\nfoosomething\nfoofoosomething\n').to(testFileName1);
+      shell.ShellString('foo\nfoosomething\nfoofoosomething\n')
+        .to(testFileName1);
 
       shell.mkdir('-p', 's/weirdfile/name');
       shell.touch(testFileName2);
-      shell.ShellString('foo\nfoosomething\nfoofoosomething\n').to(testFileName2);
+      shell.ShellString('foo\nfoosomething\nfoofoosomething\n')
+        .to(testFileName2);
 
       shell.touch(testFileName3);
-      shell.ShellString('http://www.nochange.com\nhttp://www.google.com\n').to(testFileName3);
+      shell.ShellString('http://www.nochange.com\nhttp://www.google.com\n')
+        .to(testFileName3);
     });
 
     afterEach(() => {
@@ -228,13 +288,15 @@ describe('cli', () => {
     it('works with no /g and no -i', () => {
       const output = cli('sed', 's/foo/bar/', testFileName1);
       output.stdout.should.equal('bar\nbarsomething\nbarfoosomething\n');
-      shell.cat(testFileName1).stdout.should.equal('foo\nfoosomething\nfoofoosomething\n');
+      shell.cat(testFileName1).stdout.should
+        .equal('foo\nfoosomething\nfoofoosomething\n');
     });
 
     it('works with /g and -i', () => {
       const output = cli('sed', '-i', 's/foo/bar/g', testFileName1);
       output.stdout.should.equal('bar\nbarsomething\nbarbarsomething\n');
-      shell.cat(testFileName1).stdout.should.equal('bar\nbarsomething\nbarbarsomething\n');
+      shell.cat(testFileName1).stdout.should
+        .equal('bar\nbarsomething\nbarbarsomething\n');
     });
 
     it('works with regexes conatining slashes', () => {
@@ -243,14 +305,17 @@ describe('cli', () => {
         's/http:\\/\\/www\\.google\\.com/https:\\/\\/www\\.facebook\\.com/',
         testFileName3
       );
-      output.stdout.should.equal('http://www.nochange.com\nhttps://www.facebook.com\n');
-      shell.cat(testFileName3).stdout.should.equal('http://www.nochange.com\nhttp://www.google.com\n');
+      output.stdout.should
+        .equal('http://www.nochange.com\nhttps://www.facebook.com\n');
+      shell.cat(testFileName3).stdout.should
+        .equal('http://www.nochange.com\nhttp://www.google.com\n');
     });
 
     it('works with weird file names', () => {
       const output = cli('sed', 's/foo/bar/', testFileName2);
       output.stdout.should.equal('bar\nbarsomething\nbarfoosomething\n');
-      shell.cat(testFileName2).stdout.should.equal('foo\nfoosomething\nfoofoosomething\n');
+      shell.cat(testFileName2).stdout.should
+        .equal('foo\nfoosomething\nfoofoosomething\n');
     });
   });
 
@@ -282,9 +347,11 @@ describe('cli', () => {
     skipIf(process.platform === 'win32', 'works with numeric mode', () => {
       // bitmasking is to ignore the upper bits
       cli('chmod', '644', testFileName);
-      (fs.statSync(testFileName).mode & bitMask).should.equal(parseInt('644', 8));
+      (fs.statSync(testFileName).mode & bitMask).should
+        .equal(parseInt('644', 8));
       cli('chmod', '755', testFileName);
-      (fs.statSync(testFileName).mode & bitMask).should.equal(parseInt('755', 8));
+      (fs.statSync(testFileName).mode & bitMask).should
+        .equal(parseInt('755', 8));
     });
     skipIf(process.platform === 'win32', 'works with symbolic mode (all)', () => {
       // bitmasking is to ignore the upper bits