-
Notifications
You must be signed in to change notification settings - Fork 669
Fix SSH command escaping #633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not sufficient, as it doesn't deal with commands that include quotes. E.g. it breaks:
$ lima bash -c 'echo "foo bar"'
foo bar
With the current patch is breaks like this:
$ lima bash -c 'echo "foo bar"'
bar': -c: line 1: unexpected EOF while looking for matching `''
bar': -c: line 2: syntax error: unexpected end of file
All right, should be good now. Thanks @jandubois! As an aside, both of these examples would be good tests to add for the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but since you volunteered to add tests (😄 ), maybe add them to this PR?
lol, all right, done. I confirmed that the new tests currently pass, and each one also fails as expected with the two previous versions of shell.go. Edit: Fixed the tests (initially wrote them as unit tests by mistake). |
d90b8e1
to
3198780
Compare
Thanks, but please sign the commit for DCO (run |
Signed-off-by: Ryan Lester <[email protected]>
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Addresses #632.
I haven't tested very thoroughly, and may be making some incorrect assumptions about how shellescape works, but I can confirm that this at least fixes the
lima bash -c "$(echo -e '\n\techo balls\n')"
example for me.