On Wed, Jun 11, 2014 at 5:08 AM, Elias Persson delreich@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@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 theloop
<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.