<div dir="ltr"><div>I thought I knew bash pretty well, but I've picked up some valuable hints from this discussion.<br><br></div>-- CHS<br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 15, 2015 at 6:44 AM, Leam Hall <span dir="ltr"><<a href="mailto:leamhall@gmail.com" target="_blank">leamhall@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Scott, thanks!<br>
<br>
Several folks have provided critique, and if I hadn't been ill for the past week or so much of their input would already be integrated. As is I can mostly survive work but not much else. Feel free to provide notes as late as you want; I'm more the "get it right" than "leave it alone" type.<span class="HOEnZb"><font color="#888888"><br>
<br>
Leam</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
On 04/14/15 16:46, Scott Plante wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I'll take a whack at it. Sorry for the delay, I wanted to wait until I had time to really have a good look.<br>
<br>
One thing that's often overlooked in shell scripts is that you can redirect the output<br>
of a block of statements at once. I often see stuff like<br>
echo a > filename<br>
echo b >> filename<br>
echo c >> filename<br>
instead of<br>
{<br>
echo a<br>
echo b<br>
echo c<br>
} > filename<br>
The latter also has the advantage of only opening the file once, instead of 3 (or x) times. It may not<br>
seem more concise & readable here (2 extra lines!) but in practice it usually is. Or especially in cases<br>
like yours where the block (for loop) is already there.<br>
<br>
Also, for simple "if" conditionals where there isn't both a true and false statement,<br>
and especially where there's only a single statement to execute conditionally, you can<br>
use the handy && and || connectors. && means execute the next statement if the previous<br>
returned 0 exit code (true/no error), and || when non-zero/false.<br>
<br>
e.g. [ -f "$file" ] && true > "$file"<br>
<br>
And for short sanity checks, I often do this kind of thing even though it's two statements:<br>
[ -f "$reqfile" ] ||<br>
{ echo "$reqfile is required" 1>&2; exit 1; }<br>
or in your case:<br>
[ -z "$RELEASE" ] &&<br>
{ echo "Please provide a release number." 1>&2; exit 2; }<br>
Somebody mentioned about the non-zero exit codes already. Also, the 1>&2 sends the<br>
message to std err, which would be the convention.<br>
<br>
An aside:<br>
I often use the simple<br>
> $file<br>
and I've seen<br>
: > $file"<br>
Note ":" is the null statement. In my e.g. I used "true" as it's concise and emits no output<br>
but is unlikely to be overlooked. If you think cat /dev/null is more readable then that's<br>
fine, but I don't know if I'd say any are very much better than the others. I think the cat /dev/null<br>
is mostly more readable if you're used to seeing it and not used to seeing the others, but there<br>
are no absolute answers in stylistic questions.<br>
<br>
I'm a bit unsure what's meant to be in default, but it seems like the "-r RELEASE" option is not meant<br>
to be optional. If that's true, you shouldn't have it in square brackets in your help message. But maybe<br>
you mean it's optional if it's in the default?<br>
<br>
I'm not a big getopts user so I'm going to assume that's all working correctly. I have a vague memory that<br>
it had some kind of notation for which options were required, but I could be confused.<br>
<br>
There is a handy little dev file /dev/stdout you can use for standard out when you need a file placeholder.<br>
I don't see exactly in your script where OUTFILE could be empty, so I just added a check before the "for file in":<br>
<br>
So you could make the last block a lot more concise like this:<br>
<br>
[ -z "${OUTFILE}" ] && OUTFILE=/dev/stdout<br>
for file in install network services partitions packages pre post<br>
do<br>
if [ -f "${file}.${RELEASE}.${SUFFIX}" ]<br>
cat "${file}.${RELEASE}.${SUFFIX}"<br>
elif [ -f "${file}.${RELEASE}" ]<br>
cat "${file}.${RELEASE}"<br>
else<br>
cat "${file}"<br>
fi<br>
done > "${OUTFILE}"<br>
<br>
In Linux we don't often get files/directories with white space in them, but I always try to account<br>
for the possibility. Hence the added double quotes around "$OUTFILE" etc.<br>
<br>
Ok I lied--not always--typing all those quotes does get old. But usually at least if it's meant to be a<br>
long-lived script and/or other people will use it. I have the same problem w/ the curly braces on<br>
variables. At least with them, it's not dependent on the value of the variable--if it works without the<br>
curlies, it'll always work.<br>
<br>
One minor stylistic/convention point. I notice you sometimes use all caps variable names and sometimes<br>
lower case. I was taught that environment variables were all-caps to differentiate them from the ones<br>
in your scripts. So I believe OUTFILE, RELEASE, etc. should all be lowercase. The first shell variables<br>
people learn about are environment variables so they seem to think all shell variable should be caps,<br>
and this old convention is widely ignored or unknown, or maybe I was taught wrong :-P get off my lawn!<br>
<br>
FWIW, I've written many shell scripts over the years and never known a missing line-end semicolon to<br>
cause problems. Well at least if you actually end the line. You need them if you want to type<br>
{ cmd-a; cmd-b; }<br>
instead of<br>
{<br>
cmd-a<br>
cmd-b<br>
}<br>
Not to say there isn't some edge case, but I'd be very curious to see it. Then again,<br>
I sometimes add the superfluous semis out of habit from other languages.<br>
<br>
Scott<br>
<br>
p.s. One last thought occurred to me. After fixing whatever typos, you could try something like this:<br>
<br>
for prefix in install network services partitions packages pre post<br>
do<br>
for file in "${prefix}.${RELEASE}.${<u></u>SUFFIX}" "${prefix}.${RELEASE}" "${prefix}"<br>
do<br>
[ -f "$file" ] && { cat "${file}"; continue 2; }<br>
done<br>
done > "${OUTFILE}"<br>
<br>
<br>
----- Original Message -----<br>
<br>
From: "Leam Hall" <<a href="mailto:leamhall@gmail.com" target="_blank">leamhall@gmail.com</a>><br>
To: "Atlanta Linux Enthusiasts" <<a href="mailto:ale@ale.org" target="_blank">ale@ale.org</a>><br>
Sent: Saturday, April 11, 2015 10:22:59 AM<br>
Subject: [ale] Anyone want to crtique a shell script?<br>
<br>
Could use critique on a shell script that writes Kickstart files. You<br>
don't have to understand Kickstart to critique, the output files are<br>
plain text in a certain order.<br>
<br>
<a href="https://github.com/LeamHall/SecComFrame/tree/master/tools/ks_writer" target="_blank">https://github.com/LeamHall/<u></u>SecComFrame/tree/master/tools/<u></u>ks_writer</a><br>
<br>
The script is ks_writer.sh and the other files there are what gets<br>
pulled in and what an output might look like.<br>
<br>
Thanks!<br>
<br>
Leam<br>
______________________________<u></u>_________________<br>
Ale mailing list<br>
<a href="mailto:Ale@ale.org" target="_blank">Ale@ale.org</a><br>
<a href="http://mail.ale.org/mailman/listinfo/ale" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo/ale</a><br>
See JOBS, ANNOUNCE and SCHOOLS lists at<br>
<a href="http://mail.ale.org/mailman/listinfo" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo</a><br>
______________________________<u></u>_________________<br>
Ale mailing list<br>
<a href="mailto:Ale@ale.org" target="_blank">Ale@ale.org</a><br>
<a href="http://mail.ale.org/mailman/listinfo/ale" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo/ale</a><br>
See JOBS, ANNOUNCE and SCHOOLS lists at<br>
<a href="http://mail.ale.org/mailman/listinfo" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo</a><br>
<br>
</blockquote>
______________________________<u></u>_________________<br>
Ale mailing list<br>
<a href="mailto:Ale@ale.org" target="_blank">Ale@ale.org</a><br>
<a href="http://mail.ale.org/mailman/listinfo/ale" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo/ale</a><br>
See JOBS, ANNOUNCE and SCHOOLS lists at<br>
<a href="http://mail.ale.org/mailman/listinfo" target="_blank">http://mail.ale.org/mailman/<u></u>listinfo</a><br>
</div></div></blockquote></div><br></div>