22 ms·
The use of ls in this way is not good form: cd /root for project in $(ls go-cicd); I think a better expression would be: for project in ./* do [ -d "
by chasil 3y ago
The use of ls in this way is not good form:
cd /root
for project in $(ls go-cicd);
I think a better expression would be:
for project in ./*
do [ -d "$project" ] || continue
...
- adamtaylor_13 3y agoWhy is that not good form?
- GrumpySloth 3y agoIt omits some characters from its output. It also mangles some others through octal escape codes or other such stuff. Depending on flags it will also not handle filenames with spaces or newlines properly. ls output is meant for human consumption, not parsing.
- fellerts 3y agohttps://www.shellcheck.net/wiki/SC2045 https://www.shellcheck.net/wiki/SC2045
- throwaway458864 3y agoI was wondering if Shellcheck warned on this, and of course it does. God I love Shellcheck.
- riddley 3y agoNever, ever parse or rely on the output of ls. It's very unpredictable.
- g4zj 3y agoNot that I agree 100%, but the topic is covered fairly well here: https://mywiki.wooledge.org/ParsingLs https://mywiki.wooledge.org/ParsingLs
- mst 3y agohttps://mywiki.wooledge.org/BashPitfalls#for_f_in_.24.28ls_.2A.mp3.29 https://mywiki.wooledge.org/BashPitfalls#for_f_in_.24.28ls_.... describes the failure modes (and note that it's the very first pitfall because it's a common one - and one I've perpetrated plenty of times myself). Given the situation and the project names, I wouldn't expect the use of `ls` described in TFA to ever be a problem, but doing it with a simple glob would still be nicer and is a good habit to get into overall since then you don't have to ask yourself "is this use of ls going to be safe?"
- chasil 3y agoThere are several reasons. - Running ls forks an unnecessary process. - The ls may be aliased with "-F" (or -p) which will corrupt the filenames. - Environment variables may otherwise (unexpectedly) manipulate ls behavior. - Files with spaces will not be evaluated correctly. - Hostile files can be placed that mimic command line arguments. The shell should evaluate filenames itself; it is very capable of doing so. POSIX ls: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/ls.html https://pubs.opengroup.org/onlinepubs/9699919799/utilities/l...
- chasil 3y agoSome other nit-picks about this script. - Everything here is done as root. For the day that you want to build with lesser privilege, do this: BLDUSER=~root - That will be difficult for you, because you are moving the projects to /usr/local/bin; for the day that you stop running as root, make a subdirectory "/usr/local/bin/$BLDUSER" (owned by the namesake account) and move the projects there instead. - Very minor nitpick, use <<- and prefix tabs on the here document to make it slightly easier to read. - Slight improvement, so this can print more than one argument: println() { y= for x do printf %s%s "$y" "$x" y=' ' done >> /root/gocicd.log echo >> /root/gocicd.log # for the newline }