-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Fix bash array expansion #116321
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 bash array expansion #116321
Conversation
Follow-up to dotnet#116315 (comment)
Tagging subscribers to this area: @dotnet/runtime-infrastructure |
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.
Pull Request Overview
This PR updates the Bash arguments
array handling to avoid introducing an empty string element and ensures proper expansion when the array is empty.
- Initialize
arguments
as an empty array instead of containing a lone empty string - Use a parameter expansion trick to safely append existing arguments only when present
@@ -549,7 +549,7 @@ while [[ $# > 0 ]]; do | |||
done | |||
|
|||
if [ ${#actInt[@]} -eq 0 ]; then | |||
arguments=("-restore" "-build" "${arguments[@]}") | |||
arguments=("-restore" "-build" ${arguments[@]+"${arguments[@]}"}) |
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.
[nitpick] The ${arguments[@]+"${arguments[@]}"}
expansion is more complex than needed in Bash; using a quoted "${arguments[@]}"
will safely expand existing elements and gracefully handle empty arrays.
arguments=("-restore" "-build" ${arguments[@]+"${arguments[@]}"}) | |
arguments=("-restore" "-build" "${arguments[@]}") |
Copilot uses AI. Check for mistakes.
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.
No, it doesn't with the version of bash on macOS.
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.
Thanks!
Follow-up to #116315 (comment)
/cc @omajid