(#9400 P3b,P3c) read -r and safe array assignment#9406
(#9400 P3b,P3c) read -r and safe array assignment#9406iav wants to merge 2 commits intoarmbian:mainfrom
Conversation
github-actions
bot
added
size/small
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefinements to two shell scripts: bootscript template rendering code reflowed in the BSP CLI, kernel upgrade freeze pin parsing switched to IFS/read for robust splitting, and the patch subject read now uses Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/functions/bsp/armbian-bsp-cli-deb.sh (1)
235-236: Good fix for glob-safe array assignment.Line 236 correctly prevents glob expansion for values containing
*or?.Note: Line 235 still uses unquoted
$KERNEL_UPGRADE_FREEZEin the$(echo $KERNEL_UPGRADE_FREEZE | sed ...)command substitution, which has the same glob expansion risk this PR aims to fix. Consider quoting it as"$KERNEL_UPGRADE_FREEZE"for consistency.Proposed fix
- for pin_variants in $(echo $KERNEL_UPGRADE_FREEZE | sed "s/,/ /g"); do + for pin_variants in $(echo "$KERNEL_UPGRADE_FREEZE" | sed "s/,/ /g"); do
| @@ -233,7 +233,7 @@ function compile_armbian-bsp-cli() { | |||
| # if freeze variable is removed, upgrade becomes possible again | |||
| if [[ "${BETA}" != "yes" ]]; then | |||
There was a problem hiding this comment.
@igorpecovnik @rpardini Question about the existing logic around this code:
if [[ "${BETA}" != "yes" ]]; then
for pin_variants in ...; do
...
done
else
touch "${destination}"/etc/apt/preferences.d/frozen-armbian
fiIs this BETA check intentional here? The current behavior:
BETA=yes(nightly) →touchcreates an emptyfrozen-armbian(no actual pinning)BETA=no(stable) → freeze logic runs, pins are writtenBETAunset (user-built) → freeze logic runs, butKERNEL_UPGRADE_FREEZEis typically empty so loop does nothing
The empty touch in the else branch for nightly builds seems like a no-op — dpkg-deb includes the 0-byte file, but it has no effect on apt.
Was this designed this way, or should the condition be something like [[ -n "${KERNEL_UPGRADE_FREEZE}" ]] instead of checking BETA?
There was a problem hiding this comment.
KERNEL_UPGRADE_FREEZE is a functionality that tells to which armbian version we allow upgrades. It is only used https://github.com/armbian/build/blob/main/config/boards/orangepi5pro.csc#L17 and also here we need to remove it as it is deprecated for sure. Not sure if we need this functionality at all ... I would say we drop this completly.
There was a problem hiding this comment.
Then... Should I delete this code instead of clearing it? Or should I not touch it in this PR?
|
Re: @coderabbitai nitpick about unquoted |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings used |
Other read calls in the same file already use -r. Without -r, backslashes in user input are interpreted as escape characters. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…rd splitting
Replace unquoted array assignment `arr=(${var})` with
`IFS=' ' read -ra arr <<< "${var}"` to prevent glob expansion.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
iav
force-pushed
the
fix/p3bc-read-and-array
branch
from
1c5c967 to
8dd2c10
Compare
2 participants
Summary
patching.sh: add missing-rflag toread -e -p— without it, backslashes in user input are interpreted as escape characters. Otherreadcalls in the same file already use-r.armbian-bsp-cli-deb.sh: replace unquotedarr=(${var})withIFS=' ' read -ra arr <<< "${var}"— prevents glob expansion on values containing*or?.Test plan
userpatch_createinteractive prompt still works correctly with backslash-containing inputKERNEL_UPGRADE_FREEZEvalues🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Refactor