Properly handle quotes, backslashes, etc in commands at console

[haltman-at]

PR description

Addresses #5838.

This PR makes it so that arguments to Truffle commands at the Truffle console are handled like arguments at the actual command line. This is intended primarily so that truffle call (#5647) will work properly at the Truffle console once it is merged, but it also fixes bugs with existing commands.

For instance, suppose you have a test file test/meta coin.js, and you are at the Truffle console, and you want to run specifically the tests in this file and no others. How would you do that? Without this PR, there's no straightforward way! With it, you can just do test 'test/meta coin.js' or test test/meta\ coin.js or similar.

(This PR has been substantially overhauled, original description is in strikeout below.)

I originally did this using the shell-quote package, but it turned out to have substantial problems (see discussion below), so I ended up rolling my own function to handle shell quotes and escapes. I put it in command-utils.js; it's called parseQuotesAndEscapes. I also wrote a bunch of tests for it.

Quoting and escaping works basically like in bash -- backslash is escape and (outside of a quote) causes whatever is after it to be taken literally; double-quotes cause whatever's inside to be taken literally except for the sequence \", which escapes the double-quote; single-quotes are similar but don't allow any escaping at all. Other than that, arguments are split on (unescaped, unquoted) whitespace.

However the parseQuotesAndEscapes function does allow customizing the escape characters; to avoid problems, on Windows, instead of backslash acting as escape, caret and grave will act as escape. (We check whether we're on Windows by checking path.sep, which might not be the proper way to do it, but makes sense for this case -- it's precisely when backslash is the path separator that we don't want it acting as escape!)

Finally, a word on error handling. The parseQuotesAndEscapes function will error if it finds an unmatched quote, or if the line ends with an escape character. However, erroring inside console-child.js doesn't produce useful results, so instead I added a redundant call to it from console.js which exists purely for validation purposes. Yes, this means we call it twice -- ideally we'd call it once, save the results, and pass those to console-child.js, but due to the weird way console-child.js starts up, I'm not sure that's possible. IDK, I didn't really try it; honestly, I'm not sure we really need to worry about that, I expect input strings to be pretty short.

Specifically:

1. Quotes are now handled.
2. Backslash-escaping works.
3. Environment variable substitution with $ works? That's not actually something I wanted, but the package I used to accomplish this, shell-quote, has no good way to turn this off, so whatever, let's go with it.

(I mean, there are ways to sort of turn it off, but no good ways that fully work. Like, I think the closest you could come would be passing x => "$" + x for the env argument. However, in that case, while $STUFF would remain as $STUFF as desired, ${STUFF} would get transformed into $STUFF. Which is kind of odd and warty. I figured, why not, instead of dealing with that, let's just allow evironment variable substitution, why not.)

Anyway, yeah, I just replaced the spot where we split the arguments with spaces with a call to shellQuote.parse, and it takes care of the rest. One note though -- shellQuote.parse will return an object when it encounters a bash operator so that you can handle those specially; that's very nice, but we don't want to handle those specially at Truffle console (because it would be quite a bit of work!), so I just converted those back to their underlying strings before passing them on.

Btw, there's one more spot in console.js where we split the arguments on spaces; however, I opted to leave that alone, as that's just for purposes of identifying the command. Notionally I could have replaced it with shellQuote.parse, but because the use of the code is limited, it seems to me that the existing code works fine. I did however add a comment noting this fact.

Testing instructions

I added a bunch of tests for parseQuotesAndEscapes. As for manual testing, here's one way, which is pretty much what I actually used:

1. Make a new directory and unbox metacoin there.
2. Rename test/metacoin.js to test/meta coin.js.
3. Open up truffle develop.
4. Type test test/meta\ coin.js. Observe that this works.

By comparison, you can try this on the develop branch and observe that you get an error.

[cds-amal]

Wow, that library leans into regular expressions! Windows uses\ as its path separator, and IIRC escapes spaces with the caret in Command shell and with the grave accent in PowerShell (or vice versa, it's been a while). Not a blocker, but I should probably test this on Windows.

[haltman-at]

Oh, yikes, good point about Windows! Backslash escaping would break things there.

Fortunately this problem is I think fixable; the shell-quote libraries allows customizing the escape character. So we can switch to a different escape character on Windows. (I dunno if there's any way to detect cmd.exe vs PowerShell, if that's even an appropriate thing to do.)

I'll go change that.

[eggplantzzz]

Seems like it works!

[cds-amal]

Do we have to worry about handling SHELL variables in truffle's REPL? I think it would be sufficient to handle quotes, which would eliminate the need for shell-quote.

[haltman-at]

Do we have to worry about handling SHELL variables in truffle's REPL?

I mean, this only lets the user do things they could do anyway, as I see it.

I think it would be sufficient to handle quotes, which would eliminate the need for shell-quote.

Well, once you have quotes, you have to have escaping, in case you want to put quotes in your quotes, so...

(Also, handling quotes can be a bit tricky!)

(Also, IMO, people should totally be able to escape spaces in the Truffle console.)

[haltman-at]

(Oops Amal, I accidentally edited your comment at first...)

[haltman-at]

Oh, hm, I also just noticed that this library also provides handling of shell comments. Notionally we could strip those, but I think I'll just not strip those I guess?

[cds-amal]

Looks ok, but I want to test it out on Windows, cmd and PowerShell.

[cds-amal]

I couldn't get this to work on Windows 11, cmd.exe shell. If I entered develop> test test\meta^ coin.js truffle test, errors, unable to locate chain.js. I'll revisit this in the morning and include output, as that will probably be helpful.

[cds-amal]

I couldn't get this to work on either Windows CMD or PowerShell then wound up testing shell-quote itself. The following snippet yielded the same breaking string behavior on both Windows shells, regardless of the escape string setting ("`" or "^").

const sq = require("shell-quote");
const test = `test^ string`;
const res = sq.parse(test, process.env, { escape:  '^'});
console.log(res);

PS C:\Users\cdssu\dev\shellQ> node .\index.js
[ 'test', 'string' ]

Edit: I don't think running it on different OS should matter, I suspect it would behave the same everywhere

[haltman-at]

Hm, that's odd -- it seems like it ought to work. I'll take a look?

Incidentally, it turns out that since I put up this PR, shell-quote released a new version. I'll update this PR to that version, I suppose, although it doesn't look to me like the changes should affect the escaping behavior.

[haltman-at]

Yeah something's definitely buggy here, though I'm having trouble figuring out why. Annoying. May file an issue.

Also: This thing has even more undocumented behavior, sometimes I have to check the pattern field, argh. I'll have to change that (assuming we are indeed sticking with this library).

There's also a bug in how it handles comments.

I might just change this PR to rolling our own handling after all...

[haltman-at]

OK, no, testing it out, the new release definitely did not fix the problem.

[haltman-at]

OK, I submitted a bunch of issues (1, 2, 3, 4) to the library maintainer; I think for now though I'm just going to tear this out and roll my own. Oh well!

[haltman-at]

OK, I've pushed a commit to replace the library with a hand-rolled function. Not quite ready for rereview yet, I think I'm definitely need to have to add tests for this function!

[haltman-at]

OK yes the error-handling here needs fixing at the least!

[haltman-at]

OK, updated this!

[cds-amal]

I'll be able to do a proper review tomorrow, but want to mention c-escape sequences, like \n, \r, and others. Like transforming \\n -> \n. Should we care about this, for a REPL?

[haltman-at]

Well, I'm basically trying to mimic Bash, and it doesn't do that. So to my mind the answer is no. Although yeah that does mean entering newlines isn't really possible. Oh well? 🤷

[cds-amal]

Ok, finally got around to testing this on Windows in CMD and PowerShell, using test\meta coin.js as the target file. It worked for both test test\meta^ coin.js (caret) and test test\meta`` coin.js (grave). Note: looks weird because I don't know how to escape the grave accent in GH markdown.

It is unclear to me why it works for the grave accent char.

[haltman-at]

I mean it works that way on Windows because I made it work that way; have you seen the changes to the code since you last reviewed it?

[cds-amal]

Oops, missed the grave accent char in the ternary expression!

[cds-amal]

I mean it works that way on Windows because I made it work that way; have you seen the changes to the code since you last reviewed it?

Oops, missed the little guy next to the caret.