Merge ~cjwatson/launchpad:improve-shell-quoting into launchpad:master

[Colin Watson]

Improve quoting in shell scripts

In shell, $-expansions should normally be double-quoted to prevent word splitting being applied to the result of the expansion, except in cases where word splitting is specifically needed. `shellcheck` complained about a number of cases where we were failing to do this, so fix them.

[Jürgen Gmach (jugmac00)]

I may miss the reason why you do not add the pre-commit hook for shellcheck with this commit?

If we do not enforce shellcheck, there is always the chance that bad code gets added again.

review: Approve

[Colin Watson (cjwatson)]

I'm working up to adding the shellcheck pre-commit hook, but there are some other things that need to be fixed first - this is part of a series.

diff --git a/cronscripts/nightly.sh b/cronscripts/nightly.sh
index 9c4a674..78cf8e9 100755
--- a/cronscripts/nightly.sh
+++ b/cronscripts/nightly.sh
@@ -14,38 +14,38 @@ LOGFILE=$LOGDIR/nightly.log
 LOCK=/var/lock/launchpad_nightly.lock
 lockfile -r0 -l 259200 $LOCK
 if [ $? -ne 0 ]; then
-    echo $(date): Unable to grab $LOCK lock - aborting | tee -a $LOGFILE
+    echo "$(date): Unable to grab $LOCK lock - aborting" | tee -a "$LOGFILE"
     ps fuxwww
     exit 1
 fi
 
-cd `dirname $0`
+cd "$(dirname "$0")"
 
-echo $(date): Grabbed lock >> $LOGFILE
+echo "$(date): Grabbed lock" >> "$LOGFILE"
 
-echo $(date): Expiring memberships >> $LOGFILE
-./flag-expired-memberships.py -q --log-file=DEBUG:$LOGDIR/flag-expired-memberships.log
+echo "$(date): Expiring memberships" >> "$LOGFILE"
+./flag-expired-memberships.py -q --log-file=DEBUG:"$LOGDIR/flag-expired-memberships.log"
 
-echo $(date): Allocating revision karma >> $LOGFILE
-./allocate-revision-karma.py -q --log-file=DEBUG:$LOGDIR/allocate-revision-karma.log
+echo "$(date): Allocating revision karma" >> "$LOGFILE"
+./allocate-revision-karma.py -q --log-file=DEBUG:"$LOGDIR/allocate-revision-karma.log"
 
-echo $(date): Recalculating karma >> $LOGFILE
-./foaf-update-karma-cache.py -q --log-file=INFO:$LOGDIR/foaf-update-karma-cache.log
+echo "$(date): Recalculating karma" >> "$LOGFILE"
+./foaf-update-karma-cache.py -q --log-file=INFO:"$LOGDIR/foaf-update-karma-cache.log"
 
-echo $(date): Updating cached statistics >> $LOGFILE
-./update-stats.py -q --log-file=DEBUG:$LOGDIR/update-stats.log
+echo "$(date): Updating cached statistics" >> "$LOGFILE"
+./update-stats.py -q --log-file=DEBUG:"$LOGDIR/update-stats.log"
 
-echo $(date): Expiring questions >> $LOGFILE
-./expire-questions.py -q --log-file=DEBUG:$LOGDIR/expire-questions.log
+echo "$(date): Expiring questions" >> "$LOGFILE"
+./expire-questions.py -q --log-file=DEBUG:"$LOGDIR/expire-questions.log"
 
-echo $(date): Updating bugtask target name caches >> $LOGFILE
-./update-bugtask-targetnamecaches.py -q --log-file=DEBUG:$LOGDIR/update-bugtask-targetnamecaches.log
+echo "$(date): Updating bugtask target name caches" >> "$LOGFILE"
+./update-bugtask-targetnamecaches.py -q --log-file=DEBUG:"$LOGDIR/update-bugtask-targetnamecaches.log"
 
-echo $(date): Updating personal standings >> $LOGFILE
-./update-standing.py -q --log-file=DEBUG:$LOGDIR/update-standing.log
+echo "$(date): Updating personal standings" >> "$LOGFILE"
+./update-standing.py -q --log-file=DEBUG:"$LOGDIR/update-standing.log"
 
-echo $(date): Updating CVE database >> $LOGFILE
-./update-cve.py -q --log-file=DEBUG:$LOGDIR/update-cve.log
+echo "$(date): Updating CVE database" >> "$LOGFILE"
+./update-cve.py -q --log-file=DEBUG:"$LOGDIR/update-cve.log"
 
-echo $(date): Removing lock >> $LOGFILE
+echo "$(date): Removing lock" >> "$LOGFILE"
 rm -f $LOCK
diff --git a/cronscripts/publishing/cron.daily-ppa b/cronscripts/publishing/cron.daily-ppa
index 20b1d74..e95937e 100755
--- a/cronscripts/publishing/cron.daily-ppa
+++ b/cronscripts/publishing/cron.daily-ppa
@@ -10,8 +10,8 @@ set -x
 LOCKFILEOPTIONS="-r-1"
 
 # Variables, lockfile and exit handler for PPA scripts.
-source `dirname $0`/cron.base-ppa.sh
+source "$(dirname "$0")/cron.base-ppa.sh"
 
 # Clear out empty and thus redundant dirs.
-find $PPAROOT -type d -empty | xargs -r rmdir
-find $P3AROOT -type d -empty | xargs -r rmdir
+find "$PPAROOT" -type d -empty | xargs -r rmdir
+find "$P3AROOT" -type d -empty | xargs -r rmdir
diff --git a/cronscripts/publishing/cron.publish-copy-archives b/cronscripts/publishing/cron.publish-copy-archives
index 7e6cad0..5c35e56 100755
--- a/cronscripts/publishing/cron.publish-copy-archives
+++ b/cronscripts/publishing/cron.publish-copy-archives
@@ -5,7 +5,7 @@
 
 # LPCONFIG will come from the environment so this script can run unaltered
 # on dogfood.
-if [ -z $LPCONFIG ]; then
+if [ -z "$LPCONFIG" ]; then
     echo LPCONFIG must be set to run this script.
     exit 1
 fi
diff --git a/cronscripts/publishing/cron.publish-ppa b/cronscripts/publishing/cron.publish-ppa
index 81ed9b1..7c93542 100755
--- a/cronscripts/publishing/cron.publish-ppa
+++ b/cronscripts/publishing/cron.publish-ppa
@@ -6,12 +6,16 @@
 set -x
 
 # Variables, lockfile and exit handler for PPA scripts.
-source `dirname $0`/cron.base-ppa.sh
+source "$(dirname "$0")/cron.base-ppa.sh"
 
-LPCURRENT=`dirname $0`/../..
+LPCURRENT="$(dirname "$0")/../.."
 
 for DISTRO_ARG in "$@"; do
-  $LPCURRENT/scripts/process-accepted.py -v --ppa $DISTRO_ARG
-  $LPCURRENT/scripts/publish-distro.py -v --ppa $DISTRO_ARG
-  $LPCURRENT/scripts/publish-distro.py -v --private-ppa $DISTRO_ARG
+  # We intentionally allow word-splitting of $DISTRO_ARG here.
+  # shellcheck disable=SC2086
+  "$LPCURRENT/scripts/process-accepted.py" -v --ppa $DISTRO_ARG
+  # shellcheck disable=SC2086
+  "$LPCURRENT/scripts/publish-distro.py" -v --ppa $DISTRO_ARG
+  # shellcheck disable=SC2086
+  "$LPCURRENT/scripts/publish-distro.py" -v --private-ppa $DISTRO_ARG
 done
diff --git a/scripts/update-version-info.sh b/scripts/update-version-info.sh
index 46c5d7c..76f0c20 100755
--- a/scripts/update-version-info.sh
+++ b/scripts/update-version-info.sh
@@ -13,7 +13,7 @@ if [ ! -e .git ]; then
     exit 1
 fi
 
-if ! which git > /dev/null || ! test -x $(which git); then
+if ! which git > /dev/null || ! test -x "$(which git)"; then
     echo "No working 'git' executable found" >&2
     exit 1
 fi
diff --git a/utilities/find-changed-files.sh b/utilities/find-changed-files.sh
index bdaedd2..530138e 100755
--- a/utilities/find-changed-files.sh
+++ b/utilities/find-changed-files.sh
@@ -15,7 +15,7 @@ if [ ! -e .git ]; then
 fi
 
 git_diff_files() {
-    git diff --name-only -z $@ | perl -l -0 -ne '
+    git diff --name-only -z "$@" | perl -l -0 -ne '
         # Only show paths that exist and are not symlinks.
         print if -e and not -l'
 }
@@ -28,4 +28,4 @@ if [ -z "$files" ]; then
     files=$(git_diff_files "${1:-master}")
 fi
 
-echo $files
+echo "$files"
diff --git a/utilities/launchpad-database-setup b/utilities/launchpad-database-setup
index 83b79b4..729f941 100755
--- a/utilities/launchpad-database-setup
+++ b/utilities/launchpad-database-setup
@@ -97,8 +97,8 @@ fi
 echo Waiting 10 seconds for postgresql to come up...
 sleep 10
 
-echo Creating postgresql user $USER
-sudo -u postgres /usr/lib/postgresql/$pgversion/bin/createuser -a -d $USER
+echo "Creating postgresql user $USER"
+sudo -u postgres /usr/lib/postgresql/$pgversion/bin/createuser -a -d "$USER"
 
 echo
 echo Looks like everything went ok.
diff --git a/utilities/rocketfuel-get b/utilities/rocketfuel-get
index 7335f4a..c41416e 100755
--- a/utilities/rocketfuel-get
+++ b/utilities/rocketfuel-get
@@ -30,7 +30,7 @@ fi
 
 LP_DOWNLOAD_CACHE_PATH="$LP_PROJECT_ROOT/$LP_SOURCEDEPS_DIR/download-cache"
 YUI_PATH="$LP_PROJECT_ROOT/$LP_SOURCEDEPS_DIR/yui"
-LP_DOWNLOAD_CACHE_PATH="$(eval echo ${LP_DOWNLOAD_CACHE_PATH})"
+LP_DOWNLOAD_CACHE_PATH="$(eval echo "$LP_DOWNLOAD_CACHE_PATH")"
 
 # Pull launchpad devel from launchpad.
 INITIAL_REV=$(git -C "$LP_TRUNK_PATH" rev-parse HEAD)
@@ -94,7 +94,7 @@ find_branches_to_relink | filter_branches_to_relink | relink_branches
 
 
 # Build launchpad if there were changes.
-if [ $FINAL_REV != $INITIAL_REV ];
+if [ "$FINAL_REV" != "$INITIAL_REV" ]
 then
     make -C "$LP_TRUNK_PATH"
 fi
diff --git a/utilities/rocketfuel-setup b/utilities/rocketfuel-setup
index 0e003d5..78be81e 100755
--- a/utilities/rocketfuel-setup
+++ b/utilities/rocketfuel-setup
@@ -33,12 +33,12 @@ while :; do
     esac
 done
 
-if [ -z ${lpusername} ]; then
+if [ -z "$lpusername" ]; then
   # Establish LP username
   whoami=`whoami`
   printf "What is your Launchpad username? [$whoami] "
   read lpusername
-  if [ -z ${lpusername} ]; then
+  if [ -z "$lpusername" ]; then
     lpusername=${whoami}
   fi
 fi
@@ -134,10 +134,10 @@ for key in $REQUIRED_PPA_KEYS; do
 done
 
 do_install() {
-  dpkg -s $pkg | grep -q "^Status: install ok installed"
+  dpkg -s "$pkg" | grep -q "^Status: install ok installed"
   if [ $? -ne 0 ]; then
     echo "Installing $pkg package..."
-    sudo apt-get install $INSTALL_OPTS $pkg
+    sudo apt-get install $INSTALL_OPTS "$pkg"
     if [ $? -ne 0 ]; then
       echo "Unable to install $pkg."
       exit 1
@@ -249,11 +249,11 @@ fi
 
 
 # Create the local branch structure we will use for managing Launchpad code
-mkdir -p $LP_PROJECT_ROOT
-cd $LP_PROJECT_ROOT
+mkdir -p "$LP_PROJECT_ROOT"
+cd "$LP_PROJECT_ROOT"
 
 echo "Logging bzr into Launchpad (it's okay if this errors)..."
-bzr launchpad-login $lpusername
+bzr launchpad-login "$lpusername"
 if [ $? -ne 0 ]; then
   echo ""
   echo "You can ignore any error above about not registering an SSH key"
@@ -275,17 +275,17 @@ if [ "$(git ls-remote --get-url lp:launchpad)" = lp:launchpad ]; then
   fi
 fi
 
-cd $LP_PROJECT_ROOT
-if [ ! -d $LP_TRUNK_NAME ]; then
+cd "$LP_PROJECT_ROOT"
+if [ ! -d "$LP_TRUNK_NAME" ]; then
   echo "Making local clone of Launchpad; this may take a while..."
-  git clone lp:launchpad $LP_TRUNK_NAME
+  git clone lp:launchpad "$LP_TRUNK_NAME"
   if [ $? -ne 0 ]; then
     echo "ERROR: Unable to create local clone of Launchpad"
     exit 1
   fi
 fi
 
-cd $LP_TRUNK_NAME
+cd "$LP_TRUNK_NAME"
 git status
 if [ $? -ne 0 ]; then
   echo "ERROR: Your clone in $LP_TRUNK_PATH is corrupted.
@@ -313,7 +313,7 @@ fi
 # Set up scripts in /usr/local/bin
 cd /usr/local/bin
 if [ ! -e rocketfuel-get ]; then
-  sudo ln -s $LP_TRUNK_PATH/utilities/rocketfuel-get
+  sudo ln -s "$LP_TRUNK_PATH/utilities/rocketfuel-get"
 fi
 ls -l rocketfuel-get | cut -d">" -f2 | grep -q "$LP_TRUNK_NAME\/utilities\/rocketfuel"
 if [ $? -ne 0 ]; then
@@ -321,7 +321,7 @@ if [ $? -ne 0 ]; then
          recreate it."
 fi
 if [ ! -e rocketfuel-setup ]; then
-  sudo ln -s $LP_TRUNK_PATH/utilities/rocketfuel-setup
+  sudo ln -s "$LP_TRUNK_PATH/utilities/rocketfuel-setup"
 fi
 ls -l rocketfuel-setup | cut -d">" -f2 | grep -q "$LP_TRUNK_NAME\/utilities\/rocketfuel"
 if [ $? -ne 0 ]; then
diff --git a/utilities/start-dev-soyuz.sh b/utilities/start-dev-soyuz.sh
index efe4b0f..2517b5a 100755
--- a/utilities/start-dev-soyuz.sh
+++ b/utilities/start-dev-soyuz.sh
@@ -11,7 +11,7 @@ start_twistd() {
     bin/twistd \
         --logfile "/var/tmp/development-$name.log" \
         --pidfile "/var/tmp/development-$name.pid" \
-        -y "$tac" $@
+        -y "$tac" "$@"
 }
 
 start_twistd_plugin() {