[ale] Anyone want to crtique a shell script?

Wed Apr 15 09:16:23 EDT 2015

I thought I knew bash pretty well, but I've picked up some valuable hints
from this discussion.

-- CHS

On Wed, Apr 15, 2015 at 6:44 AM, Leam Hall <leamhall at gmail.com> wrote:

> 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
>>
>>  _______________________________________________
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.ale.org/pipermail/ale/attachments/20150415/e0e09fd7/attachment.html>