FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > ArchLinux > ArchLinux Pacman Development

 
 
LinkBack Thread Tools
 
Old 11-15-2009, 03:55 AM
Allan McRae
 
Default Changing [ to [[ and (( - Part Two - Amended

Isaac Good wrote:

From e3e5bb93cb2b33315a8d6eff8956aa5b6c74d28d Mon Sep 17 00:00:00 2001

From: Isaac Good <pacman@isaac.otherinbox.com>
Date: Thu, 12 Nov 2009 15:09:05 -0500
Subject: [PATCH 2/2] Changing [ to [[ and (( - Part Two - Amended

Second half of makepkg
This replaces any prior patches of mine
Includes stuff like -o to || and -a to && etc
if [ $(type .. were preserved due to a bash bug with [[ and set -e and ERR trap

Signed-off-by: Isaac Good <pacman@isaac.otherinbox.com>


<snip>

I have pushed these two patches and Cedric's one for the other scripts
to the "bashtest" branch in my repo [1]. I am going through these now
and will get back with any queries.


Allan

[1] http://projects.archlinux.org/users/allan/pacman.git/log/?h=bashtest
 
Old 11-15-2009, 01:40 PM
Allan McRae
 
Default Changing [ to [[ and (( - Part Two - Amended

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.




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



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



Allan
 
Old 11-15-2009, 03:26 PM
Isaac Good
 
Default Changing [ to [[ and (( - Part Two - Amended

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
 
Old 11-18-2009, 09:50 PM
Cedric Staniewski
 
Default Changing [ to [[ and (( - Part Two - Amended

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.
>
> [...]
>
> 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.
>

To quote myself [1]:
> In my opinion, it is often not obvious whether they are required or not
> which is why I tended to use more quotes than actually were needed. I am
> fairly familiar with quoting by now and can use both "quoting styles",
> but I think it would probably be a good idea to decide for one. Using
> just as much quotes as required would be a little bit shorter, but using
> quotes `where possible` may be a little bit more foolproof.

Meaning that I am fine with quoting strings generally.

[1]
http://mailman.archlinux.org/pipermail/pacman-dev/2009-November/010115.html


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

Yep, sounds reasonable, especially since we already explicitly test all
(or at least many) other non-boolean variables.
 

Thread Tools




All times are GMT. The time now is 11:22 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org