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
fix: Quote commands correctly in plugin-test #1078
Changes from 2 commits
add6df8
d126735
dcf3004
9a9e846
d261eed
37690ed
e48db3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,12 +4,10 @@ plugin_test_command() { | |
|
||
local plugin_name=$1 | ||
local plugin_url=$2 | ||
local plugin_command_array=() | ||
local plugin_command | ||
local plugin_command=() | ||
local plugin_gitref="master" | ||
local tool_version | ||
# shellcheck disable=SC2086 | ||
set -- ${*:3} | ||
shift 2 | ||
|
||
while [[ $# -gt 0 ]]; do | ||
case $1 in | ||
|
@@ -24,13 +22,15 @@ plugin_test_command() { | |
shift # past value | ||
;; | ||
*) | ||
plugin_command_array+=("$1") # save it in an array for later | ||
shift # past argument | ||
plugin_command+=("$1") # save it in an array for later | ||
shift # past argument | ||
;; | ||
esac | ||
done | ||
|
||
plugin_command="${plugin_command_array[*]}" | ||
if [ "${#plugin_command[@]}" -eq 1 ]; then | ||
plugin_command=(sh -c "${plugin_command[@]}") | ||
fi | ||
|
||
local exit_code | ||
local TEST_DIR | ||
|
@@ -128,11 +128,11 @@ plugin_test_command() { | |
fail_test "could not reshim plugin" | ||
fi | ||
|
||
if [ -n "$plugin_command" ]; then | ||
$plugin_command | ||
if [ "${#plugin_command[@]}" -gt 0 ]; then | ||
"${plugin_command[@]}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While we are currently using Bash, our goal is to take the POSIX compat soln where possible to support more Shells in future. Are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the code, I think it would not be too hard in this case to refactor so it does not use arrays at all, would you like me to do that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raxod502 which do you think would be better? I only see one line that actually uses this variable as an array (line 25 pushes an element to the array). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, if you want to make asdf POSIX compliant in the long run, we might as well get rid of the array usage, since you'll have to do that at some point. I pushed a new commit doing so, and fixing some other quoting errors. |
||
exit_code=$? | ||
if [ $exit_code != 0 ]; then | ||
fail_test "$plugin_command failed with exit code $?" | ||
fail_test "${plugin_command[*]} failed with exit code $?" | ||
fi | ||
fi | ||
|
||
|
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.
Should this be
sh
orbash
?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.
Pushed a commit making it default to
$SHELL
with fallback to POSIX-safesh
.