diff mbox series

[2/4] base-files/functions.sh: do not iterate over ls

Message ID 20200920053140.35255-2-rosenp@gmail.com
State Superseded
Headers show
Series [1/4] base-files/functions.sh: read grep output by line | expand

Commit Message

Rosen Penev Sept. 20, 2020, 5:31 a.m. UTC
From https://github.com/koalaman/shellcheck/wiki/SC2045 :

When looping over a set of files, it's always better to use globs when
possible. Using command expansion causes word splitting and glob
expansion, which will cause problems for certain filenames (typically
first seen when trying to process a file with spaces in the name).

Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 package/base-files/files/lib/functions.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

edgar.soldin@web.de Sept. 20, 2020, 10:12 a.m. UTC | #1
On 20.09.2020 07:31, Rosen Penev wrote:
> -	for file in $(ls $1/*.sh 2>/dev/null); do
> -		. $file
> +	for file in "$1"/*.sh; do
> +		[ -e "$file" ] || break
> +		. "$file"

the existence check is probably meant to catch "../*.sh" entry found if no files match. as files will likely not vanish during the loop this will probably work.
still it'll break the loop completely without any need. using 'continue' instead of 'break' would not break the loop and be safer here.

..ede
Rosen Penev Sept. 26, 2020, 2:22 a.m. UTC | #2
On Sun, Sep 20, 2020 at 3:15 AM <edgar.soldin@web.de> wrote:
>
> On 20.09.2020 07:31, Rosen Penev wrote:
> > -     for file in $(ls $1/*.sh 2>/dev/null); do
> > -             . $file
> > +     for file in "$1"/*.sh; do
> > +             [ -e "$file" ] || break
> > +             . "$file"
>
> the existence check is probably meant to catch "../*.sh" entry found if no files match. as files will likely not vanish during the loop this will probably work.
> still it'll break the loop completely without any need. using 'continue' instead of 'break' would not break the loop and be safer here.
I was confused on this.
https://github.com/koalaman/shellcheck/wiki/SC2045 lists break.
http://mywiki.wooledge.org/BashPitfalls#for_f_in_.24.28ls_.2A.mp3.29
lists continue. I'll make the change.
>
> ..ede
>
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
diff mbox series

Patch

diff --git a/package/base-files/files/lib/functions.sh b/package/base-files/files/lib/functions.sh
index 59bb0fe711..0f56387244 100755
--- a/package/base-files/files/lib/functions.sh
+++ b/package/base-files/files/lib/functions.sh
@@ -290,8 +290,9 @@  default_postinst() {
 include() {
 	local file
 
-	for file in $(ls $1/*.sh 2>/dev/null); do
-		. $file
+	for file in "$1"/*.sh; do
+		[ -e "$file" ] || break
+		. "$file"
 	done
 }