Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Minor fixes to pass shellcheck #14996

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+3 −3
Open

Minor fixes to pass shellcheck #14996

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1353ab7
base-files: Avoid re-splitting in firstboot script
miervasi-edalab Mar 27, 2024
bf49979
base-files: Fix potential issues in board_detect script
miervasi-edalab Mar 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions package/base-files/files/bin/board_detect
View file
Open in desktop
Expand Up @@ -5,8 +5,8 @@ CFG=$1
[ -n "$CFG" ] || CFG=/etc/board.json

[ -d "/etc/board.d/" -a ! -s "$CFG" ] && {
for a in $(ls /etc/board.d/*); do
Copy link
Contributor

Choose a reason for hiding this comment

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

While I agree with this change, is there any reason why someone would want to do ls <some glob>? Probably this is done because ls guarantees that they are sorted which makes sense in this context.

Copy link
Contributor

@rany2 rany2 Mar 27, 2024 •
edited

Choose a reason for hiding this comment

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

P.S. it does seem like echo /etc/board.d/* is sorted, but the ls man page explicitly mentions that the output would be sorted by default. I'm not sure if we can rely on it for glob.

Copy link
Contributor

Choose a reason for hiding this comment

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

The commit message for when this was introduced didn't shed any light on this. I think @rany2 almost certainly has the right of it though. $(ls /etc/board.d/*) is used since it gives the files in sorted order. A favorite approach is to name the scripts 0-first-script.sh, 1-second-script.sh, 2-third-script.sh, etc in order to ensure ordering of the scripts. Same idea as patch filenames starting with 3 digits. As a result this first line is almost certainly wrong.

[ -s $a ] || continue;
for a in /etc/board.d/*; do
Copy link
Contributor

Choose a reason for hiding this comment

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

I would be awfully tempted to substitute find /etc/board.d -maxdepth 1 -print | sort | while read a; do as this avoids having the shell scan the directory.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which would not yield equivalent results though, as it returns /etc/board.d at depth 0 as well, so it should probably be -mindepth 1 -maxdepth 1.

While generally preferring find for finding operations, the line gets pretty unhandy here and would also depend on different flavors of find and sort (BusyBox, findutils, coreutils) installed on the target. (though they all should behave the same in this case). I'd therefore slightly prefer the shorter glob version here.

[ -s "$a" ] || continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though it is unlikely for files in /etc/board.d/ to have spaces in their filenames, this is quite appropriate.

$(. $a)
done
}
Expand Down
2 changes: 1 addition & 1 deletion package/base-files/files/sbin/firstboot
View file
Open in desktop
@@ -1,3 +1,3 @@
#!/bin/sh

/sbin/jffs2reset $@
/sbin/jffs2reset "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

First commit looks simple and reasonable. The subject line (first line used by shortlog) should be prefixed with "base-files:".