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

Use Bash arrays for argument lists in shell scripts #10877

Merged
merged 2 commits into from
Feb 8, 2021

Conversation

cameel
Copy link
Member

@cameel cameel commented Feb 2, 2021

Depends on #10585.

This PR fixes shellcheck warnings about missing quotes in cases where they can contain spaces and are unquoted on purpose (to rely on Bash word splitting behavior). The solution is to use Bash arrays.

@cameel cameel self-assigned this Feb 2, 2021
…DEBUGGER

- There are ways to fix the warning properly but they're all less readable than this in my opinion.
@cameel cameel force-pushed the bash-arrays-for-arg-variables branch from 37b0f80 to cba6e68 Compare February 2, 2021 14:47
Base automatically changed from fix-quoting-and-whitespace-in-shell-scripts to develop February 3, 2021 10:27
@cameel
Copy link
Member Author

cameel commented Feb 3, 2021

I think I can mark this as ready for review without extra manual testing because all the modified scripts in this PR do run in CI.

@cameel cameel marked this pull request as ready for review February 3, 2021 11:05
Copy link
Member

@hrkrshnn hrkrshnn left a comment

Choose a reason for hiding this comment

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

Looks good. I wonder if it's worth all these efforts to write this in bash :)

if [ -n "$log_directory" ]
then
if [ -n "$optimize" ]
then
log=--logger=JUNIT,error,$log_directory/opt_$vm.xml
log+=("--logger=JUNIT,error,$log_directory/opt_$vm.xml")
Copy link
Member

Choose a reason for hiding this comment

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

= should work here as well, right?

Just asking, doesn't need any changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, for not getting back to this one earlier :)

Yeah, it would work and it would match the original code better but I think that += is more appropriate here. If log were initialized with a non-empty array, we'd likely want to add to it rather than ovewrite it.

Copy link
Member

Choose a reason for hiding this comment

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

No worries!

@chriseth chriseth merged commit c82c218 into develop Feb 8, 2021
@chriseth chriseth deleted the bash-arrays-for-arg-variables branch February 8, 2021 12:31
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.

4 participants