(#9400 P2a) Replace echo|tr/sed subshells with parameter expansion#9809
Open
iav wants to merge 10 commits into
Open
(#9400 P2a) Replace echo|tr/sed subshells with parameter expansion#9809iav wants to merge 10 commits into
iav wants to merge 10 commits into
Conversation
…sion
Replace 'for x in $(echo "${VAR}" | tr "," " ")' with the bash-native
'${VAR//,/ }' pattern, avoiding the subshell and pipe.
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
… parameter expansion
Replace 'for x in $(echo "${KERNEL_TARGET}" | tr "," "\\n")' with the
bash-native '${KERNEL_TARGET//,/ }' pattern, avoiding the subshell and pipe.
The for-loop word-splits on whitespace, so substituting commas with spaces
is equivalent to the original substitution with newlines.
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
…thmetic for-loop
Replace 'for i in $(seq 1 "${loops}")' with C-style 'for ((i = 1; i <= loops; i++))',
avoiding the seq(1) subshell. 'loops' is already 'declare -i' in both callers.
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
…expansion
Replace 'for x in $(tr "," " " <<< "${CLEAN_LEVEL}")' with the bash-native
'${CLEAN_LEVEL//,/ }' pattern, avoiding the subshell.
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
…arameter expansion
Replace 'for i in $(echo "${SERIALCON:-...}" | sed "s/,/ /g")' with the
bash-native '${var//,/ }' pattern, avoiding the subshell and pipe. Use a
local intermediate variable because parameter expansion does not nest cleanly
with the default-value operator.
Behavior is preserved as-is; the pre-existing 'ttyS0' literal-quotes default
is intentionally left untouched (out of scope for P2a).
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
…dsub with mapfile
Replace 'for dir in $(< /tmp/.overlayfs_wrapper_*)' — which uses unquoted
command substitution and relies on word-splitting — with 'mapfile -t arr < file'
followed by 'for dir in "${arr[@]}"'. The mapfile/array form preserves entries
verbatim (one path per line) and handles paths with whitespace correctly.
Part of the bash-safety cleanup in #9400 (P2a).
Assisted-by: Claude:claude-opus-4.7
Whitespace-only changes to bring the file in line with the project's shfmt config (space_redirects=true). Touched here because the file was modified in this branch; running shfmt on it would have produced these whitespace deltas anyway. Assisted-by: Claude:claude-opus-4.7
Whitespace-only changes to bring the file in line with the project's .editorconfig (tabs, indent-size 4). The file had drifted into 4-space indent in two functions. Touched here because the file was modified in this branch. Assisted-by: Claude:claude-opus-4.7
Whitespace-only changes to bring the file in line with the project's shfmt config (space_redirects=true, switch_case_indent=true, no space before ';'). Touched here because the file was modified in this branch. Assisted-by: Claude:claude-opus-4.7
Contributor
📝 WalkthroughWalkthroughThis PR refactors multiple shell scripts to replace external command pipelines and subshells with native Bash constructs: ChangesShell Syntax Modernization
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/functions/rootfs/distro-agnostic.sh`:
- Around line 509-510: The default value for SERIALCON currently includes
literal single quotes which become part of _serialcon_csv and break downstream
device names; change the assignment of local _serialcon_csv from
"${SERIALCON:-'ttyS0'}" to "${SERIALCON:-ttyS0}" (remove the single quotes) so
that when you split with ${_serialcon_csv//,/ } you get clean device names like
ttyS0 rather than 'ttyS0'; update the variable _serialcon_csv in the code where
it is declared to use the unquoted default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 74de7416-83c6-49c5-a198-749ae85d2345
📒 Files selected for processing (6)
lib/functions/compilation/utils-compilation.shlib/functions/configuration/interactive.shlib/functions/general/countdown.shlib/functions/general/extensions.shlib/functions/main/build-packages.shlib/functions/rootfs/distro-agnostic.sh
When SERIALCON is unset, the default 'ttyS0' (with embedded single quotes) was retained verbatim through the parameter expansion, so the loop variable became the 4-character string "'ttyS0'" rather than "ttyS0". Downstream that produces a malformed `/etc/securetty` entry and a systemd unit path `serial-getty@'ttyS0'.service` — neither matches a real device, and `systemctl daemon-reload`/`enable` fails silently. The original odd default predates this branch but was uncovered after the P2a rewrite of the loop header surfaced the value directly. Bug pointed out by coderabbitai on PR #9809. Assisted-by: Claude:claude-opus-4.7
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces 6 instances where a
forloop iterates over a comma-separated string by spawningecho … | tr/sed(ortr <<<) subshells. All replacements use bash-native parameter expansion${VAR//,/ }, plus one switch fromseq(1)to a C-style arithmetic loop and one switch from word-splitting$(< file)tomapfile. Part of the bash-safety cleanup tracked in #9400 (P2a).GitHub issue reference: #9400
Jira reference number AR-2818
Changes
lib/functions/general/extensions.shfor x in $(echo "${ENABLE_EXTENSIONS:-${EXT}}" | tr "," " ")${_enable_csv//,/ }lib/functions/configuration/interactive.shfor x in $(echo "${KERNEL_TARGET}" | tr "," "\n")for x in ${KERNEL_TARGET//,/ }lib/functions/general/countdown.sh(×2)for i in $(seq 1 "${loops}")for ((i = 1; i <= loops; i++))lib/functions/main/build-packages.shfor x in $(tr ',' ' ' <<< "${CLEAN_LEVEL}")for x in ${CLEAN_LEVEL//,/ }lib/functions/rootfs/distro-agnostic.shfor i in $(echo "${SERIALCON:-'ttyS0'}" | sed "s/,/ /g")${_serialcon_csv//,/ }; also drop the literal single quotes from the default (see follow-up commit below)lib/functions/compilation/utils-compilation.sh(×2)for dir in $(< /tmp/.overlayfs_wrapper_*)mapfile -t arr < file; for dir in "${arr[@]}"Why this matters
Each
echo "${var}" | tr/sedform forks a subshell and one or two external processes just to do what bash can do in-place with${var//pattern/replacement}. The replacements also reduce one class of word-splitting surprise: in themapfilecase, paths with whitespace are now preserved verbatim instead of being broken apart.Note on intentional word splitting
The replacements
for x in ${VAR//,/ }deliberately keep the expansion unquoted — that is the mechanism for the loop to split on whitespace. Inside[[ ]]or assignments these expansions would still need to be quoted; outside the loop header, they are not modified.Follow-up: SERIALCON default
A separate commit drops the literal single quotes from the
SERIALCON:-'ttyS0'default. WhenSERIALCONwas unset, the value'ttyS0'(with the quotes) was retained through the parameter expansion and downstreamarray[0]became the 4-character string'ttyS0'— producing a malformed/etc/securettyentry and a non-existentserial-getty@'ttyS0'.servicepath. Bug pointed out by coderabbitai on this PR.Out of scope (deferred)
bsp/armbian-bsp-cli-deb.sh:235(from the original P2a list in Proposal: Improve Bash syntax safety across the codebase #9400): the relevant block has been refactored away since the audit was written; no change needed.Style commits
Three commits in the middle of the chain are pure
shfmtwhitespace fixes on the three files this PR touches (extensions.sh,interactive.sh,distro-agnostic.sh) — keeping them on these files only, so the diff stays scoped. They contain no behavior changes.Test plan
bash lib/tools/shellcheck.sh— clean, no new warnings on the changed files (/tmp/.overlayfs_wrapper_*reads, comma-split loops, countdowns)shfmt— files in this PR conform to project configSummary by CodeRabbit