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

kaleidoscope-builder may choke on white space #299

Closed
kingparra opened this issue Feb 21, 2018 · 2 comments
Closed

kaleidoscope-builder may choke on white space #299

kingparra opened this issue Feb 21, 2018 · 2 comments
Assignees

Comments

@kingparra
Copy link
Contributor

kingparra commented Feb 21, 2018

There are a lot of unquoted parameter expansions in Kaleidoscope/bin/kaleidoscope-builder. Running ShellCheck (a static analysis tool for shell scripts) on it produces a litany of errors related to word splitting.

This is bad because If a path used in this script has spaces in it, the shell will split the path into multiple arguments, which in turn will probably cause it to fail. Worse yet, it may just continue silently doing the wrong thing.

I'd like to humbly suggest that you run shellcheck against this file and correct the quoting problems. (Also, consulting BashPitfalls.) Changing the shabang to #!/usr/bin/env bash makes sense to me as well, since the script already uses bashisms and bash can be found on both mac and Linux. You don't gain any compatibility by invoking this script through sh.

I would re-write it and submit a pull request, but am pretty green and don't want to break anything. I hope this issue is helpful, and not just me nitpicking about a minor detail that doesn't matter.

ps. Command substitution "$( command )" in bash opens up a new quoting context, so arguments within it should be quoted, too. E.g. echo "$(cat "${path_prefix}"/"$(uname)")" is valid, working bash. Most of the errors seem to be a result of not quoting arguments within the new context on the assumption that the outer quotes will prevent word splitting.

@kingparra kingparra changed the title kaleidoscope-builder may choke on white space in paths kaleidoscope-builder may choke on white space Feb 21, 2018
@kingparra
Copy link
Contributor Author

kingparra commented Feb 21, 2018

Just an update. I'm working on a pull request related to this issue, and will link to it once it's up. Although, no promises -- I'm still learning how to use git. Edit: Working on the issue here: https://github.com/kingparra/Kaleidoscope

@algernon
Copy link
Contributor

algernon commented Mar 9, 2018

Fixed by #305.

@algernon algernon closed this as completed Mar 9, 2018
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

No branches or pull requests

2 participants