<div dir="ltr">On Wed, Jun 11, 2014 at 5:08 AM, Elias Persson <span dir="ltr"><<a href="mailto:delreich@takeit.se" target="_blank">delreich@takeit.se</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Maybe I'm too sleep deprived to make sense of what I'm reading,<br>
but...<br>
<div class=""><br>
On 2014-06-10 16:12, Karanbir Singh wrote:<br>
> On 06/10/2014 03:08 PM, Pat Riehecky wrote:<br>
>> From: Pat Riehecky <<a href="mailto:riehecky@fnal.gov">riehecky@fnal.gov</a>><br>
</div><snip><br>
<div class="">>> +#####################################################################<br>
>> +# setup args in the right order for making getopt evaluation<br>
>> +# nice and easy. You'll need to read the manpages for more info<br>
>> +args=$(getopt -o hr -- "$@")<br>
>> +if [[ $? -ne 0 ]]; then<br>
>> + usage<br>
>> +fi<br>
>> +eval set -- "$args"<br>
>> +<br>
>> +RHELTAG=0<br>
>> +for arg in $@; do<br>
<br>
</div>Shouldn't this be quoted (or just `for arg;`)?<br>
Otherwise, why not just use `$args` and skip the `eval set ...`?<br></blockquote><div><br></div><div>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.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="">
>> + case $1 in<br>
>> + -- )<br>
>> + # end of getopt args, shift off the -- and get out of the loop<br>
</div><snip><br>
<div class="">>> + exit 1<br>
>> +fi<br>
>> +<br>
>> +msg=$(git log --pretty=format:"%s")<br>
>> +pkg=$(echo ${msg} | cut -d' ' -f2)<br>
<br>
</div>More missing quoting?<br>
<div class=""><br>
<br></div></blockquote><div><br></div><div>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<br><br></div><div>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.<br>
# msg="import foo-1-1"<br># pkg=$(echo ${msg} | cut -d' ' -f2)<br># printf "%q\n" "$pkg"<br>foo-1-1<br># pkg=$(echo "${msg}" | cut -d' ' -f2)<br># printf "%q\n" "$pkg"<br>
''<br><br></div><div>Personally, I'd probably just let bash do the splitting. E.g.<br># set -- $msg<br># pkg="$2"<br># printf "%q\n" "$pkg"<br>foo-1-1<br><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="">
>> +<br>
>> +if [[ ${RHELTAG} -eq 0 ]]; then<br>
>> + thispkg=(echo ${pkg} | head -1)<br>
>> +elif [[ ${RHELTAG} -eq 1 ]]; then<br>
>> + thispkg=(echo ${pkg} grep -v centos | head -1)<br>
<br>
</div>In addition to the already mentioned missing $:s, there should<br>
presumably be a pipe before that `grep`, and unless that `head`<br>
is redundant, `${pkg}` should be quoted.<br>
(Unless there's some portability issue I'm missing, isn't<br>
`grep -v -m 1 -e centos` equivalent?)<br>
<div class="im"><br></div></blockquote><div><br></div><div>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).<br>
</div><div> <br></div><div>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.<br></div></div></div></div>