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

[Dan McGee]

On Sun, Nov 15, 2009 at 8:40 AM, Allan McRae <allan at archlinux.org> 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.
>
>
>
> 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

As a side note, in the pacman C code we try to stick to a convention
where if testing for 0 makes sense, do it explicitly. This comes up
often in the case of strcmp() == 0, which means "these two strings
match". e.g.
    if(strcmp(pkgname, name) == 0) {
instead of
    if(!strcmp(pkgname, name)) {

I like the logic here of doing explicit declarations of 0 in the EUID
case. For a true boolean in the other cases, I'm fine with dropping
the zero check.

-Dan