Skip to content
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

Quote the File Path #105

Closed
wants to merge 1 commit into from
Closed

Quote the File Path #105

wants to merge 1 commit into from

Conversation

toddsundsted
Copy link

Quote the file path so that spaces in the path don't break the command passed to the shell.

Quote the file path so that spaces in the path don't break the command
passed to the shell.
@jwoertink
Copy link
Collaborator

Do you have an example where this would fail? Can you add it to https://github.com/crystal-community/icr/blob/master/spec/integration/icr_spec.cr ?

@toddsundsted
Copy link
Author

you can reproduce reliably by creating a directory with white space in the name, changing to that directory, invoking icr and trying to run anything. it don't immediately see how to reproduce that in an integration test without manipulating the filesystem in the test, but i'll see if i can cook one up. on my laptop:

$ cd /tmp
$ mkdir "dir with spaces"
$ cd dir\ with\ spaces/
$ icr
...
icr(0.27.2) > Hash(String).new
Error: unknown command: /tmp/dir

@@ -20,7 +20,7 @@ module Icr
File.write(@tmp_file_path, @command_stack.to_code)
io_out = IO::Memory.new
io_error = IO::Memory.new
command = "#{CRYSTAL_COMMAND} #{@tmp_file_path} --no-debug"
command = "#{CRYSTAL_COMMAND} \"#{@tmp_file_path}\" --no-debug"
status = Process.run(command, shell: true, output: io_out, error: io_error)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be even better to not rely on shell splitting mechanism, and properly give the command and args separately since we can:

Process.new(CRYSTAL_COMMAND, {@tmp_file_path, "--no-debug"}, output: io_out, error: io_out)

@toddsundsted
Copy link
Author

created #106 using Process.new

will close this PR if that approach is successful

@veelenga
Copy link
Member

veelenga commented Nov 4, 2019

Closing in favor of #113

@veelenga veelenga closed this Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants