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 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.