[ale] Anyone want to crtique a shell script?

[Leam Hall]

Comments in-line.

On 04/11/15 18:37, Alex Carver wrote:
> First, a lot of your statements are missing semicolons.  You'll need
> those or bash will complain.

Haven't seen it choke yet. Do you see any edge cases I'm not thinking about?


> Your exit for any unknown option should use a non-zero exit value.  Just
> add a value to the exit command:
>
> echo "Sorry, I don't understand $OPTARG."
> exit 1

Err...yeah. Good catch!


> On lines 61-64 it looks like you're trying to null out the file using a
> redirect but you don't use an input.  You may want to either explicitly
> cat /dev/null into the file or do an rm followed by a touch that way you
> are less likely to get any nasty surprises (like stdin suddenly showing
> up in OUTFILE).
>
> cat /dev/null > $OUTFILE
> rm $OUTFILE && touch $OUTFILE

While I think my original is valid, your suggestions are better because 
they are more readable. It's easy to miss a lone >.


> In the big for loop you could clean up a bit by using a temporary
> variable and then moving the -z check to a single statement (plus add a
> condition for a missing file which you don't have) and have only a
> single pair of cat statements:
>
> for file in install network services partitions packages pre post;
> do
> 	if [ -f "${file}.${RELEASE}.${SUFFIX}" ];
> 		INFILE=${file}.${RELEASE}.${SUFFIX};
> 	elif [ -f "${file}.${RELEASE}" ]
> 		INFILE=${file}.${RELEASE};
> 	elif [ -f "${file}" ]
> 		INFILE=${file};
> 	else
> 		#whoops, there was supposed to be a file here
> 		break; #get out of the loop before something blow up
> 		#alternatively abort with an error code
> 		#exit 10
> 	fi;
>
> 	if [ -z $OUTFILE ];
> 		cat "${INFILE}";
> 	else
> 		cat "${INFILE}" >> $OUTFILE;
> 	fi;
> done

Hmm... not bad. Let me think on this. I missed the idea that the entire 
file might be missing. All but the last couple are required.

> Lastly if you're worried about paths then append pwd to the outfile
> before using it instead of the ./ and also use basename to strip any
> paths (so someone can't be clever and use lots of ../../.. to grab a
> file they shouldn't)
>
> OUTFILE=`basename "${OUTFILE}"`
> OUTFILE="${PWD}/${OUTFILE}"

Good one, thanks!

Leam