[CentOS-devel] [PATCH] Added script for extracting what I think is the dist tag

[Mike McLean]

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