Skip to content

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

Merged
merged 1 commit into from
Jun 5, 2025
Merged

Conversation

akoeplinger
Copy link
Member

Follow-up to #116315 (comment)

/cc @omajid

@Copilot Copilot AI review requested due to automatic review settings June 4, 2025 20:12
Copy link
Contributor

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

@Copilot Copilot AI left a 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[@]}"})
Copy link
Preview

Copilot AI Jun 4, 2025

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.

Suggested change
arguments=("-restore" "-build" ${arguments[@]+"${arguments[@]}"})
arguments=("-restore" "-build" "${arguments[@]}")

Copilot uses AI. Check for mistakes.

Copy link
Member

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.

Copy link
Member

@omajid omajid left a comment

Choose a reason for hiding this comment

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

Thanks!

@akoeplinger akoeplinger merged commit 624737e into dotnet:main Jun 5, 2025
150 of 152 checks passed
@akoeplinger akoeplinger deleted the fix-array-expansion branch June 5, 2025 08:09
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants