[ale] Anyone want to crtique a shell script?

[Leam Hall]

Scott, thanks!

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.

Leam

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