Fix and improve quote support in Run Options (#1424)

* Fix using quotes in Run Options

Currently, whenever Command Arguments or Program Arguments contain quotes in
the Run Options dialog, then the script blows up with a "Uncaught TypeError:
Cannot read property 'concat' of undefined".

Commit 668057c ("Make linter happy and fix convert errors", 2016-12-05) changed
`(matches != null) ? matches : []` to `!matches ? matches : []`, which reverses
the original meaning. This only causes errors when the string contains quotes,
because there's an early return in this function.

Fixes #1339.

* Test argument splitting

PR #742, that changed the logic from the simple "split on space" logic to the
current version, mentions these cases as the ones that were tested with this
implementation. Add tests to guarantee behavior remains the same for these
cases.

* Allow using nested quotes and parentheses in Run Options

These changes allow using the expression shown in the added test, which allows
running racket scripts. This usage differs from the default racket behavior,
because this also outputs the return value of the script.

The change to `matches` creation uses a regex backreference and the lazy
(non-greedy) quantifier to only match outer quotes. So whereas the current
version would've matched both `"{FILE_ACTIVE}"` and `'(load "{FILE_ACTIVE}")'`,
then the new version only matches the outer quotes. The following code then
broke with the previous version, trying to match and replace both of these
overlapping instances.

The `args.replace` change was necessary, because otherwise the argument was
used as regex, and any parentheses and other regex operators were not being
escaped. Using simple substring replacing instead avoids the need to escape
special characters.

The last change avoids removing all quotes from the arguments and instead only
replaces matching quotes that are at the beginning and end of the argument.

I'm not sure if the "strips quotes from argument values" cases were actually
useful in any situation or the behavior was simply incidental. Currently
removed support for that behavior. If this behavior turns out to be necessary,
then separate handling should be added for it.

* Simplify and improve argument splitting logic

Instead of multiple back and forth replacements, this uses a single regex to
find the arguments and simply adds them together while looping over the
provided text.

As far as I can tell, this now behaves exactly like most shells do. Except for
escaped quotes. This does not support escaping quotes.

This re-adds support for the "strips quotes from argument values" tests that
was removed in the previous commit c4c48cb ("Allow using nested quotes and
parentheses in Run Options", 2017-09-17).

Author: deiwin

lib/script-options-view.js

@@ -87,55 +87,39 @@ export default class ScriptOptionsView extends View {
     this.panel.hide();
   }
 
-  splitArgs(element) {
-    let args = element.get(0).getModel().getText().trim();
-
-    if (args.indexOf('"') === -1 && args.indexOf("'") === -1) {
-      // no escaping, just split
-      return (args.split(' ').filter(item => item !== '').map(item => item));
-    }
-
-    const replaces = {};
-
-    const regexps = [/"[^"]*"/ig, /'[^']*'/ig];
-
-    let matches;
-    // find strings in arguments
-    regexps.forEach((regex) => {
-      matches = (!matches ? matches : []).concat((args.match(regex)) || []);
-    });
-
-    // format replacement as bash comment to avoid replacing valid input
-    matches.forEach((match) => {
-      replaces[`\`#match${Object.keys(replaces).length + 1}\``] = match;
-    });
-
-    // replace strings
-    for (const match in replaces) {
-      const part = replaces[match];
-      args = args.replace(new RegExp(part, 'g'), match);
-    }
-    const split = (args.split(' ').filter(item => item !== '').map(item => item));
-
-    const replacer = (argument) => {
-      for (const match in replaces) {
-        const replacement = replaces[match];
-        argument = argument.replace(match, replacement);
+  static splitArgs(argText) {
+    const text = argText.trim();
+    const argSubstringRegex = /([^'"\s]+)|((["'])(.*?)\3)/g;
+    const args = [];
+    let lastMatchEndPosition = -1;
+    let match = argSubstringRegex.exec(text);
+    while (match !== null) {
+      const matchWithoutQuotes = match[1] || match[4];
+      // Combine current result with last match, if last match ended where this
+      // one begins.
+      if (lastMatchEndPosition === match.index) {
+        args[args.length - 1] += matchWithoutQuotes;
+      } else {
+        args.push(matchWithoutQuotes);
       }
-      return argument;
-    };
 
-    // restore strings, strip quotes
-    return split.map(argument => replacer(argument).replace(/"|'/g, ''));
+      lastMatchEndPosition = argSubstringRegex.lastIndex;
+      match = argSubstringRegex.exec(text);
+    }
+    return args;
   }
 
   getOptions() {
     return {
       workingDirectory: this.inputCwd.get(0).getModel().getText(),
       cmd: this.inputCommand.get(0).getModel().getText(),
-      cmdArgs: this.splitArgs(this.inputCommandArgs),
+      cmdArgs: this.constructor.splitArgs(
+        this.inputCommandArgs.get(0).getModel().getText(),
+      ),
       env: this.inputEnv.get(0).getModel().getText(),
-      scriptArgs: this.splitArgs(this.inputScriptArgs),
+      scriptArgs: this.constructor.splitArgs(
+        this.inputScriptArgs.get(0).getModel().getText(),
+      ),
     };
   }
 

spec/script-options-view-spec.js

@@ -0,0 +1,67 @@
+'use babel';
+
+/* eslint-disable no-underscore-dangle */
+import ScriptOptionsView from '../lib/script-options-view';
+
+describe('ScriptOptionsView', () => {
+  describe('splitArgs', () => {
+    [{
+      text: '',
+      expectedArgs: [],
+      description: 'returns an empty array for empty string',
+    }, {
+      text: ' \t\n',
+      expectedArgs: [],
+      description: 'returns an empty array for just whitespace',
+    }, {
+      text: 'arg1 arg2',
+      expectedArgs: ['arg1', 'arg2'],
+      description: 'splits arguments on whitespace',
+    }, {
+      text: 'arg1=val1 arg2',
+      expectedArgs: ['arg1=val1', 'arg2'],
+      description: 'keeps argument values',
+    }, {
+      text: '"foo bar" arg2',
+      expectedArgs: ['foo bar', 'arg2'],
+      description: 'does not split quoted arguments on whitespace',
+    }, {
+      text: '\'foo bar\' arg2',
+      expectedArgs: ['foo bar', 'arg2'],
+      description: 'recognizes single quotes',
+    }, {
+      text: '"foo bar" "another string"',
+      expectedArgs: ['foo bar', 'another string'],
+      description: 'handles multiple quoted arguments',
+    }, {
+      text: '\'foo bar\' \'another string\'',
+      expectedArgs: ['foo bar', 'another string'],
+      description: 'handles multiple single quoted arguments',
+    }, {
+      text: '"foo bar" \'another string\'',
+      expectedArgs: ['foo bar', 'another string'],
+      description: 'handles multiple quoted arguments, with mixed single and double quotes',
+    }, {
+      text: 'arg1="foo bar"',
+      expectedArgs: ['arg1=foo bar'],
+      description: 'strips quotes from argument values',
+    }, {
+      text: 'arg1=\'foo bar\'',
+      expectedArgs: ['arg1=foo bar'],
+      description: 'strips single quotes from argument values',
+    }, {
+      text: '-e \'(load "{FILE_ACTIVE}")\'',
+      expectedArgs: ['-e', '(load "{FILE_ACTIVE}")'],
+      description: 'keeps nested quotes intact',
+    }, {
+      text: 'we"ird way to inc"l"ude spaces in arg"s',
+      expectedArgs: ['weird way to include spaces in args'],
+      description: 'supports multiple top level quotes',
+    }].forEach(({ text, expectedArgs, description }) => {
+      it(description, () => {
+        const args = ScriptOptionsView.splitArgs(text);
+        expect(args).toEqual(expectedArgs);
+      });
+    });
+  });
+});