diff --git a/CHANGES.md b/CHANGES.md index 05076ea..4739476 100644 --- a/CHANGES.md +++ b/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. diff --git a/lib/common.js b/lib/common.js index fe54f0a..c48ab1b 100644 --- a/lib/common.js +++ b/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'}); - kid.on('exit', function (code) { - if (code) { - return (cb(code)); + var argv = argvFromLine(format('%s "%s"', editor, tmpPath)); + if (opts.log) { + opts.log.trace({argv: argv}, 'editInEditor argv'); + } + + 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: diff --git a/lib/do_profile/do_edit.js b/lib/do_profile/do_edit.js index ed3f6fb..fa970c8 100644 --- a/lib/do_profile/do_edit.js +++ b/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)); diff --git a/lib/errors.js b/lib/errors.js index 04ed552..59fe810 100644 --- a/lib/errors.js +++ b/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, diff --git a/test/unit/argvFromLine.test.js b/test/unit/argvFromLine.test.js new file mode 100644 index 0000000..1348c98 --- /dev/null +++ b/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(); + }); + }); +});