[pacman-dev] [PATCH 1/2] Changing [ to [[ and ((

[Isaac Good]

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 "${source[@]}"; 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 "${source[@]}"; 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