On Wed, Jun 11, 2014 at 5:08 AM, Elias Persson <delreich at takeit.se> wrote: > Maybe I'm too sleep deprived to make sense of what I'm reading, > but... > > On 2014-06-10 16:12, Karanbir Singh wrote: > > On 06/10/2014 03:08 PM, Pat Riehecky wrote: > >> From: Pat Riehecky <riehecky at fnal.gov> > <snip> > >> +##################################################################### > >> +# setup args in the right order for making getopt evaluation > >> +# nice and easy. You'll need to read the manpages for more info > >> +args=$(getopt -o hr -- "$@") > >> +if [[ $? -ne 0 ]]; then > >> + usage > >> +fi > >> +eval set -- "$args" > >> + > >> +RHELTAG=0 > >> +for arg in $@; do > > Shouldn't this be quoted (or just `for arg;`)? > Otherwise, why not just use `$args` and skip the `eval set ...`? > Agreed. "$@" to preserve the argv array. Granted, I don't think the script is likely to run into any such cases, but still good practice. An unquoted $@ is a little silly anyway -- that's what $* is for. > >> + case $1 in > >> + -- ) > >> + # end of getopt args, shift off the -- and get out of the > loop > <snip> > >> + exit 1 > >> +fi > >> + > >> +msg=$(git log --pretty=format:"%s") > >> +pkg=$(echo ${msg} | cut -d' ' -f2) > > More missing quoting? > > > Well here it's actually arguable. You don't need quotes around a single command substitution in an assignment. Not that they'd hurt anything As for ${msg} vs "${msg}", it's just a matter of collapsing whitespace. In fact, unquoted is beneficial if there happens to be a stray space in the commit message. # msg="import foo-1-1" # pkg=$(echo ${msg} | cut -d' ' -f2) # printf "%q\n" "$pkg" foo-1-1 # pkg=$(echo "${msg}" | cut -d' ' -f2) # printf "%q\n" "$pkg" '' Personally, I'd probably just let bash do the splitting. E.g. # set -- $msg # pkg="$2" # printf "%q\n" "$pkg" foo-1-1 > >> + > >> +if [[ ${RHELTAG} -eq 0 ]]; then > >> + thispkg=(echo ${pkg} | head -1) > >> +elif [[ ${RHELTAG} -eq 1 ]]; then > >> + thispkg=(echo ${pkg} grep -v centos | head -1) > > In addition to the already mentioned missing $:s, there should > presumably be a pipe before that `grep`, and unless that `head` > is redundant, `${pkg}` should be quoted. > (Unless there's some portability issue I'm missing, isn't > `grep -v -m 1 -e centos` equivalent?) > > Yes, the pipe is missing and ${pkg} should be quoted. The head was not originally redundant, but one of my patches made it so (and inadvertently broke the script's -r option). Actually, about to post a new patch which will address the quoting and also allow the script to work even if the repo is cloned to a differently named directory. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.centos.org/pipermail/centos-devel/attachments/20140611/3640c37b/attachment-0007.html>