(#9400 P2b) Replace echo|pipe subshells with here-strings/parameter expansion#9947
Open
iav wants to merge 11 commits into
Open
(#9400 P2b) Replace echo|pipe subshells with here-strings/parameter expansion#9947iav wants to merge 11 commits into
iav wants to merge 11 commits into
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactors multiple Armbian build library scripts to replace pipeline-based text processing with bash-native constructs: parameter expansion, IFS-based parsing, and here-strings. Preserves existing behavior while tightening hostname matching patterns and refactoring heredoc construction into command-substitution wrappers. ChangesUnified bash pattern modernization
Sequence Diagram(s)(No sequence diagrams generated: changes are primarily refactoring existing patterns with preserved behavior rather than introducing new multi-component interactions or control flow modifications.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested labels
Suggested reviewers
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 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
08
![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/host/docker.sh`:
- Line 213: The DOCKER_SERVER_NAME_HOST assignment is matching any "name:" token
(including "Username:"), so update the extraction to anchor to the literal
"Name:" field in DOCKER_INFO; modify the DOCKER_SERVER_NAME_HOST line that reads
from DOCKER_INFO to grep only lines that start (optionally after whitespace)
with "Name:" (case-insensitive if desired), then continue to cut -d ":" -f2 |
xargs echo -n as before so the Darwin case dispatch uses the correct host value.
🪄 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: 32a3c7a6-c7d8-4680-936b-8f9f8b7da937
📒 Files selected for processing (7)
lib/functions/compilation/uboot.shlib/functions/general/git-ref2info.shlib/functions/general/git.shlib/functions/general/hash-files.shlib/functions/general/python-tools.shlib/functions/host/docker.shlib/functions/image/partitioning.sh
Contributor
✅ Action performedReview finished.
|
… parameter expansion
Replace `echo $root_partition | sed 's/\/dev\///g'` with the bash-native
`${root_partition#/dev/}` prefix-trim. Removes a subshell plus two external
processes (echo, sed) on every u-boot update path.
The variable `root_partition_name` is assigned but unused both before and
after this change; cleaning that up is out of scope for P2b.
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
Re-indent the `python_proxy_env` array elements with one extra tab so the file no longer drifts when shellfmt is run scoped on it. No behavior change. Pure preparation for the following P2b commit; carried in the same PR so the functional change diff is not contaminated with formatting noise. Assisted-by: Claude:claude-opus-4.7
…ll with parameter expansion
Replace
$(echo "${python3_version_full}" | awk '{print $2}' | cut -d. -f1,2)
with two bash-native parameter expansions: `${python3_version_full#* }`
strips the "Python " prefix, then `${_py_ver_triplet%.*}` drops the patch
component. Same observable output (e.g. "3.12"), one subshell and three
external processes (echo, awk, cut) eliminated.
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
…h parameter expansion
Split the sfdisk version (e.g. "2.41.1") directly with `IFS=. read` and
format the numeric form (e.g. "024101") with `printf -v`, eliminating
the `echo | awk` subshell. Same observable output.
Each component is then stripped of any non-numeric suffix (e.g.
"2.41-rc1" -> minor "41", not "41-rc1") before the printf so we don't
bomb under `set -e` on prerelease util-linux builds; the prior
`awk -F. '{printf "%d%02d%02d"}'` quietly accepted the numeric prefix,
and we keep that tolerance here.
Spotted by @chatgpt-codex-connector on PR #161 (iav/armbian fork pre-review).
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
shellfmt collapses the extra spaces before the inline `# -> libfoo` and `# -> libfoo.path` comments to its single-space rule. No behavior change. Pure preparation for the following P2b commit; carried in the same PR so the functional change diff is not contaminated with formatting noise. Assisted-by: Claude:claude-opus-4.7
…x-trim Replace the `echo "$url" | sed "s|^https://github.com/|${GITHUB_SOURCE}/|"` form with a guarded bash-native prefix trim. The guard preserves the sed-with-^ semantics: only URLs literally starting with the GitHub host are rewritten; anything else (private mirrors, gitlab, etc.) is left alone. Eliminates one subshell + the sed external on every fetch_from_repo call. Part of armbian#9400 (P2b). Assisted-by: Claude:claude-opus-4.7
…ith IFS=/ read
Replace three identical `$(echo "${git_source}" | cut -d/ -f4-5)` subshells
(in the gitverse.ru, gitee.com and github.com mirror branches) with bash-
native `IFS=/ read -r _ _ _ _gr_org _gr_repo _ <<< "${git_source}"`. Same
output: skip three leading components (scheme, empty, host), capture org
and repo, drop the rest.
Eliminates three echo+cut subshells per ref-resolution call.
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
…lls with here-strings
Replace three `echo "${var}" | sha256sum | cut -d' ' -f1` forms with
`sha256sum <<< "${var}" | cut -d' ' -f1`. Verified empirically that both
forms emit identical bytes (one trailing newline in both cases, same hash);
the form-equivalence is also the reason this group was originally excluded
from the armbian#9400 P2b list — that exclusion has now been removed from armbian#9400
because the trailing-newline concern does not apply (echo without -n and
the here-string redirection both append a single \n).
Eliminates one subshell + one external `echo` invocation per call.
For `function_bodies` the in-array `[@]` splat is swapped for `[*]` so
the array is joined into a single here-string token (IFS default = space).
Empirically equivalent to the prior `echo "${arr[@]}"` output.
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
fix/p2b-echo-pipe-subshells
branch
from
127c794 to
6000c81
Compare
Apply shellfmt's space-after-redirect (`2> /dev/null`), C-style for-loop operator spacing (`((i = 2; i < ...))`), and collapse a stray double blank line. Heredoc bodies (the `cat <<- 'EOT'` wrapper/cron scripts) are left untouched on purpose: shfmt re-indents them, which survives only because `<<-` strips leading tabs; it bloats the diff for no functional gain, so it is kept out of scope. No behavior change. Assisted-by: Claude:claude-opus-4.8
…th here-strings
Seven near-identical `echo "${DOCKER_INFO}" | grep -i -e <key>: | cut -d : -f 2 | xargs echo -n`
chains parse fields out of cached `docker info` output. Replace each with
`grep -i -e <key>: <<< "${DOCKER_INFO}" | cut -d : -f 2 | xargs echo -n`.
The here-string and `echo "${DOCKER_INFO}"` produce the same bytes on
stdin (one trailing newline), so grep, cut and xargs see identical input
and emit the same field value. The change drops one subshell + one echo
process per invocation; `get_docker_info_once` runs these in a tight
sequence at the top of every Docker-mode build.
Part of armbian#9400 (P2b).
Assisted-by: Claude:claude-opus-4.7
`docker info` emits both `Username: <user>` (under `Server:`) and `Name: <host>` near the bottom; the previous grep pattern `-i -e name:` caught both, so on hosts where docker stores credentials we ended up with `DOCKER_SERVER_NAME_HOST="<user> <host>"` after `cut + xargs` glued the two matches together. The Darwin `case` dispatch downstream then failed to recognise "Docker Desktop" / "Rancher Desktop", silently turning off the loop-hack workarounds. Anchor the grep to a leading-whitespace + literal `Name:` and take only the first match (`-m1`). Pre-existing on main, surfaced during P2b review by @coderabbitai. Assisted-by: Claude:claude-opus-4.7
iav
force-pushed
the
fix/p2b-echo-pipe-subshells
branch
from
6000c81 to
649c954
Compare
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
Closes the last open group from #9400 (the Bash syntax safety cleanup) — P2b:
seventeen
echo "$var" | grep/sed/cut/awk/sha256sum …chains across sevenfiles, replaced with bash-native here-strings or parameter expansions. Each
chain forked a subshell and one or two external processes just to do
something bash can do in-place; behavior is preserved verbatim.
GitHub issue reference: #9400
Changes
lib/functions/compilation/uboot.shecho $root_partition | sed 's/\/dev\///g'"${root_partition#/dev/}"lib/functions/general/python-tools.sh$(echo … | awk '{print $2}' | cut -d. -f1,2)${var#* }then${_%.*})lib/functions/image/partitioning.sh$(echo "$ver" | awk -F. …)IFS=. read+printf -v(with non-numeric suffix strip so prerelease util-linux builds don't bomb)lib/functions/general/git.sh$(echo "$url" | sed "s|^https://github.com/|…")[[ "${url}" == https://github.com/* ]]+${url#…})lib/functions/general/git-ref2info.sh$(echo "${git_source}" | cut -d/ -f4-5)(×3)IFS=/ read -r _ _ _ org repo _lib/functions/general/hash-files.sh$(echo "${x}" | sha256sum | cut -d' ' -f1)(×3)$(sha256sum <<< "${x}" | cut -d' ' -f1)lib/functions/host/docker.sh$(echo "${DOCKER_INFO}" | grep -i -e "K:" | cut -d : -f 2 | xargs echo -n)(×7)$(grep -i -e "K:" <<< "${DOCKER_INFO}" | …)Why this matters
Each removed
echo \| …chain saves one subshell and one external processper call.
get_docker_info_oncealone runs seven of these in a tightsequence at the top of every Docker-mode build;
hash-filesones run onevery artifact hash computation. The overall wall-clock benefit is modest,
but the diff value is largely about readability and not pretending shell
needs
awk/sed/echofor trivial string surgery.Note on
hash-files.sh(was excluded in #9400, now included)The original #9400 P2b excluded
echo "${x}" | sha256sum, on the assumptionthat the
<<<here-string alternative would append a different trailingnewline and so change the hash. Empirically that is not the case: both
echo "$x" | sha256sumandsha256sum <<< "$x"emit one trailing\nand produce identical hashes — only
printf '%s' "$x" | sha256sum(nonewline) differs. The exclusion has been removed from #9400 accordingly,
and the three
hash-files.shsites are converted here.Preparatory style commits
Three
style(shellfmt): …commits are carried in this PR ahead of thefunctional changes they touch. They are pure shellfmt runs over the same
file: re-indented
python_proxy_envarray elements, collapsed commentalignment in a
git.shsubmodule loop, and a tree-wide format ofdocker.sh(which had drifted significantly from shellfmt's rules). Theyare split out so each P2b commit is purely functional.
Out of scope (deliberately)
[[ -z $var ]]quoting (issue Proposal: Improve Bash syntax safety across the codebase #9400 excludes this —[[ ]]alreadyprevents word splitting; pure style preference).
evalmachinery in extension / artifact code (architectural,excluded by Proposal: Improve Bash syntax safety across the codebase #9400).
root_partition_nameassignment inuboot.sh(dead-codecleanup belongs in a separate change).
Test plan
bash lib/tools/shellcheck.shover the whole tree — clean.echo "$x" | sha256sumvssha256sum <<< "$x"verified empirically with three sample inputs.
compile.shDocker build (to exercise thedocker.shget_docker_info_oncefield reads).Summary by CodeRabbit
sfdiskversion strings.