[PATCH] scripts: replace test builtin [ with shell keywords [[ and ((

[Cedric Staniewski]

FS#16623 suggested this change for makepkg; this patch applies it to the
remaining files in the scripts directory.

Signed-off-by: Cedric Staniewski <cedric@...>
---
 scripts/pacman-optimize.sh.in |   22 ++++++------
 scripts/pkgdelta.sh.in        |   22 ++++++------
 scripts/rankmirrors.sh.in     |   12 +++---
 scripts/repo-add.sh.in        |   74 ++++++++++++++++++++--------------------
 4 files changed, 65 insertions(+), 65 deletions(-)

diff --git a/scripts/pacman-optimize.sh.in b/scripts/pacman-optimize.sh.in
index e43e946..d9f1ca2 100644
--- a/scripts/pacman-optimize.sh.in
+++ b/scripts/pacman-optimize.sh.in
@@ -74,23 +74,23 @@ die_r() {
 # PROGRAM START
 
 # determine whether we have gettext; make it a no-op if we do not
-if [ ! $(type -t gettext) ]; then
+if ! type gettext &>/dev/null; then
  gettext() {
  echo "$@"
  }
 fi
 
-if [ "$1" = "-h" -o "$1" = "--help" ]; then
+if [[ $1 = "-h" || $1 = "--help" ]]; then
  usage
  exit 0
 fi
 
-if [ "$1" = "-V" -o "$1" = "--version" ]; then
+if [[ $1 = "-V" || $1 = "--version" ]]; then
  version
  exit 0
 fi
 
-if [ "$1" != "" ]; then
+if [[ -n $1 ]]; then
  dbroot="$1"
 fi
 
@@ -99,11 +99,11 @@ if ! type diff >/dev/null 2>&1; then
  die "$(gettext "diff tool was not found, please install diffutils.")"
 fi
 
-if [ ! -d "$dbroot" ]; then
+if [[ ! -d $dbroot ]]; then
  die "$(gettext "%s does not exist or is not a directory.")" "$dbroot"
 fi
 
-if [ ! -w "$dbroot" ]; then
+if [[ ! -w $dbroot ]]; then
  die "$(gettext "You must have correct permissions to optimize the database.")"
 fi
 
@@ -113,7 +113,7 @@ dbroot="${dbroot%/}"
 lockfile="${dbroot}/db.lck"
 
 # make sure pacman isn't running
-if [ -f "$lockfile" ]; then
+if [[ -f $lockfile ]]; then
  die "$(gettext "Pacman lock file was found. Cannot run while pacman is running.")"
 fi
 # do not let pacman run while we do this
@@ -130,7 +130,7 @@ find "$dbroot" -type f | sort | xargs md5sum > "$workdir/pacsums.old"
 msg "$(gettext "Tar'ing up %s...")" "$dbroot"
 cd "$dbroot"
 bsdtar -czf "$workdir/pacman-db.tar.gz" ./
-if [ $? -ne 0 ]; then
+if (( $? )); then
  rm -rf "$workdir"
  die_r "$(gettext "Tar'ing up %s failed.")" "$dbroot"
 fi
@@ -139,7 +139,7 @@ fi
 msg "$(gettext "Making and MD5sum'ing the new database...")"
 mkdir "$dbroot.new"
 bsdtar -xpf "$workdir/pacman-db.tar.gz" -C "$dbroot.new"
-if [ $? -ne 0 ]; then
+if (( $? )); then
        rm -rf "$workdir"
        die_r "$(gettext "Untar'ing %s failed.")" "$dbroot"
 fi
@@ -152,7 +152,7 @@ find "$dbroot.new" -type f | sort | \
 # step 4: compare the sums
 msg "$(gettext "Checking integrity...")"
 diff "$workdir/pacsums.old" "$workdir/pacsums.new" >/dev/null 2>&1
-if [ $? -ne 0 ]; then
+if (( $? )); then
  # failed
  # leave our pacman-optimize tmpdir for checking to see what doesn't match up
  rm -rf "$dbroot.new"
@@ -167,7 +167,7 @@ mv "$dbroot" "$dbroot.old" || fail=1
 mv "$dbroot.new" "$dbroot" || fail=1
 chmod --reference="$dbroot.old" "$dbroot" || fail=1
 chown --reference="$dbroot.old" "$dbroot" || fail=1
-if [ $fail -ne 0 ]; then
+if (( fail )); then
  # failure with our directory shuffle
  die_r "$(gettext "New database substitution failed. Check for $dbroot,\n$dbroot.old, and $dbroot.new directories.")"
 fi
diff --git a/scripts/pkgdelta.sh.in b/scripts/pkgdelta.sh.in
index 588dc49..1550fa1 100644
--- a/scripts/pkgdelta.sh.in
+++ b/scripts/pkgdelta.sh.in
@@ -35,7 +35,7 @@ QUIET=0
 umask 0022
 
 msg() {
- [ $QUIET -ne 0 ] && return
+ (( QUIET )) && return
  local mesg=$1; shift
  printf "==> ${mesg}\n" "$@" >&1
 }
@@ -79,7 +79,7 @@ read_pkginfo()
  for line in $(bsdtar -xOf "$1" .PKGINFO 2>/dev/null |
  grep -v "^#" | sed 's|\(\w*\)\s*=\s*\(.*\)|\1="\2"|'); do
  eval "$line"
- if [ -n "$pkgname" -a -n "$pkgver" -a -n "$arch" ]; then
+ if [[ -n $pkgname && -n $pkgver && -n $arch ]]; then
  IFS=$OLDIFS
  return 0
  fi
@@ -108,17 +108,17 @@ create_xdelta()
  newver="$pkgver"
  newarch="$arch"
 
- if [ "$oldname" != "$newname" ]; then
+ if [[ $oldname != $newname ]]; then
  error "$(gettext "The package names don't match : '%s' and '%s'")" "$oldname" "$newname"
  return 1
  fi
 
- if [ "$oldarch" != "$newarch" ]; then
+ if [[ $oldarch != $newarch ]]; then
  error "$(gettext "The package architectures don't match : '%s' and '%s'")" "$oldarch" "$newarch"
  return 1
  fi
 
- if [ "$oldver" == "$newver" ]; then
+ if [[ $oldver == $newver ]]; then
  error "$(gettext "Both packages have the same version : '%s'")" "$newver"
  return 1
  fi
@@ -128,12 +128,12 @@ create_xdelta()
  local ret=0
 
  xdelta3 -q -f -s "$oldfile" "$newfile" "$deltafile" || ret=$?
- if [ $ret -ne 0 ]; then
+ if (( ret )); then
  error "$(gettext "Delta could not be created.")"
  return 1
  else
  msg "$(gettext "Generated delta : '%s'")" "$deltafile"
- [ $QUIET -eq 1 ] && echo "$deltafile"
+ (( QUIET )) && echo "$deltafile"
  fi
  return 0
 }
@@ -142,22 +142,22 @@ case "$1" in
  -q|--quiet) QUIET=1; shift ;;
 esac
 
-if [ $# -ne 2 ]; then
+if (( $# != 2 )); then
  usage
  exit 0
 fi
 
-if [ ! -f "$1" ]; then
+if [[ ! -f $1 ]]; then
  error "$(gettext "File '%s' does not exist")" "$1"
  exit 0
 fi
 
-if [ ! -f "$2" ]; then
+if [[ ! -f $2 ]]; then
  error "$(gettext "File '%s' does not exist")" "$2"
  exit 0
 fi
 
-if [ ! "$(type -p xdelta3)" ]; then
+if ! type xdelta3 &>/dev/null; then
  error "$(gettext "Cannot find the xdelta3 binary! Is xdelta3 installed?")"
  exit 1
 fi
diff --git a/scripts/rankmirrors.sh.in b/scripts/rankmirrors.sh.in
index 015b39a..d5cbadd 100644
--- a/scripts/rankmirrors.sh.in
+++ b/scripts/rankmirrors.sh.in
@@ -56,8 +56,8 @@ err() {
 # returns the fetching time, or timeout, or unreachable
 gettime() {
  IFS=' ' output=( $(curl -s -m 10 -w "%{time_total} %{http_code}" "$1" -o/dev/null) )
- [[ $? = 28 ]] && echo timeout && return
- [[ ${output[1]} -ge 400 || ${output[1]} -eq 0 ]] && echo unreachable && return
+ (( $? == 28 )) && echo timeout && return
+ (( ${output[1]} >= 400 || ! ${output[1]} )) && echo unreachable && return
  echo "${output[0]}"
 }
 
@@ -96,13 +96,13 @@ finaloutput() {
  for line in "${sortedarray[@]}"; do
  echo "${line#* } : ${line% *}"
  ((numiterator++))
- [[ $NUM -ne 0 && $numiterator -ge $NUM ]] && break
+ (( NUM && numiterator >= NUM )) && break
  done
  else
  for line in "${sortedarray[@]}"; do
  echo "Server = ${line#* }"
  ((numiterator++))
- [[ $NUM -ne 0 && $numiterator -ge $NUM ]] && break
+ (( NUM && numiterator >= NUM )) && break
  done
  fi
  exit 0
@@ -112,7 +112,7 @@ finaloutput() {
 # Argument parsing
 [[ $1 ]] || usage
 while [[ $1 ]]; do
- if [[ "${1:0:2}" = -- ]]; then
+ if [[ ${1:0:2} = -- ]]; then
  case "${1:2}" in
  help) usage ;;
  version) version ;;
@@ -142,7 +142,7 @@ while [[ $1 ]]; do
  done
  shift $snum
  fi
- elif [[ -f "$1" ]]; then
+ elif [[ -f $1 ]]; then
  FILE="1"
  IFS=$'\n' linearray=( $(<$1) )
  [[ $linearray ]] || err "File is empty."
diff --git a/scripts/repo-add.sh.in b/scripts/repo-add.sh.in
index 3f00441..0e46c37 100644
--- a/scripts/repo-add.sh.in
+++ b/scripts/repo-add.sh.in
@@ -42,7 +42,7 @@ msg() {
 }
 
 msg2() {
- [ $QUIET -ne 0 ] && return
+ (( QUIET )) && return
  local mesg=$1; shift
  printf "  -> ${mesg}\n" "$@" >&1
 }
@@ -90,7 +90,7 @@ There is NO WARRANTY, to the extent permitted by law.\n")"
 # arg2 - List
 # arg3 - File to write to
 write_list_entry() {
- if [ -n "$2" ]; then
+ if [[ -n $2 ]]; then
  echo "%$1%" >>$3
  echo -e $2 >>$3
  fi
@@ -102,7 +102,7 @@ find_pkgentry()
  local pkgentry
  for pkgentry in $tmpdir/$pkgname*; do
  name=${pkgentry##*/}
- if [ "${name%-*-*}" = "$pkgname" ]; then
+ if [[ ${name%-*-*} = $pkgname ]]; then
  echo $pkgentry
  return 0
  fi
@@ -126,12 +126,12 @@ db_write_delta()
  pkgname="$(get_delta_pkgname $deltafile)"
 
  pkgentry=$(find_pkgentry $pkgname)
- if [ -z "$pkgentry" ]; then
+ if [[ -z $pkgentry ]]; then
  return 1
  fi
  deltas="$pkgentry/deltas"
  # create deltas file if it does not already exist
- if [ ! -f "$deltas" ]; then
+ if [[ ! -f $deltas ]]; then
  msg2 "$(gettext "Creating 'deltas' db entry...")"
  echo -e "%DELTAS%" >>$deltas
  fi
@@ -162,11 +162,11 @@ db_remove_delta()
  pkgname="$(get_delta_pkgname $deltafile)"
 
  pkgentry=$(find_pkgentry $pkgname)
- if [ -z "$pkgentry" ]; then
+ if [[ -z $pkgentry ]]; then
  return 1
  fi
  deltas="$pkgentry/deltas"
- if [ ! -f "$deltas" ]; then
+ if [[ ! -f $deltas ]]; then
  return 1
  fi
  if grep -q "$filename" $deltas; then
@@ -219,14 +219,14 @@ db_write_entry()
  csize=$(@SIZECMD@ "$pkgfile")
 
  # ensure $pkgname and $pkgver variables were found
- if [ -z "$pkgname" -o -z "$pkgver" ]; then
+ if [[ -z $pkgname || -z $pkgver ]]; then
  error "$(gettext "Invalid package file '%s'.")" "$pkgfile"
  return 1
  fi
 
  cd "$tmpdir"
 
- if [ -d "$pkgname-$pkgver" ]; then
+ if [[ -d $pkgname-$pkgver ]]; then
  warning "$(gettext "An entry for '%s' already existed")" "$pkgname-$pkgver"
  fi
 
@@ -238,30 +238,30 @@ db_write_entry()
  cd "$pkgname-$pkgver"
 
  # restore an eventual deltas file
- [ -f "../$pkgname.deltas" ] && mv "../$pkgname.deltas" deltas
+ [[ -f ../$pkgname.deltas ]] && mv "../$pkgname.deltas" deltas
 
  # create desc entry
  msg2 "$(gettext "Creating 'desc' db entry...")"
  echo -e "%FILENAME%\n$(basename "$1")\n" >>desc
  echo -e "%NAME%\n$pkgname\n" >>desc
- [ -n "$pkgbase" ] && echo -e "%BASE%\n$pkgbase\n" >>desc
+ [[ -n $pkgbase ]] && echo -e "%BASE%\n$pkgbase\n" >>desc
  echo -e "%VERSION%\n$pkgver\n" >>desc
- [ -n "$pkgdesc" ] && echo -e "%DESC%\n$pkgdesc\n" >>desc
+ [[ -n $pkgdesc ]] && echo -e "%DESC%\n$pkgdesc\n" >>desc
  write_list_entry "GROUPS" "$_groups" "desc"
- [ -n "$csize" ] && echo -e "%CSIZE%\n$csize\n" >>desc
- [ -n "$size" ] && echo -e "%ISIZE%\n$size\n" >>desc
+ [[ -n $csize ]] && echo -e "%CSIZE%\n$csize\n" >>desc
+ [[ -n $size ]] && echo -e "%ISIZE%\n$size\n" >>desc
 
  # compute checksums
  msg2 "$(gettext "Computing md5 checksums...")"
  echo -e "%MD5SUM%\n$md5sum\n" >>desc
 
- [ -n "$url" ] && echo -e "%URL%\n$url\n" >>desc
+ [[ -n $url ]] && echo -e "%URL%\n$url\n" >>desc
  write_list_entry "LICENSE" "$_licenses" "desc"
- [ -n "$arch" ] && echo -e "%ARCH%\n$arch\n" >>desc
- [ -n "$builddate" ] && echo -e "%BUILDDATE%\n$builddate\n" >>desc
- [ -n "$packager" ] && echo -e "%PACKAGER%\n$packager\n" >>desc
+ [[ -n $arch ]] && echo -e "%ARCH%\n$arch\n" >>desc
+ [[ -n $builddate ]] && echo -e "%BUILDDATE%\n$builddate\n" >>desc
+ [[ -n $packager ]] && echo -e "%PACKAGER%\n$packager\n" >>desc
  write_list_entry "REPLACES" "$_replaces" "desc"
- [ -n "$force" ] && echo -e "%FORCE%\n" >>desc
+ [[ -n $force ]] && echo -e "%FORCE%\n" >>desc
 
  # create depends entry
  msg2 "$(gettext "Creating 'depends' db entry...")"
@@ -283,9 +283,9 @@ db_remove_entry() {
  local pkgname=$1
  local notfound=1
  local pkgentry=$(find_pkgentry $pkgname)
- while [ -n "$pkgentry" ]; do
+ while [[ -n $pkgentry ]]; do
  notfound=0
- if [ -f "$pkgentry/deltas" ]; then
+ if [[ -f $pkgentry/deltas ]]; then
  mv "$pkgentry/deltas" "$tmpdir/$pkgname.deltas"
  fi
  msg2 "$(gettext "Removing existing entry '%s'...")" \
@@ -303,16 +303,16 @@ check_repo_db()
  CLEAN_LOCK=1
  else
  error "$(gettext "Failed to acquire lockfile: %s.")" "$LOCKFILE"
- [ -f "$LOCKFILE" ] && error "$(gettext "Held by process %s")" "$(cat $LOCKFILE)"
+ [[ -f $LOCKFILE ]] && error "$(gettext "Held by process %s")" "$(cat $LOCKFILE)"
  exit 1
  fi
 
- if [ -f "$REPO_DB_FILE" ]; then
+ if [[ -f $REPO_DB_FILE ]]; then
  # there are two situations we can have here- a DB with some entries,
  # or a DB with no contents at all.
  if ! bsdtar -tqf "$REPO_DB_FILE" '*/desc' >/dev/null 2>&1; then
  # check empty case
- if [ -n "$(bsdtar -tqf "$REPO_DB_FILE" '*' 2>/dev/null)" ]; then
+ if [[ -n $(bsdtar -tqf "$REPO_DB_FILE" '*' 2>/dev/null) ]]; then
  error "$(gettext "Repository file '%s' is not a proper pacman database.")" "$REPO_DB_FILE"
  exit 1
  fi
@@ -339,15 +339,15 @@ check_repo_db()
 
 add()
 {
- if [ ! -f "$1" ]; then
+ if [[ ! -f $1 ]]; then
  error "$(gettext "File '%s' not found.")" "$1"
  return 1
  fi
 
- if [ "${1##*.}" == "delta" ]; then
+ if [[ ${1##*.} == "delta" ]]; then
  deltafile=$1
  msg "$(gettext "Adding delta '%s'")" "$deltafile"
- if [ ! "$(type -p xdelta3)" ]; then
+ if ! type xdelta3 &>/dev/null; then
  error "$(gettext "Cannot find the xdelta3 binary! Is xdelta3 installed?")"
  exit 1
  fi
@@ -371,7 +371,7 @@ add()
 
 remove()
 {
- if [ "${1##*.}" == "delta" ]; then
+ if [[ ${1##*.} == "delta" ]]; then
  deltafile=$1
  msg "$(gettext "Searching for delta '%s'...")" "$deltafile"
  if db_remove_delta "$deltafile"; then
@@ -405,8 +405,8 @@ clean_up() {
  local exit_code=$?
 
  cd "$startdir"
- [ -d "$tmpdir" ] && rm -rf "$tmpdir"
- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE"
+ [[ -d $tmpdir ]] && rm -rf "$tmpdir"
+ (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
 
  exit $exit_code
 }
@@ -414,7 +414,7 @@ clean_up() {
 # PROGRAM START
 
 # determine whether we have gettext; make it a no-op if we do not
-if [ ! $(type -t gettext) ]; then
+if ! type gettext &>/dev/null; then
  gettext() {
  echo "$@"
  }
@@ -427,7 +427,7 @@ esac
 
 # figure out what program we are
 cmd="$(basename $0)"
-if [ "$cmd" != "repo-add" -a "$cmd" != "repo-remove" ]; then
+if [[ $cmd != "repo-add" && $cmd != "repo-remove" ]]; then
  error "$(gettext "Invalid command name '%s' specified.")" "$cmd"
  exit 1
 fi
@@ -447,7 +447,7 @@ for arg in "$@"; do
  case "$arg" in
  -q|--quiet) QUIET=1;;
  *)
- if [ -z "$REPO_DB_FILE" ]; then
+ if [[ -z $REPO_DB_FILE ]]; then
  REPO_DB_FILE="$arg"
  LOCKFILE="$REPO_DB_FILE.lck"
  check_repo_db
@@ -462,7 +462,7 @@ for arg in "$@"; do
 done
 
 # if at least one operation was a success, re-zip database
-if [ $success -eq 1 ]; then
+if (( success )); then
  msg "$(gettext "Creating updated database file '%s'")" "$REPO_DB_FILE"
 
  case "$REPO_DB_FILE" in
@@ -476,7 +476,7 @@ if [ $success -eq 1 ]; then
  filename=$(basename "$REPO_DB_FILE")
 
  cd "$tmpdir"
- if [ -n "$(ls)" ]; then
+ if [[ -n $(ls) ]]; then
  bsdtar -c${TAR_OPT}f "$filename" *
  else
  # we have no packages remaining? zip up some emptyness
@@ -485,8 +485,8 @@ if [ $success -eq 1 ]; then
  fi
  cd "$startdir"
 
- [ -f "$REPO_DB_FILE" ] && mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old"
- [ -f "$tmpdir/$filename" ] && mv "$tmpdir/$filename" "$REPO_DB_FILE"
+ [[ -f $REPO_DB_FILE ]] && mv -f "$REPO_DB_FILE" "${REPO_DB_FILE}.old"
+ [[ -f $tmpdir/$filename ]] && mv "$tmpdir/$filename" "$REPO_DB_FILE"
 else
  msg "$(gettext "No packages modified, nothing to do.")"
  exit 1
--
1.6.5.2

[Allan McRae-3]

Cedric Staniewski wrote:

> FS#16623 suggested this change for makepkg; this patch applies it to the
> remaining files in the scripts directory.
>
> Signed-off-by: Cedric Staniewski <cedric@...>
> ---
>  scripts/pacman-optimize.sh.in |   22 ++++++------
>  scripts/pkgdelta.sh.in        |   22 ++++++------
>  scripts/rankmirrors.sh.in     |   12 +++---
>  scripts/repo-add.sh.in        |   74 ++++++++++++++++++++--------------------
>  4 files changed, 65 insertions(+), 65 deletions(-)

<snip>

I went through every single change and they all look correct to me.  I
would appreciate someone else also doing a review here are there are a
lot of changes and I could have overlooked something.

The only caution I have is this:

- [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE"
+ (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"

The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne 0 ]
and not [ $CLEAN_LOCK -eq 1 ].  In this case it does not matter as
$CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee that
changing.  But it may pay to be careful.

As an aside. Bash tests are confusing...
(( 0 )) returns 1 or "false"
(( 1 )) returns 0 or "true"
Does that not confuse other people too?

Allan

[Isaac Good-4]

On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:

> Cedric Staniewski wrote:
> >FS#16623 suggested this change for makepkg; this patch applies it to the
> >remaining files in the scripts directory.
> >
> >Signed-off-by: Cedric Staniewski <cedric@...>
> >---
> > scripts/pacman-optimize.sh.in |   22 ++++++------
> > scripts/pkgdelta.sh.in        |   22 ++++++------
> > scripts/rankmirrors.sh.in     |   12 +++---
> > scripts/repo-add.sh.in        |   74 ++++++++++++++++++++--------------------
> > 4 files changed, 65 insertions(+), 65 deletions(-)
> <snip>
>
> I went through every single change and they all look correct to me.
> I would appreciate someone else also doing a review here are there
> are a lot of changes and I could have overlooked something.
>
> The only caution I have is this:
>
> - [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE"
> + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
>
> The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne
> 0 ] and not [ $CLEAN_LOCK -eq 1 ].  In this case it does not matter
> as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee
> that changing.  But it may pay to be careful.
>
> As an aside. Bash tests are confusing...
> (( 0 )) returns 1 or "false"
> (( 1 )) returns 0 or "true"
> Does that not confuse other people too?
>
> Allan
>

In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean.

(( 0 )) and (( 1 )) are very much like how C treats integer testing. I just think of (( )) as a C-style test.

 - Isaac

[Allan McRae-3]

Isaac Good wrote:

> On Fri, Nov 06, 2009 at 03:18:23PM +0000, Allan McRae wrote:
>> Cedric Staniewski wrote:
>>> FS#16623 suggested this change for makepkg; this patch applies it to the
>>> remaining files in the scripts directory.
>>>
>>> Signed-off-by: Cedric Staniewski <cedric@...>
>>> ---
>>> scripts/pacman-optimize.sh.in |   22 ++++++------
>>> scripts/pkgdelta.sh.in        |   22 ++++++------
>>> scripts/rankmirrors.sh.in     |   12 +++---
>>> scripts/repo-add.sh.in        |   74 ++++++++++++++++++++--------------------
>>> 4 files changed, 65 insertions(+), 65 deletions(-)
>> <snip>
>>
>> I went through every single change and they all look correct to me.
>> I would appreciate someone else also doing a review here are there
>> are a lot of changes and I could have overlooked something.
>>
>> The only caution I have is this:
>>
>> - [ $CLEAN_LOCK -eq 1 -a -f "$LOCKFILE" ] && rm -f "$LOCKFILE"
>> + (( CLEAN_LOCK )) && [[ -f $LOCKFILE ]] && rm -f "$LOCKFILE"
>>
>> The (( CLEAN_LOCK )) test is really equivalent of [ $CLEAN_LOCK -ne
>> 0 ] and not [ $CLEAN_LOCK -eq 1 ].  In this case it does not matter
>> as $CLEAN_LOCK can only be assigned 0 or 1 and I can not foresee
>> that changing.  But it may pay to be careful.
>>
>> As an aside. Bash tests are confusing...
>> (( 0 )) returns 1 or "false"
>> (( 1 )) returns 0 or "true"
>> Does that not confuse other people too?
>>
>> Allan
>>
>
> In my original patch for makepkg there were a number of instances where I replaced a test for -eq 1 with a non-zero test. If the variable is assigned always to 0 or 1, this shouldn't be a problem, should it? With the notable exception of SOURCEONLY a lot of variables are boolean.

Well, SOURCEONLY used to only have values 0 and 1 but then got extended
to have 2 as well.  This was more flagging that we will need to be
careful if we ever expand other variables.

Allan

[Cedric Staniewski]

Forgot to point out that the 'type -p anything' tests might not work as intended.

> - if [ ! "$(type -p xdelta3)" ]; then
> + if ! type xdelta3 &>/dev/null; then


$ type asdf
bash: type: asdf: not found
$ type -p asdf; echo $?
1
$
$
$ touch ~/bin/asdf; chmod +x ~/bin/asdf
$ type asdf
asdf is /home/makepkg/bin/asdf
$ type -p asdf; echo $?
/home/makepkg/bin/asdf
0
$ rm ~/bin/asdf
$
$
$ asdf() {
> echo 1
> }
$ type asdf
asdf is a function
asdf ()
{
    echo 1
}
$ type -p asdf; echo $?
0


That means you cannot rely on the return code when you want to emulate which.
You have to test the length of the resulting string instead.

[Isaac Good-4]

On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:

> Forgot to point out that the 'type -p anything' tests might not work as intended.
>
> > - if [ ! "$(type -p xdelta3)" ]; then
> > + if ! type xdelta3 &>/dev/null; then
>
>
> $ type asdf
> bash: type: asdf: not found
> $ type -p asdf; echo $?
> 1
> $
> $
> $ touch ~/bin/asdf; chmod +x ~/bin/asdf
> $ type asdf
> asdf is /home/makepkg/bin/asdf
> $ type -p asdf; echo $?
> /home/makepkg/bin/asdf
> 0
> $ rm ~/bin/asdf
> $
> $
> $ asdf() {
> > echo 1
> > }
> $ type asdf
> asdf is a function
> asdf ()
> {
>     echo 1
> }
> $ type -p asdf; echo $?
> 0
>
>
> That means you cannot rely on the return code when you want to emulate which.
> You have to test the length of the resulting string instead.
>

If the command is a bash function, is that not good enough? (ie why the -p)
type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then

$  type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $?
1
1
0

Definitely worth its own patch, though.

- Isaac

[Cedric Staniewski]

Isaac Good wrote:

> On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
>> Forgot to point out that the 'type -p anything' tests might not work as intended.
>>
>>> - if [ ! "$(type -p xdelta3)" ]; then
>>> + if ! type xdelta3 &>/dev/null; then
>>
>> $ type asdf
>> bash: type: asdf: not found
>> $ type -p asdf; echo $?
>> 1
>> $
>> $
>> $ touch ~/bin/asdf; chmod +x ~/bin/asdf
>> $ type asdf
>> asdf is /home/makepkg/bin/asdf
>> $ type -p asdf; echo $?
>> /home/makepkg/bin/asdf
>> 0
>> $ rm ~/bin/asdf
>> $
>> $
>> $ asdf() {
>>> echo 1
>>> }
>> $ type asdf
>> asdf is a function
>> asdf ()
>> {
>>     echo 1
>> }
>> $ type -p asdf; echo $?
>> 0
>>
>>
>> That means you cannot rely on the return code when you want to emulate which.
>> You have to test the length of the resulting string instead.
>>
>
> If the command is a bash function, is that not good enough? (ie why the -p)
> type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
>
> $  type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $?
> 1
> 1
> 0
>
> Definitely worth its own patch, though.
>
> - Isaac

It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.

[Dan McGee]

On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski <cedric@...> wrote:

> Isaac Good wrote:
>> On Fri, Nov 06, 2009 at 08:01:04PM +0000, Cedric Staniewski wrote:
>>> Forgot to point out that the 'type -p anything' tests might not work as intended.
>>>
>>>> -           if [ ! "$(type -p xdelta3)" ]; then
>>>> +           if ! type xdelta3 &>/dev/null; then
>>>
>>> $ type asdf
>>> bash: type: asdf: not found
>>> $ type -p asdf; echo $?
>>> 1
>>> $
>>> $
>>> $ touch ~/bin/asdf; chmod +x ~/bin/asdf
>>> $ type asdf
>>> asdf is /home/makepkg/bin/asdf
>>> $ type -p asdf; echo $?
>>> /home/makepkg/bin/asdf
>>> 0
>>> $ rm ~/bin/asdf
>>> $
>>> $
>>> $ asdf() {
>>>> echo 1
>>>> }
>>> $ type asdf
>>> asdf is a function
>>> asdf ()
>>> {
>>>     echo 1
>>> }
>>> $ type -p asdf; echo $?
>>> 0
>>>
>>>
>>> That means you cannot rely on the return code when you want to emulate which.
>>> You have to test the length of the resulting string instead.
>>>
>>
>> If the command is a bash function, is that not good enough? (ie why the -p)
>> type -pf will suppress functions, so you can do : if type -pf xdelta3 ; then
>>
>> $  type -p q ; echo $? ; q () { echo ; } ; type -pf q ; echo $? ; type -p q ; echo $?
>> 1
>> 1
>> 0
>>
>> Definitely worth its own patch, though.
>>
>> - Isaac
>
> It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.

dmcgee@galway ~/projects/pacman (gpg-more)
$ type -p xdelta3; echo $?
1

dmcgee@galway ~/projects/pacman (gpg-more)
$ type xdelta3; echo $?
-bash: type: xdelta3: not found
1

Note that -p suppresses error messages that you may not want; it's
output is only the return code.

-Dan

[Isaac Good-4]

On Tue, Nov 10, 2009 at 02:23:25PM -0600, Dan McGee wrote:

> On Tue, Nov 10, 2009 at 2:20 PM, Cedric Staniewski <cedric@...> wrote:
> >
> > It might be enough, but I wonder, as well, why the -p option is used. I removed it in my patch since it is not needed when testing the return code, but maybe I missed something.
>
> dmcgee@galway ~/projects/pacman (gpg-more)
> $ type -p xdelta3; echo $?
> 1
>
> dmcgee@galway ~/projects/pacman (gpg-more)
> $ type xdelta3; echo $?
> -bash: type: xdelta3: not found
> 1
>
> Note that -p suppresses error messages that you may not want; it's
> output is only the return code.
>
> -Dan
>

The -p only suppresses error messages (STDERR) when it fails. On success it still emits the path to the file on disk (STDOUT).
If you want to do the test like this:
 $ if [ -n "$(type -p xdelta3)" ] ; then
then that is the desired behaviour. Output on success, nothing on failure. If you want to just use the return status, you still need to do something with the STDOUT.
 $ if type -p xdelta3 >/dev/null ; then
or
 $ if type xdelta3 >/dev/null 2>&1 ; then

[Isaac Good-4]

>From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001
From: Isaac Good <pacman@...>
Date: Wed, 11 Nov 2009 16:47:43 -0500
Subject: [PATCH 1/2] Changing [ to [[ and ((

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

Signed-off-by: Isaac Good <pacman@...>
---
 scripts/makepkg.sh.in |  202 ++++++++++++++++++++++++------------------------
 1 files changed, 101 insertions(+), 101 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index 25fb8d9..8a80d6c 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -112,7 +112,7 @@ error() {
 # the fakeroot call, the error message will be printed by the main call.
 ##
 trap_exit() {
- if [ "$INFAKEROOT" -eq 0 ]; then
+ if (( ! INFAKEROOT )); then
  echo
  error "$@"
  fi
@@ -126,21 +126,21 @@ trap_exit() {
 clean_up() {
  local EXIT_CODE=$?
 
- if [ "$INFAKEROOT" -eq 1 ]; then
+ if (( INFAKEROOT )); then
  # Don't clean up when leaving fakeroot, we're not done yet.
  return
  fi
 
- if [ $EXIT_CODE -eq 0 -a "$CLEANUP" -eq 1 ]; then
+ if (( ! EXIT_CODE && CLEANUP )); then
  # If it's a clean exit and -c/--clean has been passed...
  msg "$(gettext "Cleaning up...")"
  rm -rf "$pkgdir" "$srcdir"
- if [ -n "$pkgbase" ]; then
+ if [[ -n $pkgbase ]]; then
  # Can't do this unless the BUILDSCRIPT has been sourced.
  rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-build.log"*
- if [ "$PKGFUNC" -eq 1 ]; then
+ if (( PKGFUNC )); then
  rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-package.log"*
- elif [ "$SPLITPKG" -eq 1 ]; then
+ elif (( SPLITPKG )); then
  for pkg in ${pkgname[@]}; do
  rm -f "${pkg}-${pkgver}-${pkgrel}-${CARCH}-package.log"*
  done
@@ -190,14 +190,14 @@ get_url() {
 ##
 check_option() {
  local ret=$(in_opt_array "$1" ${options[@]})
- if [ "$ret" != '?' ]; then
+ if [[ $ret != '?' ]]; then
  echo $ret
  return
  fi
 
  # fall back to makepkg.conf options
  ret=$(in_opt_array "$1" ${OPTIONS[@]})
- if [ "$ret" != '?' ]; then
+ if [[ $ret != '?' ]]; then
  echo $ret
  return
  fi
@@ -231,10 +231,10 @@ in_opt_array() {
  local opt
  for opt in "$@"; do
  opt="${opt,,}"
- if [ "$opt" = "$needle" ]; then
+ if [[ $opt = $needle ]]; then
  echo 'y' # Enabled
  return
- elif [ "$opt" = "!$needle" ]; then
+ elif [[ $opt = "!$needle" ]]; then
  echo 'n' # Disabled
  return
  fi
@@ -251,10 +251,10 @@ in_opt_array() {
 ##
 in_array() {
  local needle=$1; shift
- [ -z "$1" ] && return 1 # Not Found
+ [[ -z $1 ]] && return 1 # Not Found
  local item
  for item in "$@"; do
- [ "$item" = "$needle" ] && return 0 # Found
+ [[ $item = $needle ]] && return 0 # Found
  done
  return 1 # Not Found
 }
@@ -268,14 +268,14 @@ get_downloadclient() {
  local i
  for i in "${DLAGENTS[@]}"; do
  local handler="${i%%::*}"
- if [ "$proto" = "$handler" ]; then
+ if [[ $proto = $handler ]]; then
  agent="${i##*::}"
  break
  fi
  done
 
  # if we didn't find an agent, return an error
- if [ -z "$agent" ]; then
+ if [[ -z $agent ]]; then
  error "$(gettext "There is no agent set up to handle %s URLs. Check %s.")" "$proto" "$MAKEPKG_CONF"
  plain "$(gettext "Aborting...")"
  exit 1 # $E_CONFIG_ERROR
@@ -283,7 +283,7 @@ get_downloadclient() {
 
  # ensure specified program is installed
  local program="${agent%% *}"
- if [ ! -x "$program" ]; then
+ if [[ ! -x $program ]]; then
  local baseprog=$(basename $program)
  error "$(gettext "The download program %s is not installed.")" "$baseprog"
  plain "$(gettext "Aborting...")"
@@ -317,25 +317,25 @@ download_file() {
 
  local ret=0
  eval "$dlcmd || ret=\$?"
- if [ $ret -gt 0 ]; then
- [ ! -s "$dlfile" ] && rm -f -- "$dlfile"
+ if (( ret )); then
+ [[ ! -s $dlfile ]] && rm -f -- "$dlfile"
  return $ret
  fi
 
  # rename the temporary download file to the final destination
- if [ "$dlfile" != "$file" ]; then
+ if [[ $dlfile != $file ]]; then
  mv -f "$SRCDEST/$dlfile" "$SRCDEST/$file"
  fi
 }
 
 check_deps() {
- [ $# -gt 0 ] || return
+ (( $# )) || return
 
  pmout=$(pacman $PACMAN_OPTS -T "$@")
  ret=$?
- if [ $ret -eq 127 ]; then #unresolved deps
+ if (( ret == 127 )); then #unresolved deps
  echo "$pmout"
- elif [ $ret -ne 0 ]; then
+ elif (( ret )); then
  error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout"
  exit 1
  fi
@@ -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
 
  local deplist="$*"
 
- if [ "$DEP_BIN" -eq 0 ]; then
+ if (( ! DEP_BIN )); then
  return $R_DEPS_MISSING
  fi
 
- if [ "$DEP_BIN" -eq 1 ]; then
+ if (( DEP_BIN )); then
  # install missing deps from binary packages (using pacman -S)
  msg "$(gettext "Installing missing dependencies...")"
  local ret=0
 
- if [ "$ASROOT" -eq 0 ]; then
+ if (( ! ASROOT )); then
  sudo pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
  else
  pacman $PACMAN_OPTS -S --asdeps $deplist || ret=$?
  fi
 
- if [ $ret -ne 0 ]; then
+ if (( ret )); then
  error "$(gettext "Pacman failed to install missing dependencies.")"
  exit 1 # TODO: error code
  fi
@@ -385,7 +385,7 @@ resolve_deps() {
  local R_DEPS_MISSING=1
 
  local deplist="$(check_deps $*)"
- if [ -z "$deplist" ]; then
+ if [[ -z $deplist ]]; then
  return $R_DEPS_SATISFIED
  fi
 
@@ -393,8 +393,8 @@ resolve_deps() {
  pkgdeps="$pkgdeps $deplist"
  # check deps again to make sure they were resolved
  deplist="$(check_deps $*)"
- [ -z "$deplist" ] && return $R_DEPS_SATISFIED
- elif [ "$DEP_BIN" -eq 1 ]; then
+ [[ -z $deplist ]] && return $R_DEPS_SATISFIED
+ elif (( DEP_BIN )); then
  error "$(gettext "Failed to install all missing dependencies.")"
  fi
 
@@ -410,8 +410,8 @@ resolve_deps() {
 # fix flyspray bug #5923
 remove_deps() {
  # $pkgdeps is a GLOBAL variable, set by resolve_deps()
- [ "$RMDEPS" -eq 0 ] && return
- [ -z "$pkgdeps" ] && return
+ (( ! RMDEPS )) && return
+ [[ -z $pkgdeps ]] && return
 
  local dep depstrip deplist
  deplist=""
@@ -422,14 +422,14 @@ remove_deps() {
 
  msg "Removing installed dependencies..."
  local ret=0
- if [ "$ASROOT" -eq 0 ]; then
+ if (( ! ASROOT )); then
  sudo pacman $PACMAN_OPTS -Rns $deplist || ret=$?
  else
  pacman $PACMAN_OPTS -Rns $deplist || ret=$?
  fi
 
  # Fixes FS#10039 - exit cleanly as package has built successfully
- if [ $ret -ne 0 ]; then
+ if (( ret )); then
  warning "$(gettext "Failed to remove installed dependencies.")"
  return 0
  fi
@@ -438,7 +438,7 @@ remove_deps() {
 download_sources() {
  msg "$(gettext "Retrieving Sources...")"
 
- if [ ! -w "$SRCDEST" ] ; then
+ if [[ ! -w $SRCDEST ]] ; then
  error "$(gettext "You do not have write permission to store downloads in %s.")" "$SRCDEST"
  plain "$(gettext "Aborting...")"
  exit 1
@@ -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/"
@@ -463,7 +463,7 @@ download_sources() {
  fi
 
  # if we get here, check to make sure it was a URL, else fail
- if [ "$file" = "$url" ]; then
+ if [[ $file = $url ]]; then
  error "$(gettext "%s was not found in the build directory and is not a URL.")" "$file"
  exit 1 # $E_MISSING_FILE
  fi
@@ -475,7 +475,7 @@ download_sources() {
  # fix flyspray bug #3289
  local ret=0
  download_file "$dlclient" "$url" "$file" || ret=$?
- if [ $ret -gt 0 ]; then
+ if (( ret )); then
  error "$(gettext "Failure while downloading %s")" "$file"
  plain "$(gettext "Aborting...")"
  exit 1
@@ -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
@@ -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
@@ -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'"
  ct=$(($ct+1))
- [ $ct -lt $numsrc ] && echo
+ (( $ct < $numsrc )) && echo
  done
 
  echo ")"
@@ -544,7 +544,7 @@ generate_checksums() {
 }
 
 check_checksums() {
- [ ${#source[@]} -eq 0 ] && return 0
+ (( ! ${#source[@]} )) && return 0
 
  if [ ! $(type -p openssl) ]; then
  error "$(gettext "Cannot find openssl.")"
@@ -555,7 +555,7 @@ check_checksums() {
  local integ required
  for integ in md5 sha1 sha256 sha384 sha512; do
  local integrity_sums=($(eval echo "\${${integ}sums[@]}"))
- if [ ${#integrity_sums[@]} -eq ${#source[@]} ]; then
+ if (( ${#integrity_sums[@]} == ${#source[@]} )); then
  msg "$(gettext "Validating source files with %s...")" "${integ}sums"
  correlation=1
  local errors=0
@@ -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
@@ -576,11 +576,11 @@ check_checksums() {
  fi
  fi
 
- if [ $found -gt 0 ] ; then
+ if (( $found )) ; then
  local expectedsum="${integrity_sums[$idx],,}"
  local realsum="$(openssl dgst -${integ} "$file")"
  realsum="${realsum##* }"
- if [ "$expectedsum" = "$realsum" ]; then
+ if [[ $expectedsum = $realsum ]]; then
  echo "$(gettext "Passed")" >&2
  else
  echo "$(gettext "FAILED")" >&2
@@ -591,18 +591,18 @@ check_checksums() {
  idx=$((idx + 1))
  done
 
- if [ $errors -gt 0 ]; then
+ if (( errors )); then
  error "$(gettext "One or more files did not pass the validity check!")"
  exit 1 # TODO: error code
  fi
- elif [ ${#integrity_sums[@]} -gt 0 ]; then
+ elif (( ${#integrity_sums[@]} )); then
  error "$(gettext "Integrity checks (%s) differ in size from the source array.")" "$integ"
  exit 1 # TODO: error code
  fi
  done
 
- if [ $correlation -eq 0 ]; then
- if [ $SKIPINTEG -eq 1 ]; then
+ if (( ! correlation )); then
+ if (( SKIPINTEG )); then
  warning "$(gettext "Integrity checks are missing.")"
  else
  error "$(gettext "Integrity checks are missing.")"
@@ -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
@@ -662,31 +662,31 @@ extract_sources() {
 
  local ret=0
  msg2 "$(gettext "Extracting %s with %s")" "$file" "$cmd"
- if [ "$cmd" = "bsdtar" ]; then
+ if [[ $cmd = bsdtar ]]; then
  $cmd -xf "$file" || ret=?
  else
  rm -f "${file%.*}"
  $cmd -dcf "$file" > "${file%.*}" || ret=?
  fi
- if [ $ret -ne 0 ]; then
+ if (( ret )); then
  error "$(gettext "Failed to extract %s")" "$file"
  plain "$(gettext "Aborting...")"
  exit 1
  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
 }
 
 error_function() {
- if [ -p "$logpipe" ]; then
+ if [[ -p $logpipe ]]; then
  rm "$logpipe"
  fi
  # first exit all subshells, then print the error
- if [ $BASH_SUBSHELL -eq 0 ]; then
+ if (( ! BASH_SUBSHELL )); then
  plain "$(gettext "Aborting...")"
  remove_deps
  fi
@@ -694,13 +694,13 @@ error_function() {
 }
 
 run_function() {
- if [ -z "$1" ]; then
+ if [[ -z $1 ]]; then
  return 1
  fi
  pkgfunc="$1"
 
  # clear user-specified makeflags if requested
- if [ "$(check_option makeflags)" = "n" ]; then
+ if [[ "$(check_option makeflags)" = "n" ]]; then
  MAKEFLAGS=""
  fi
 
@@ -713,12 +713,12 @@ run_function() {
  local shellopts=$(shopt -p)
 
  local ret=0
- if [ "$LOGGING" -eq 1 ]; then
+ if (( LOGGING )); then
  BUILDLOG="${startdir}/${pkgname}-${pkgver}-${pkgrel}-${CARCH}-$pkgfunc.log"
- if [ -f "$BUILDLOG" ]; then
+ if [[ -f $BUILDLOG ]]; then
  local i=1
  while true; do
- if [ -f "$BUILDLOG.$i" ]; then
+ if [[ -f "$BUILDLOG.$i" ]]; then
  i=$(($i +1))
  else
  break
@@ -752,24 +752,24 @@ run_function() {
 
 run_build() {
  # use distcc if it is requested (check buildenv and PKGBUILD opts)
- if [ "$(check_buildenv distcc)" = "y" -a "$(check_option distcc)" != "n" ]; then
- [ -d /usr/lib/distcc/bin ] && export PATH="/usr/lib/distcc/bin:$PATH"
+ if [[ "$(check_buildenv distcc)" = "y" && "$(check_option distcc)" != "n" ]]; then
+ [[ -d /usr/lib/distcc/bin ]] && export PATH="/usr/lib/distcc/bin:$PATH"
  export DISTCC_HOSTS
- elif [ "$(check_option distcc)" = "n" ]; then
+ elif [[ "$(check_option distcc)" = "n" ]]; then
  # if it is not wanted, clear the makeflags too
  MAKEFLAGS=""
  fi
 
  # use ccache if it is requested (check buildenv and PKGBUILD opts)
- if [ "$(check_buildenv ccache)" = "y" -a "$(check_option ccache)" != "n" ]; then
- [ -d /usr/lib/ccache/bin ] && export PATH="/usr/lib/ccache/bin:$PATH"
+ if [[ "$(check_buildenv ccache)" = "y" && "$(check_option ccache)" != "n" ]]; then
+ [[ -d /usr/lib/ccache/bin ]] && export PATH="/usr/lib/ccache/bin:$PATH"
  fi
 
  run_function "build"
 }
 
 run_package() {
- if [ -z "$1" ]; then
+ if [[ -z $1 ]]; then
  pkgfunc="package"
  else
  pkgfunc="package_$1"
@@ -782,16 +782,16 @@ tidy_install() {
  cd "$pkgdir"
  msg "$(gettext "Tidying install...")"
 
- if [ "$(check_option docs)" = "n" -a -n "${DOC_DIRS[*]}" ]; then
+ if [[ "$(check_option docs)" = "n" && -n "${DOC_DIRS[*]}" ]]; then
  msg2 "$(gettext "Removing doc files...")"
  rm -rf ${DOC_DIRS[@]}
  fi
 
- if [ "$(check_option purge)" = "y" -a -n "${PURGE_TARGETS[*]}" ]; then
+ if [[ "$(check_option purge)" = "y" && -n "${PURGE_TARGETS[*]}" ]]; then
  msg2 "$(gettext "Purging other files...")"
  local pt
  for pt in "${PURGE_TARGETS[@]}"; do
- if [ "${pt}" = "${pt//\/}" ]; then
+ if [[ "${pt}" = "${pt//\/}" ]]; then
  find . -type f -name "${pt}" -exec rm -f -- '{}' \;
  else
  rm -f ${pt}
@@ -799,16 +799,16 @@ tidy_install() {
  done
  fi
 
- if [ "$(check_option zipman)" = "y" -a -n "${MAN_DIRS[*]}" ]; then
+ if [[ "$(check_option zipman)" = "y" && -n "${MAN_DIRS[*]}" ]]; then
  msg2 "$(gettext "Compressing man and info pages...")"
  local manpage ext file link hardlinks hl
  find ${MAN_DIRS[@]} -type f 2>/dev/null |
  while read manpage ; do
  # check file still exists (potentially compressed with hard link)
- if [ -f ${manpage} ]; then
+ if [[ -f ${manpage} ]]; then
  ext="${manpage##*.}"
  file="${manpage##*/}"
- if [ "$ext" != "gz" -a "$ext" != "bz2" ]; then
+ if [[ $ext != gz && $ext != bz2 ]]; then
  # update symlinks to this manpage
  find ${MAN_DIRS[@]} -lname "$file" 2>/dev/null |
  while read link ; do
@@ -834,7 +834,7 @@ tidy_install() {
  done
  fi
 
- if [ "$(check_option strip)" = "y" -a -n "${STRIP_DIRS[*]}" ]; then
+ if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then
  msg2 "$(gettext "Stripping debugging symbols from binaries and libraries...")"
  local binary
  find ${STRIP_DIRS[@]} -type f 2>/dev/null | while read binary ; do
@@ -851,12 +851,12 @@ tidy_install() {
  done
  fi
 
- if [ "$(check_option libtool)" = "n" ]; then
+ if [[ "$(check_option libtool)" = "n" ]]; then
  msg2 "$(gettext "Removing libtool .la files...")"
  find . ! -type d -name "*.la" -exec rm -f -- '{}' \;
  fi
 
- if [ "$(check_option emptydirs)" = "n" ]; then
+ if [[ "$(check_option emptydirs)" = "n" ]]; then
  msg2 "$(gettext "Removing empty directories...")"
  find . -depth -type d -empty -delete
  fi
@@ -864,7 +864,7 @@ tidy_install() {
 
 write_pkginfo() {
  local builddate=$(date -u "+%s")
- if [ -n "$PACKAGER" ]; then
+ if [[ -n $PACKAGER ]]; then
  local packager="$PACKAGER"
  else
  local packager="Unknown Packager"
@@ -874,12 +874,12 @@ write_pkginfo() {
 
  msg2 "$(gettext "Generating .PKGINFO file...")"
  echo "# Generated by makepkg $myver" >.PKGINFO
- if [ "$INFAKEROOT" -eq 1 ]; then
+ if (( INFAKEROOT )); then
  echo "# using $(fakeroot -v)" >>.PKGINFO
  fi
  echo "# $(LC_ALL=C date -u)" >>.PKGINFO
  echo "pkgname = $1" >>.PKGINFO
- [ "$SPLITPKG" -eq 1 ] && echo "pkgbase = $pkgbase" >>.PKGINFO
+ (( SPLITPKG )) && echo pkgbase = $pkgbase >>.PKGINFO
  echo "pkgver = $pkgver-$pkgrel" >>.PKGINFO
  echo "pkgdesc = $pkgdesc" >>.PKGINFO
  echo "url = $url" >>.PKGINFO
@@ -887,7 +887,7 @@ write_pkginfo() {
  echo "packager = $packager" >>.PKGINFO
  echo "size = $size" >>.PKGINFO
  echo "arch = $PKGARCH" >>.PKGINFO
- if [ "$(check_option force)" = "y" ]; then
+ if [[ "$(check_option force)" = "y" ]]; then
  echo "force = true" >> .PKGINFO
  fi
 
@@ -918,8 +918,8 @@ write_pkginfo() {
  done
  for it in "${packaging_options[@]}"; do
  local ret="$(check_option $it)"
- if [ "$ret" != "?" ]; then
- if [ "$ret" = "y" ]; then
+ if [[ $ret != "?" ]]; then
+ if [[ $ret = y ]]; then
  echo "makepkgopt = $it" >>.PKGINFO
  else
  echo "makepkgopt = !$it" >>.PKGINFO
@@ -929,7 +929,7 @@ write_pkginfo() {
 
  # TODO maybe remove this at some point
  # warn if license array is not present or empty
- if [ -z "$license" ]; then
+ if [[ -z $license ]]; then
  warning "$(gettext "Please add a license line to your %s!")" "$BUILDSCRIPT"
  plain "$(gettext "Example for GPL\'ed software: license=('GPL').")"
  fi
@@ -941,14 +941,14 @@ check_package() {
  # check existence of backup files
  local file
  for file in "${backup[@]}"; do
- if [ ! -f "$file" ]; then
+ if [[ ! -f $file ]]; then
  warning "$(gettext "Invalid backup entry : %s")" "$file"
  fi
  done
 }
 
 create_package() {
- if [ ! -d "$pkgdir" ]; then
+ if [[ ! -d $pkgdir ]]; then
  error "$(gettext "Missing pkg/ directory.")"
  plain "$(gettext "Aborting...")"
  exit 1 # $E_MISSING_PKGDIR
@@ -959,13 +959,13 @@ create_package() {
  cd "$pkgdir"
  msg "$(gettext "Creating package...")"
 
- if [ -z "$1" ]; then
+ if [[ -z $1 ]]; then
  nameofpkg="$pkgname"
  else
  nameofpkg="$1"
  fi
 
- if [ "$arch" = "any" ]; then
+ if [[ $arch = "any" ]]; then
  PKGARCH="any"
  else
  PKGARCH=$CARCH
@@ -976,14 +976,14 @@ create_package() {
  local comp_files=".PKGINFO"
 
  # check for an install script
- if [ -n "$install" ]; then
+ if [[ -n $install ]]; then
  msg2 "$(gettext "Adding install script...")"
  cp "$startdir/$install" .INSTALL
  comp_files="$comp_files .INSTALL"
  fi
 
  # do we have a changelog?
- if [ -n "$changelog" ]; then
+ if [[ -n $changelog ]]; then
  msg2 "$(gettext "Adding package changelog...")"
  cp "$startdir/$changelog" .CHANGELOG
  comp_files="$comp_files .CHANGELOG"
@@ -1009,7 +1009,7 @@ create_package() {
  bsdtar -cf - $comp_files * > "$pkg_file" || ret=$?
  shopt -u nullglob
 
- if [ $ret -eq 0 ]; then
+ if (( ! ret )); then
  case "$PKGEXT" in
  *tar.gz)  gzip -f -n "$pkg_file" ;;
  *tar.bz2) bzip2 -f "$pkg_file" ;;
@@ -1018,7 +1018,7 @@ create_package() {
  ret=$?
  fi
 
- if [ $ret -ne 0 ]; then
+ if (( ret )); then
  error "$(gettext "Failed to create package file.")"
  exit 1 # TODO: error code
  fi
@@ -1042,8 +1042,8 @@ create_srcpackage() {
  msg2 "$(gettext "Adding %s...")" "$BUILDSCRIPT"
  ln -s "${BUILDFILE}" "${srclinks}/${pkgbase}/${BUILDSCRIPT}"
 
- if [ -n "$install" ]; then
- if [ -f $install ]; then
+ if [[ -n $install ]]; then
+ if [[ -f $install ]]; then
  msg2 "$(gettext "Adding install script...")"
  ln -s "${startdir}/$install" "${srclinks}/${pkgbase}/"
  else
@@ -1051,8 +1051,8 @@ create_srcpackage() {
  fi
  fi
 
- if [ -n "$changelog" ]; then
- if [ -f "$changelog" ]; then
+ if [[ -n $changelog ]]; then
+ if [[ -f $changelog ]]; then
  msg2 "$(gettext "Adding package changelog...")"
  ln -s "${startdir}/$changelog" "${srclinks}/${pkgbase}/"
  else
@@ -1063,10 +1063,10 @@ create_srcpackage() {
  local netfile
  for netfile in "${source[@]}"; do
  local file=$(get_filename "$netfile")
- if [ -f "$netfile" ]; then
+ if [[ -f $netfile ]]; then
  msg2 "$(gettext "Adding %s...")" "$netfile"
  ln -s "${startdir}/$netfile" "${srclinks}/${pkgbase}"
- elif [ "$SOURCEONLY" -eq 2 -a -f "$SRCDEST/$file" ]; then
+ elif (( SOURCEONLY == 2 )) && [[ -f "$SRCDEST/$file" ]]; then
  msg2 "$(gettext "Adding %s...")" "$file"
  ln -s "$SRCDEST/$file" "${srclinks}/${pkgbase}/"
  fi
--
1.6.5.2

[Cedric Staniewski]

Isaac Good wrote:

>> >From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001
> From: Isaac Good <pacman@...>
> Date: Wed, 11 Nov 2009 16:47:43 -0500
> Subject: [PATCH 1/2] Changing [ to [[ and ((
>
> First 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 traps
>
> Signed-off-by: Isaac Good <pacman@...>
> ---

I hope your patch do not get lost in this thread.

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

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

> @@ -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"?

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

> @@ -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 )) ?

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

[Isaac Good-4]

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

[Isaac Good-4]

On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:

> > @@ -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/"

What about something like this?
> if [[ "$(check_option makeflags)" = "n" ]]; then

None of the quotes are needed. I'm inclined to replace it with:
> if [[ $(check_option makeflags) = "n" ]]; then

While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes.

Maybe some style guidelines are needed for the bash language...

 - Isaac

[Cedric Staniewski]

Isaac Good wrote:
> 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?
>

I guess you hit the reply button instead of writing a new mail to
pacman-dev. At least, there is the In-Reply-To header field in your mail
header.

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

I am fine with that, but would it not makes sense to use (( $# > 0 ))
instead of just (( $# )) then?

>>> @@ -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++ ))
>

In this case, I would either get rid of -lt by converting to a for loop
 directly or not touch that line at all in this patch.

> Should I implement these changes and resend this patch?
> /me goes looking for git help
>

If you like, but you could also wait for at least Allan's comment. You
can use "git commit --amend" to alter your existing commit in git by the
way.

[Allan McRae-3]

Isaac Good wrote:

> On Thu, Nov 12, 2009 at 07:16:41PM +0000, Cedric Staniewski wrote:
>>> @@ -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/"
>
> What about something like this?
>> if [[ "$(check_option makeflags)" = "n" ]]; then
>
> None of the quotes are needed. I'm inclined to replace it with:
>> if [[ $(check_option makeflags) = "n" ]]; then
>
> While the quotes around n are not needed, they highlight that n is a string which appeals to me since it looks like a string as found in other languages. Or would you say to drop all the quotes.
>

Aren't the quotes around the string on the right required if it does
contain spaces? Anyway, I would prefer all such strings to be quoted so
I am happy with your suggestion.

Allan

[Allan McRae-3]

Cedric Staniewski wrote:

> Isaac Good wrote:
>>> >From f40b051c1c649889a13f35965fa8f8decd505b47 Mon Sep 17 00:00:00 2001
>> From: Isaac Good <pacman@...>
>> Date: Wed, 11 Nov 2009 16:47:43 -0500
>> Subject: [PATCH 1/2] Changing [ to [[ and ((
>>
>> First 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 traps
>>
>> Signed-off-by: Isaac Good <pacman@...>
>> ---
>
> I hope your patch do not get lost in this thread.

I have the patches marked so that they will not be lost.

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

Hmmm...  can we.  I know we can when there are spaces in a single
variable, but for some reason I thought quotes were needed when joining
multiple variables like that.  I could be wrong...

Allan

[Isaac Good-4]

On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:

> Isaac Good wrote:
> > 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?
> >
>
> I guess you hit the reply button instead of writing a new mail to
> pacman-dev. At least, there is the In-Reply-To header field in your mail
> header.
>
> >>> @@ -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.
> >
>
> I am fine with that, but would it not makes sense to use (( $# > 0 ))
> instead of just (( $# )) then?

I am not sure I understand what you are saying.
In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG ))
With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for.
All of these do the same thing:
$ (( $# == 0 )) && return $R_DEPS_SATISFIED
$ (( $# > 0 )) || return $R_DEPS_SATISFIED
$ (( ! $# ))  && return $R_DEPS_SATISFIED
$ (( $# )) || return $R_DEPS_SATISFIED

I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.

> >>> @@ -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++ ))
> >
>
> In this case, I would either get rid of -lt by converting to a for loop
>  directly or not touch that line at all in this patch.

amended/reversed

> > Should I implement these changes and resend this patch?
> > /me goes looking for git help
> >
>
> If you like, but you could also wait for at least Allan's comment. You
> can use "git commit --amend" to alter your existing commit in git by the
> way.
 
Thanks. I'm learning a lot of git today ;)

 - Isaac

[Cedric Staniewski]

Allan McRae wrote:
> Cedric Staniewski wrote:
>> We could remove the quotes here, too.
>>
>
> Hmmm...  can we.  I know we can when there are spaces in a single
> variable, but for some reason I thought quotes were needed when joining
> multiple variables like that.  I could be wrong...
>

[[ behaves quite different compared to [ in this aspect, but as far as I
know, you do not need quotes to join variables as long as there are no
spaces between the variables ;). There are several exceptions though,
like cd for example, which require quotes even for single variables when
they contain spaces. I think this has something to do with the type of
the command. Builtins/external commands (usually) need quotes whereas
everything else should work without.
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.

$ a=" a d"
$ b=" e"
$ c=$a:$b
$ echo "$c"
 a d: e
$
$
$ filename="a d e"
$ suffix=".f"
$ [ -e ${filename}${suffix} ] && echo 1
bash: [: too many arguments
$ [[ -e ${filename}${suffix} ]] && echo 1
$ touch "${filename}${suffix}"
$ [[ -e ${filename}${suffix} ]] && echo 1
1
$ [[ -e a d e.f ]] && echo 1
bash: syntax error in conditional expression
bash: syntax error near `d'
$ [[ -e "a d e.f" ]] && echo 1
1

[Cedric Staniewski]

Isaac Good wrote:

> On Thu, Nov 12, 2009 at 10:37:31PM +0000, Cedric Staniewski wrote:
>> Isaac Good wrote:
>>> 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?
>>>
>> I guess you hit the reply button instead of writing a new mail to
>> pacman-dev. At least, there is the In-Reply-To header field in your mail
>> header.
>>
>>>>> @@ -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.
>>>
>> I am fine with that, but would it not makes sense to use (( $# > 0 ))
>> instead of just (( $# )) then?
>
> I am not sure I understand what you are saying.
> In general there are no negative values. With boolean flags either it is set or not set. So you get : if (( FLAG )) and if (( ! FLAG ))
> With $# (and EUID) the code is set up to test for zero, not non-zero. (( $# > 0 )) and (( $# )) are the reverse of what the code checks for.
> All of these do the same thing:
> $ (( $# == 0 )) && return $R_DEPS_SATISFIED
> $ (( $# > 0 )) || return $R_DEPS_SATISFIED
> $ (( ! $# ))  && return $R_DEPS_SATISFIED
> $ (( $# )) || return $R_DEPS_SATISFIED
>
> I left the test as && because there was no compelling reason to change it. That aside, I am in favor of either of the first two for $# as opposed to the latter two for boolean flags.
>

Sorry, I meant another line from your patch where you use (( $# )).

>  }
>  
>  check_deps() {
> - [ $# -gt 0 ] || return
> + (( $# )) || return
>  
>   pmout=$(pacman $PACMAN_OPTS -T "$@")
>   ret=$?
> - if [ $ret -eq 127 ]; then #unresolved deps
> + if (( ret == 127 )); then #unresolved deps
>   echo "$pmout"
> - elif [ $ret -ne 0 ]; then
> + elif (( ret )); then
>   error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout"
>   exit 1
>   fi

ret is another non-boolean variable by the way. I think some kind of
code style guide is a good idea. :)

[Isaac Good-4]

On Thu, Nov 12, 2009 at 11:52:31PM +0000, Cedric Staniewski wrote:

> Allan McRae wrote:
> > Cedric Staniewski wrote:
> >> We could remove the quotes here, too.
> >>
> >
> > Hmmm...  can we.  I know we can when there are spaces in a single
> > variable, but for some reason I thought quotes were needed when joining
> > multiple variables like that.  I could be wrong...
> >
>
> [[ behaves quite different compared to [ in this aspect, but as far as I
> know, you do not need quotes to join variables as long as there are no
> spaces between the variables ;). There are several exceptions though,
> like cd for example, which require quotes even for single variables when
> they contain spaces. I think this has something to do with the type of
> the command. Builtins/external commands (usually) need quotes whereas
> everything else should work without.
> 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.
>
> $ a=" a d"
> $ b=" e"
> $ c=$a:$b
> $ echo "$c"
>  a d: e
> $
> $
> $ filename="a d e"
> $ suffix=".f"
> $ [ -e ${filename}${suffix} ] && echo 1
> bash: [: too many arguments
> $ [[ -e ${filename}${suffix} ]] && echo 1
> $ touch "${filename}${suffix}"
> $ [[ -e ${filename}${suffix} ]] && echo 1
> 1
> $ [[ -e a d e.f ]] && echo 1
> bash: syntax error in conditional expression
> bash: syntax error near `d'
> $ [[ -e "a d e.f" ]] && echo 1
> 1
>

The [[ ]] and (( )) are the exception to everything else in bash (cd, [, touch, grep). I know of nothing else that acts like it (except array indices).
With all bash commands, the variable is expanded early enough that a space will split the result into multiple parameters (aka word splitting). Ergo these two are the same:
$ cd a b
$ var="a b"; cd $var
cd will see two arguments. Compare to this, which has one parameter:
$ cd "a b"
$ var="a b"; cd "$var"

The same applies to [, which can make stuff fun, eg:
$ [ -n a -a -z a ]               -> returns false because the second condition is false and there is an and (-a)
$ var="a -a -z a"; [ -n $var ]   -> false, too! Go figure...

$ var="a -a -z a"; [ -n "$var" ] -> works right

The [[ and (( are "special". Word splitting will not occur inside. (Array indices are computed in arithmetic mode, ie like inside ((: ${arr[stuff]}).
The entire parsing is set up different. Word splitting does not occur. And (mostly as an aside,) bash parses them over double as fast.

Concatenating variables has no special rules. It acts identical to one variable with both the contents. Assuming there is no whitespace between them, it is just like one variable. Whitespace inside causes word splitting but not in (( and [[.

Once discussing word splitting, this also applies to arrays, specifically in this context:
$ for var in ${arr[@]} ; do

Without quotes, elements with a space will be turned into multiple elements for the loop.

 - Isaac