Make shell script POSIX-compliant #1628
+11
−14
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,7 @@ | ||
| #!/bin/sh | ||
|
|
||
| # get_matched_files finds file for image extending | ||
| get_matched_files() { | ||
| custom_dir="$1" | ||
| default_dir="$2" | ||
| files_matched="$3" | ||
|
|
@@ -13,19 +12,17 @@ function get_matched_files() { | |
| # process_extending_files process extending files in $1 and $2 directories | ||
| # - source all *.sh files | ||
| # (if there are files with same name source only file from $1) | ||
| process_extending_files() { | ||
| custom_dir=$1 | ||
| default_dir=$2 | ||
| get_matched_files "$custom_dir" "$default_dir" '*.sh' | sort -u | while read -r filename; do | ||
| if [ "$filename" ]; then | ||
| echo "=> sourcing $filename ..." | ||
| if [ -f "$custom_dir/$filename" ]; then | ||
| . "$custom_dir/$filename" | ||
| elif [ -f "$default_dir/$filename" ]; then | ||
| . "$default_dir/$filename" | ||
| fi | ||
| fi | ||
| done | ||
|
Comment on lines
+18
to
+27
There was a problem hiding this comment. 💡 Verification agent ❓ Verification inconclusiveSub-shell created by pipeline may surprise future modifications Piping into Two POSIX-safe alternatives that avoid the subshell: # 1. Redirect loop body (preferred)
while IFS= read -r filename; do
...
done <<EOF
$(get_matched_files "$custom_dir" "$default_dir" '*.sh' | sort -u)
EOF
# 2. Use a temporary file
tmp=$(mktemp) || exit 1
get_matched_files "$custom_dir" "$default_dir" '*.sh' | sort -u >"$tmp"
while IFS= read -r filename; do
...
done <"$tmp"
rm -f "$tmp"Consider refactoring now while context is fresh to prevent future bugs. Avoid subshell in pipeline-driven loops Piping into Please refactor the loop in to one of these POSIX-compliant patterns:
Refactoring now will safeguard future additions that rely on variables set inside this loop. 🧰 Tools🪛 Shellcheck (0.10.0)[warning] 22-22: ShellCheck can't follow non-constant source. Use a directive to specify location. (SC1090) [warning] 24-24: ShellCheck can't follow non-constant source. Use a directive to specify location. (SC1090) 🤖 Prompt for AI AgentsThere was a problem hiding this comment. @coderabbitai what about changing the #! to read /bin/bash, and then it would not be necessary to worry about POSIX compatibility. There was a problem hiding this comment. @jiridanek, that's an excellent architectural point! Changing to However, this creates a trade-off with the current PR objectives: Advantages of switching to bash:
Advantages of maintaining POSIX compliance:
Given that this PR was specifically created to address issue #1271 requesting POSIX compliance for portability, switching to bash would fundamentally change the scope. Question: Should we pivot this PR to use bash for simplicity, or do you want to maintain the POSIX compliance goal for portability reasons? If we switch to bash, we could revert most of the POSIX changes and use more robust bash constructs instead. What's your priority here - maximum portability or implementation simplicity? 🧠 Learnings usedThere was a problem hiding this comment. We do have bash available in all our docker images, so it makes sense to me to make use of it There was a problem hiding this comment. @jiridanek Perfect! That's a much cleaner architectural solution. Since bash is guaranteed available in your Docker images, switching to This would mean:
Should we:
Given your preference for architectural solutions, option 1 would address the root cause immediately rather than layering POSIX workarounds on top. ✏️ Learnings added
🧠 Learnings usedThere was a problem hiding this comment. @jiridanek what are your final thoughts on this? Are we going with POSIX-compliant or bash? Please let me know so I can make changes accordingly. There was a problem hiding this comment.
|
||
| } | ||
Oops, something went wrong.
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.
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.
Quote positional-parameter assignments to preserve spaces
custom_dir=$1anddefault_dir=$2perform parameter expansion without quotes, so word-splitting and pathname expansion occur if either parameter contains whitespace, glob characters, or IFS bytes. That silently breaks valid directory paths such as/data/My Notebooks.🤖 Prompt for AI Agents