joyent/node-triton#148 triton profile edit failes if EDITOR contains a space

CHANGES.md

@@ -7,6 +7,8 @@ Known issues:
 
 ## not yet released
 
+- [joyent/node-triton#148] Fix `triton profile edit ...` to work with an
+  "EDITOR" environment variable with quotes and spaces.
 - [joyent/node-triton#183] `triton profile create` will no longer use ANSI
   codes for styling if stdout isn't a TTY.
 

lib/common.js

@@ -796,18 +796,26 @@ function cliSetupTritonApi(opts, cb) {
 function editInEditor(opts, cb) {
     assert.string(opts.text, 'opts.text');
     assert.optionalString(opts.filename, 'opts.filename');
+    assert.optionalObject(opts.log, 'opts.log');
     assert.func(cb, 'cb');
 
     var tmpPath = path.resolve(os.tmpDir(),
         format('triton-%s-edit-%s', process.pid, opts.filename || 'text'));
     fs.writeFileSync(tmpPath, opts.text, 'utf8');
 
-    // TODO: want '-f' opt for vi? What about others?
     var editor = process.env.EDITOR || '/usr/bin/vi';
-    var kid = child_process.spawn(editor, [tmpPath], {stdio: 'inherit'});
+    var argv = argvFromLine(format('%s "%s"', editor, tmpPath));
-    kid.on('exit', function (code) {
+    if (opts.log) {
-        if (code) {
+        opts.log.trace({argv: argv}, 'editInEditor argv');
-            return (cb(code));
+    }
+
+    var kid = child_process.spawn(argv[0], argv.slice(1), {stdio: 'inherit'});
+    kid.on('exit', function (code, signal) {
+        if (code || signal) {
+            cb(new errors.TritonError(format(
+                'editor terminated abnormally: argv=%j, code=%j, signal=%j',
+                argv, code, signal)));
+            return;
         }
         var afterText = fs.readFileSync(tmpPath, 'utf8');
         fs.unlinkSync(tmpPath);
@@ -1080,6 +1088,108 @@ function objFromKeyValueArgs(args, opts)
 }
 
 
+/*
+ * Parse the given line into an argument vector, e.g. for use in sending to
+ * `child_process.spawn(argv[0], argv.slice(1), ...)`.
+ *
+ * Translated from the Python `line2argv` in https://github.com/trentm/cmdln
+ * See also the tests in "test/unit/argvFromLine.test.js".
+ *
+ * @throws {Error} if there are unbalanced quotes or some other parse failure.
+ */
+function argvFromLine(line) {
+    assert.string(line, 'line');
+
+    var trimmed = line.trim();
+    var argv = [];
+    var state = 'default';
+    var arg = null; // the current argument being parsed
+    var i = -1;
+    var WHITESPACE = {
+        ' ': true,
+        '\t': true,
+        '\n': true,
+        '\r': true
+        // Other whitespace chars?
+    };
+
+    while (true) {
+        i += 1;
+        if (i >= trimmed.length) {
+            break;
+        }
+        var ch = trimmed[i];
+
+        // An escaped char always added to the arg.
+        if (ch == '\\' && i+1 < trimmed.length) {
+            if (arg === null) { arg = ''; }
+            /*
+             * Include the escaping backslash, unless it is escaping a quote
+             * inside a quoted string. E.g.:
+             *      foo\Xbar    =>  foo\Xbar
+             *      'foo\'bar'  =>  foo'bar
+             *      "foo\"bar"  =>  foo"bar
+             *
+             * Note that cmdln.py's line2argv had a Windows-specific subtlety
+             * here (dating to cmdln commit 87430930160f) that we are skipping
+             * for now.
+             */
+            if ((state === 'double-quoted' && trimmed[i+1] !== '"') ||
+                (state === 'single-quoted' && trimmed[i+1] !== '\'')) {
+                arg += ch;
+            }
+            i += 1;
+            arg += trimmed[i];
+            continue;
+        }
+
+        if (state === 'single-quoted') {
+            if (ch === '\'') {
+                state = 'default';
+            } else {
+                arg += ch;
+            }
+        } else if (state === 'double-quoted') {
+            if (ch === '"') {
+                state = 'default';
+            } else {
+                arg += ch;
+            }
+        } else if (state === 'default') {
+            if (ch === '"') {
+                if (arg === null) { arg = ''; }
+                state = 'double-quoted';
+            } else if (ch === '\'') {
+                if (arg === null) { arg = ''; }
+                state = 'single-quoted';
+            } else if (WHITESPACE.hasOwnProperty(ch)) {
+                if (arg !== null) {
+                    argv.push(arg);
+                }
+                arg = null;
+            } else {
+                if (arg === null) { arg = ''; }
+                arg += ch;
+            }
+        }
+    }
+    if (arg !== null) {
+        argv.push(arg);
+    }
+
+    /*
+     * Note: cmdln.py's line2argv would not throw this error on Windows, i.e.
+     * allowing unclosed quoted-strings. This impl. is not following that lead.
+     */
+    if (state !== 'default') {
+        throw new Error(format('unfinished %s segment in line: %j',
+            state, line));
+    }
+
+    return argv;
+}
+
+
 
 //---- exports
 
@@ -1115,6 +1225,7 @@ module.exports = {
     execPlus: execPlus,
     deepEqual: deepEqual,
     tildeSync: tildeSync,
-    objFromKeyValueArgs: objFromKeyValueArgs
+    objFromKeyValueArgs: objFromKeyValueArgs,
+    argvFromLine: argvFromLine
 };
 // vim: set softtabstop=4 shiftwidth=4:

lib/do_profile/do_edit.js

@@ -85,7 +85,8 @@ function _editProfile(opts, cb) {
     function editAttempt(text) {
         common.editInEditor({
             text: text,
-            filename: filename
+            filename: filename,
+            log: cli.log
         }, function (err, afterText, changed) {
             if (err) {
                 return cb(new errors.TritonError(err));

lib/errors.js

@@ -101,7 +101,7 @@ function TritonError(cause, message) {
         message = cause;
         cause = undefined;
     }
-    assert.string(message);
+    assert.string(message, 'message');
     _TritonBaseVError.call(this, {
         cause: cause,
         message: message,

test/unit/argvFromLine.test.js

@@ -0,0 +1,200 @@
+/*
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+/*
+ * Copyright 2017 Joyent, Inc.
+ */
+
+/*
+ * Unit tests for `argvFromLine()` in "common.js".
+ */
+
+var assert = require('assert-plus');
+var format = require('util').format;
+var test = require('tape');
+
+var argvFromLine = require('../../lib/common').argvFromLine;
+
+
+// ---- globals
+
+var log = require('../lib/log');
+
+var debug = function () {};
+// debug = console.warn;
+
+
+// ---- test cases
+
+var cases = [
+    {
+        line: '"/Applications/Mac Vim/MacVim.app/Contents/MacOS/Vim" -fg',
+        expect: {
+            argv: ['/Applications/Mac Vim/MacVim.app/Contents/MacOS/Vim', '-fg']
+        }
+    },
+
+    {
+        line: 'foo',
+        expect: {
+            argv: ['foo']
+        }
+    },
+    {
+        line: 'foo bar',
+        expect: {
+            argv: ['foo', 'bar']
+        }
+    },
+    {
+        line: 'foo bar ',
+        expect: {
+            argv: ['foo', 'bar']
+        }
+    },
+    {
+        line: ' foo bar',
+        expect: {
+            argv: ['foo', 'bar']
+        }
+    },
+
+
+    // Quote handling
+    {
+        line: '\'foo bar\'',
+        expect: {
+            argv: ['foo bar']
+        }
+    },
+    {
+        line: '"foo bar"',
+        expect: {
+            argv: ['foo bar']
+        }
+    },
+    {
+        line: '"foo\\"bar"',
+        expect: {
+            argv: ['foo"bar']
+        }
+    },
+    {
+        line: '"foo bar" spam',
+        expect: {
+            argv: ['foo bar', 'spam']
+        }
+    },
+    {
+        line: '"foo "bar spam',
+        expect: {
+            argv: ['foo bar', 'spam']
+        }
+    },
+
+    {
+        line: 'some\tsimple\ttests',
+        expect: {
+            argv: ['some', 'simple', 'tests']
+        }
+    },
+    {
+        line: 'a "more complex" test',
+        expect: {
+            argv: ['a', 'more complex', 'test']
+        }
+    },
+    {
+        line: 'a more="complex test of " quotes',
+        expect: {
+            argv: ['a', 'more=complex test of ', 'quotes']
+        }
+    },
+    {
+        line: 'a more" complex test of " quotes',
+        expect: {
+            argv: ['a', 'more complex test of ', 'quotes']
+        }
+    },
+    {
+        line: 'an "embedded \\"quote\\""',
+        expect: {
+            argv: ['an', 'embedded "quote"']
+        }
+    },
+
+    {
+        line: 'foo bar C:\\',
+        expect: {
+            argv: ['foo', 'bar', 'C:\\']
+        }
+    },
+    {
+        line: '"\\test\\slash" "foo bar" "foo\\"bar"',
+        expect: {
+            argv: ['\\test\\slash', 'foo bar', 'foo"bar']
+        }
+    },
+
+    {
+        line: '\\foo\\bar',
+        expect: {
+            argv: ['foobar']
+        }
+    },
+    {
+        line: '\\\\foo\\\\bar',
+        expect: {
+            argv: ['\\foo\\bar']
+        }
+    },
+
+    {
+        line: '"foo',
+        expect: {
+            err: /unfinished .* segment in line/
+        }
+    }
+];
+
+
+// ---- test driver
+
+test('argvFromLine', function (tt) {
+    cases.forEach(function (c, num) {
+        var testName = format('case %d: %s', num, c.line);
+        tt.test(testName, function (t) {
+            debug('--', num);
+            debug('c: %j', c);
+
+            var argv = null;
+            var err = null;
+            try {
+                argv = argvFromLine(c.line);
+            } catch (err_) {
+                err = err_;
+            }
+
+            if (c.expect.err) {
+                var errRegexps = (Array.isArray(c.expect.err)
+                    ? c.expect.err : [c.expect.err]);
+                errRegexps.forEach(function (regexp) {
+                    assert.regexp(regexp, 'case.expect.err');
+                    t.ok(err, 'expected an error');
+                    t.ok(regexp.test(err.message), format(
+                        'error message matches %s, actual %j',
+                        regexp, err.message));
+                });
+            } else {
+                t.ifError(err, 'no err');
+            }
+            if (c.expect.hasOwnProperty('argv')) {
+                t.deepEqual(argv, c.expect.argv);
+            }
+            t.end();
+        });
+    });
+});