[pacman-dev] [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended

[Isaac Good]

On Sun, Nov 15, 2009 at 02:40:47PM +0000, Allan McRae wrote:
> I have looked at the two patches and the good news is that I can
> spot nothing wrong!  I just have a few comments about style that I
> would like to discuss.
> 
> 
> I would like to get some more consistency in quoting. e.g. here are
> some varied tests:
> 
> -	if [ "$ret" != '?' ]; then
> +	if [[ $ret != '?' ]]; then
> 
> +		if [[ $ret != "?" ]]; then
> +			if [[ $ret = y ]]; then
> 
> -		if [ "$cmd" = "bsdtar" ]; then
> +		if [[ $cmd = bsdtar ]]; then
> 
> -	if [ "$(check_option makeflags)" = "n" ]; then
> +	if [[ $(check_option makeflags) = "n" ]]; then
> 
> -	if [ "$arch" != 'any' ]; then
> +	if [[ $arch != 'any' ]]; then
> 
> -		if [ ${1:0:2} = '--' ]; then
> +		if [[ ${1:0:2} = '--' ]]; then
> 
> -			if [ -d ./src/$_hgrepo ] ; then
> +			if [[ -d ./src/$_hgrepo ]] ; then
> 
> -if [ -r ~/.makepkg.conf ]; then
> +if [[ -r ~/.makepkg.conf ]]; then
> 
> My suggestion is that any thing with text (i.e. not a pure variable)
> is quoted.  I know this is excessive in some cases (e.g. the last
> case) but the only exception I would be happy with is tests that are
> pure paths with only added "/" (e.g. $startdir/$file).  Even then,
> maybe quotes would be nicer...  I am happy to be debated on this.
 
That "n" and 'any' slipped my radar. I tried removing quotes for simple strings. ? looks like a bash pattern so I left it quotes. -- is also "different". 
 
> There should be quotes kept in the gettext calls in this test:
> -		if [ "$answer" = "$(gettext "YES")" -o "$answer" = "$(gettext
> "Y")" ]; then
> +		if [[ $answer = $(gettext YES) || $answer = $(gettext Y) ]]; then
 
Perhaps? Around the arguments passed to gettext or around the $( )?
 
> Why is EUID tested against 0 explicitly when all other tests for
> zero just use ! EUID? e.g.
> 
> -	if [ $EUID -eq 0 -a "$ASROOT" -eq 0 ]; then
> +	if (( EUID == 0 && ! ASROOT )); then
> 
> In fact, I quite like that things are explicitly tested in this case.
> 
> I wonder if the tests of return values should explicitly test for
> "== 0" or "!= 0". e.g. these test have become less clear to me when
> I read the code.
> 
> -	if [ $ret -gt 0 ]; then
> +	if (( ret )); then
> 
> -	elif [ $ret -ne 0 ]; then
> +	elif (( ret )); then
> 
> Note that these explicitly test for "== 0" or "> 0" and I think that
> is much clearer:
> 
> -	[ $# -gt 0 ] || return
> +	(( $# > 0 )) || return
> 
> -	[ $# -eq 0 ] && return $R_DEPS_SATISFIED
> +	(( $# == 0 )) && return $R_DEPS_SATISFIED
 
As mentioned earlier, most the variables tested are being used as booleans. Either TRUE or FALSE. Hence: if (( FLAG )) then the flag is set. Similar to C/Java/Perl/etc
The EUID and $# are not flags. They are integer values. So I left the test explicit.
 
 - Isaac