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.

Sign up for GitHub

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

Jump to bottom

fix: Quote commands correctly in plugin-test #1078

Merged
merged 7 commits into from Nov 2, 2021
Merged

fix: Quote commands correctly in plugin-test #1078

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
add6df8
fix: Quote commands correctly in plugin-test
raxod502 Oct 24, 2021
d126735
fix ci
raxod502 Oct 25, 2021
dcf3004
make it posix compliant
raxod502 Oct 31, 2021
9a9e846
default to $SHELL
raxod502 Oct 31, 2021
d261eed
clarity
raxod502 Oct 31, 2021
37690ed
cleanup
raxod502 Oct 31, 2021
e48db3c
shfmt
raxod502 Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 8 additions & 8 deletions lib/commands/command-plugin-test.bash
View file
Open in desktop
Expand Up @@ -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
Expand All @@ -24,13 +22,15 @@ plugin_test_command() {
shift # past value
;;
*)
plugin_command_array+=("$1") # save it in an array for later
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
Expand Down Expand Up @@ -129,10 +129,10 @@ plugin_test_command() {
fi

if [ -n "$plugin_command" ]; then
$plugin_command
"${plugin_command[@]}"
Copy link
Contributor

Choose a reason for hiding this comment

The 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 shift and [@] POSIX compatible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

shift is POSIX per this ref. Unfortunately, POSIX sh does not have arrays at all, so you'll have to do a more thorough rearchitecting in order to make this code portable. Applying this PR won't decrease the portability since all shells with arrays should have [@] indexing (otherwise they're not much use, since [*] doesn't handle arguments with spaces correctly).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Expand Down