On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
> I hope your patch do not get lost in this thread.
Erm. Yeah, I hope so too. How does it get threaded?
>
> > @@ -345,26 +345,26 @@ handle_deps() {
> > local R_DEPS_SATISFIED=0
> > local R_DEPS_MISSING=1
> >
> > - [ $# -eq 0 ] && return $R_DEPS_SATISFIED
> > + (( $# == 0 )) && return $R_DEPS_SATISFIED
> >
>
> Is there a reason why you do not use (( ! $# )) here?
Some variables are used as a boolean flag in which case testing if ((VAR)) and
if (( ! VAR )) reads well. The $# (and EUID) are used as integer values so it
struck me as more understandable or readable in this form.
> > @@ -450,12 +450,12 @@ download_sources() {
> > for netfile in "${sour...@]}"; do
> > local file=$(get_filename "$netfile")
> > local url=$(get_url "$netfile")
> > - if [ -f "$startdir/$file" ]; then
> > + if [[ -f "$startdir/$file" ]]; then
> > msg2 "$(gettext "Found %s in build dir")" "$file"
> > rm -f "$srcdir/$file"
> > ln -s "$startdir/$file" "$srcdir/"
> > continue
> > - elif [ -f "$SRCDEST/$file" ]; then
> > + elif [[ -f "$SRCDEST/$file" ]]; then
> > msg2 "$(gettext "Using cached copy of %s")" "$file"
> > rm -f "$srcdir/$file"
> > ln -s "$SRCDEST/$file" "$srcdir/"
>
> We could remove the quotes here, too.
I was lax with some of the quotes when doing concatenation and parameter
expansion...
> > @@ -512,7 +512,7 @@ generate_checksums() {
> >
> > local i=0;
> > local indent=''
> > - while [ $i -lt $((${#integ}+6)) ]; do
> > + while [[ $i -lt $((${#integ}+6)) ]]; do
> > indent="$indent "
> > i=$(($i+1))
> > done
>
> What about "while (( $i < $((${#integ}+6)) )); do"?
I was planning a separate patch switching to a for loop.
for (( i = 0; i < ${#integ} + 6; i++ ))
> > @@ -521,8 +521,8 @@ generate_checksums() {
> > for netfile in "${sour...@]}"; do
> > local file="$(get_filename "$netfile")"
> >
> > - if [ ! -f "$file" ] ; then
> > - if [ ! -f "$SRCDEST/$file" ] ; then
> > + if [[ ! -f $file ]] ; then
> > + if [[ ! -f "$SRCDEST/$file" ]] ; then
> > error "$(gettext "Unable to find source
> > file %s to generate checksum.")" "$file"
> > plain "$(gettext "Aborting...")"
> > exit 1
>
> Same as above. Quotes are not necessary.
>
> > @@ -533,10 +533,10 @@ generate_checksums() {
> >
> > local sum="$(openssl dgst -${integ} "$file")"
> > sum=${sum##* }
> > - [ $ct -gt 0 ] && echo -n "$indent"
> > + (( ct )) && echo -n $indent
> > echo -n "'$sum'"
>
> I do not think we want to remove the quotes here.
>
> $ indent=" "
> $ echo -n $indent; echo asdf
> asdf
> $ echo -n "$indent"; echo asdf
> asdf
> $
That'd be a careless use of s/"// in a macro on my behalf. Thanks for catching.
> > @@ -566,8 +566,8 @@ check_checksums() {
> > file="$(get_filename "$file")"
> > echo -n " $file ... " >&2
> >
> > - if [ ! -f "$file" ] ; then
> > - if [ ! -f "$SRCDEST/$file" ] ; then
> > + if [[ ! -f $file ]] ; then
> > + if [[ ! -f "$SRCDEST/$file" ]] ; then
> > echo "$(gettext "NOT FOUND")"
> > >&2
> > errors=1
> > found=0
> > @@ -622,8 +622,8 @@ extract_sources() {
> > continue
> > fi
> >
> > - if [ ! -f "$file" ] ; then
> > - if [ ! -f "$SRCDEST/$file" ] ; then
> > + if [[ ! -f $file ]] ; then
> > + if [[ ! -f "$SRCDEST/$file" ]] ; then
> > error "$(gettext "Unable to find source file %s
> > for extraction.")" "$file"
> > plain "$(gettext "Aborting...")"
> > exit 1
>
> Quotes are not necessary.
>
> > @@ -662,31 +662,31 @@ extract_sources() {
> > ...
> > fi
> > done
> >
> > - if [ $EUID -eq 0 ]; then
> > + if (( EUID == 0 )); then
> > # change perms of all source files to root user & root group
> > chown -R 0:0 "$srcdir"
> > fi
> > }
> >
> > ...
>
> (( ! EUID )) ?
As above
>
> > @@ -713,12 +713,12 @@ run_function() {
> > ...
> > local i=1
> > while true; do
> > - if [ -f "$BUILDLOG.$i" ]; then
> > + if [[ -f "$BUILDLOG.$i" ]]; then
> > i=$(($i +1))
> > else
> > break
>
> Quotes again? There are several other places where quotes or the dollar
> signs (in e.g. (( $found )) ) are not needed. Maybe you want to remove
> them, maybe not.
>
> Cedric
>
>
Should I implement these changes and resend this patch?
/me goes looking for git help
Thanks for the feedback!
- Isaac
