From 79ef9b4efc307aa0aebfbeb5b0c120a3ad525399 Mon Sep 17 00:00:00 2001
From: Aneesh Agrawal <aneeshusa@gmail.com>
Date: Thu, 4 Aug 2016 15:16:04 -0400
Subject: [PATCH 1/3] Add lint for backticks in shell scripts

The "$(some_command arg1 arg2)" form is preferred to the
`some_command arg1 arg2` form because it nests unambiguously.
Add a lint for this to tidy.
---
 ports/geckolib/gecko_bindings/tools/regen.sh  |  4 +--
 .../gecko_bindings/tools/setup_bindgen.sh     |  4 +--
 python/tidy/servo_tidy/tidy.py                | 27 ++++++++++++-------
 python/tidy/servo_tidy_tests/shell_tidy.sh    |  2 ++
 python/tidy/servo_tidy_tests/test_tidy.py     |  1 +
 5 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/ports/geckolib/gecko_bindings/tools/regen.sh b/ports/geckolib/gecko_bindings/tools/regen.sh
index 2d02bebdb34..21b998141c9 100755
--- a/ports/geckolib/gecko_bindings/tools/regen.sh
+++ b/ports/geckolib/gecko_bindings/tools/regen.sh
@@ -27,9 +27,9 @@ if [ ! -d /usr/include ]; then
 fi
 
 if [ "$(uname)" == "Linux" ]; then
-  LIBCLANG_PATH=/usr/lib/llvm-3.8/lib;
+  LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
-  LIBCLANG_PATH=`brew --prefix llvm38`/lib/llvm-3.8/lib;
+  LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
 ./regen.py --target all "$@"
diff --git a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
index d42bfc9adb4..31a81111b6d 100755
--- a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
+++ b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
@@ -13,9 +13,9 @@ cd "$(dirname $0)"
 
 # Setup and build bindgen.
 if [ "$(uname)" == "Linux" ]; then
-  export LIBCLANG_PATH=/usr/lib/llvm-3.8/lib;
+  export LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
-  export LIBCLANG_PATH=`brew --prefix llvm38`/lib/llvm-3.8/lib;
+  export LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
 # Make sure we have llvm38.
diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index 34eea7f882a..d47aa0137a0 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -318,19 +318,23 @@ def check_shell(file_name, lines):
     shebang = "#!/usr/bin/env bash"
     required_options = {"set -o errexit", "set -o nounset", "set -o pipefail"}
 
+    did_shebang_check = False
+
     if len(lines) == 0:
         yield (0, 'script is an empty file')
-    else:
-        if lines[0].rstrip() != shebang:
-            yield (1, 'script does not have shebang "{}"'.format(shebang))
+        return
 
-        for idx in range(1, len(lines)):
-            stripped = lines[idx].rstrip()
+    if lines[0].rstrip() != shebang:
+        yield (1, 'script does not have shebang "{}"'.format(shebang))
 
-            # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
-            if lines[idx].startswith("#") or stripped == "":
-                continue
-            elif stripped in required_options:
+    for idx in range(1, len(lines)):
+        stripped = lines[idx].rstrip()
+        # Comments or blank lines are ignored. (Trailing whitespace is caught with a separate linter.)
+        if lines[idx].startswith("#") or stripped == "":
+            continue
+
+        if not did_shebang_check:
+            if stripped in required_options:
                 required_options.remove(stripped)
             else:
                 # The first non-comment, non-whitespace, non-option line is the first "real" line of the script.
@@ -338,7 +342,10 @@ def check_shell(file_name, lines):
                 if len(required_options) != 0:
                     formatted = ['"{}"'.format(opt) for opt in required_options]
                     yield (idx + 1, "script is missing options {}".format(", ".join(formatted)))
-                    break
+                did_shebang_check = True
+
+        if "`" in stripped:
+            yield (idx + 1, "script should not use backticks for command substitution")
 
 
 def check_rust(file_name, lines):
diff --git a/python/tidy/servo_tidy_tests/shell_tidy.sh b/python/tidy/servo_tidy_tests/shell_tidy.sh
index fbf0e784755..ff8e5fc7240 100644
--- a/python/tidy/servo_tidy_tests/shell_tidy.sh
+++ b/python/tidy/servo_tidy_tests/shell_tidy.sh
@@ -4,4 +4,6 @@
 
 set -o nounset
 
+# Talking about some `concept in backticks` # shouldn't trigger
 echo "hello world"
+some_var=`echo "command substitution"`
diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py
index b08e6057541..58e8f6e5fd3 100644
--- a/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/python/tidy/servo_tidy_tests/test_tidy.py
@@ -52,6 +52,7 @@ class CheckTidiness(unittest.TestCase):
         errors = tidy.collect_errors_for_files(iterFile('shell_tidy.sh'), [], [tidy.check_shell], print_text=False)
         self.assertEqual('script does not have shebang "#!/usr/bin/env bash"', errors.next()[2])
         self.assertEqual('script is missing options "set -o errexit", "set -o pipefail"', errors.next()[2])
+        self.assertEqual('script should not use backticks for command substitution', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_rust(self):

From 9231ca1c6991c2eaf0d7e08674b2fc9c84a31695 Mon Sep 17 00:00:00 2001
From: Aneesh Agrawal <aneeshusa@gmail.com>
Date: Thu, 4 Aug 2016 18:12:54 -0400
Subject: [PATCH 2/3] Add lint to ensure substitutions use the full form

Check that any variable substitutions use the full ${VAR} form,
not just $VAR (but don't check for quoting yet).
---
 components/servo/fake-ld.sh                          | 2 +-
 etc/ci/lockfile_changed.sh                           | 4 ++--
 etc/ci/manifest_changed.sh                           | 4 ++--
 etc/ci/upload_docs.sh                                | 2 +-
 etc/ci/upload_nightly.sh                             | 4 ++--
 ports/geckolib/gecko_bindings/tools/regen.sh         | 6 +++---
 ports/geckolib/gecko_bindings/tools/setup_bindgen.sh | 6 +++---
 python/tidy/servo_tidy/tidy.py                       | 7 +++++++
 python/tidy/servo_tidy_tests/shell_tidy.sh           | 1 +
 python/tidy/servo_tidy_tests/test_tidy.py            | 1 +
 10 files changed, 23 insertions(+), 14 deletions(-)

diff --git a/components/servo/fake-ld.sh b/components/servo/fake-ld.sh
index f14e94ea8c9..d0dd042dbf4 100755
--- a/components/servo/fake-ld.sh
+++ b/components/servo/fake-ld.sh
@@ -9,7 +9,7 @@ set -o nounset
 set -o pipefail
 
 TARGET_DIR="${OUT_DIR}/../../.."
-arm-linux-androideabi-gcc "$@" \
+arm-linux-androideabi-gcc "${@}" \
                           "${LDFLAGS-}" -lc -shared \
                           -o "${TARGET_DIR}/libservo.so"
 touch "${TARGET_DIR}/servo"
diff --git a/etc/ci/lockfile_changed.sh b/etc/ci/lockfile_changed.sh
index 1d11acee473..6b3191291cc 100755
--- a/etc/ci/lockfile_changed.sh
+++ b/etc/ci/lockfile_changed.sh
@@ -9,5 +9,5 @@ set -o nounset
 set -o pipefail
 
 diff="$(git diff -- */*/Cargo.lock)"
-echo "$diff"
-[[ ! $diff ]]
+echo "${diff}"
+[[ -z "${diff}" ]]
diff --git a/etc/ci/manifest_changed.sh b/etc/ci/manifest_changed.sh
index 195e4b9ef7e..24fc9baec6c 100755
--- a/etc/ci/manifest_changed.sh
+++ b/etc/ci/manifest_changed.sh
@@ -17,5 +17,5 @@ set -o pipefail
 ./mach test-wpt --manifest-update --binary= SKIP_TESTS > /dev/null
 
 diff="$(git diff -- tests/*/MANIFEST.json)"
-echo "$diff"
-[[ ! $diff ]]
+echo "${diff}"
+[[ -z "${diff}" ]]
diff --git a/etc/ci/upload_docs.sh b/etc/ci/upload_docs.sh
index fc8d7416f65..f55e5cb73db 100755
--- a/etc/ci/upload_docs.sh
+++ b/etc/ci/upload_docs.sh
@@ -12,7 +12,7 @@ set -o errexit
 set -o nounset
 set -o pipefail
 
-cd "$(dirname $0)/../.."
+cd "$(dirname ${0})/../.."
 
 ./mach doc
 # etc/doc.servo.org/index.html overwrites $(mach rust-root)/doc/index.html
diff --git a/etc/ci/upload_nightly.sh b/etc/ci/upload_nightly.sh
index 4f20ab658d4..bd6da3e6625 100755
--- a/etc/ci/upload_nightly.sh
+++ b/etc/ci/upload_nightly.sh
@@ -27,7 +27,7 @@ upload() {
 
 
 main() {
-    if [[ "$#" != 1 ]]; then
+    if [[ "${#}" != 1 ]]; then
         usage >&2
         return 1
     fi
@@ -58,4 +58,4 @@ main() {
     upload "${platform}" ${package} "${extension}"
 }
 
-main "$@"
+main "${@}"
diff --git a/ports/geckolib/gecko_bindings/tools/regen.sh b/ports/geckolib/gecko_bindings/tools/regen.sh
index 21b998141c9..7bbd908c1f3 100755
--- a/ports/geckolib/gecko_bindings/tools/regen.sh
+++ b/ports/geckolib/gecko_bindings/tools/regen.sh
@@ -8,8 +8,8 @@ set -o errexit
 set -o nounset
 set -o pipefail
 
-if [ $# -eq 0 ]; then
-  echo "Usage: $0 /path/to/gecko/objdir [other-regen.py-flags]"
+if [ ${#} -eq 0 ]; then
+  echo "Usage: ${0} /path/to/gecko/objdir [other-regen.py-flags]"
   exit 1
 fi
 
@@ -32,4 +32,4 @@ else
   LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
-./regen.py --target all "$@"
+./regen.py --target all "${@}"
diff --git a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
index 31a81111b6d..2c6c8831697 100755
--- a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
+++ b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
@@ -9,7 +9,7 @@ set -o nounset
 set -o pipefail
 
 # Run in the tools directory.
-cd "$(dirname $0)"
+cd "$(dirname ${0})"
 
 # Setup and build bindgen.
 if [ "$(uname)" == "Linux" ]; then
@@ -25,8 +25,8 @@ if [ ! -x "$(command -v clang-3.8)" ]; then
     exit 1
 fi
 
-export LD_LIBRARY_PATH=$LIBCLANG_PATH
-export DYLD_LIBRARY_PATH=$LIBCLANG_PATH
+export LD_LIBRARY_PATH="${LIBCLANG_PATH}"
+export DYLD_LIBRARY_PATH="${LIBCLANG_PATH}"
 
 # Check for multirust
 if [ ! -x "$(command -v multirust)" ]; then
diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index d47aa0137a0..50fd7ca8a5c 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -347,6 +347,13 @@ def check_shell(file_name, lines):
         if "`" in stripped:
             yield (idx + 1, "script should not use backticks for command substitution")
 
+        for dollar in re.finditer('\$', stripped):
+            next_idx = dollar.end()
+            if next_idx < len(stripped):
+                next_char = stripped[next_idx]
+                if not (next_char == '{' or next_char == '('):
+                    yield(idx + 1, "variable substitutions should use the full \"${VAR}\" form")
+
 
 def check_rust(file_name, lines):
     if not file_name.endswith(".rs") or \
diff --git a/python/tidy/servo_tidy_tests/shell_tidy.sh b/python/tidy/servo_tidy_tests/shell_tidy.sh
index ff8e5fc7240..e24b361fe16 100644
--- a/python/tidy/servo_tidy_tests/shell_tidy.sh
+++ b/python/tidy/servo_tidy_tests/shell_tidy.sh
@@ -7,3 +7,4 @@ set -o nounset
 # Talking about some `concept in backticks` # shouldn't trigger
 echo "hello world"
 some_var=`echo "command substitution"`
+another_var="$some_var"
diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py
index 58e8f6e5fd3..2eee04a19a3 100644
--- a/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/python/tidy/servo_tidy_tests/test_tidy.py
@@ -53,6 +53,7 @@ class CheckTidiness(unittest.TestCase):
         self.assertEqual('script does not have shebang "#!/usr/bin/env bash"', errors.next()[2])
         self.assertEqual('script is missing options "set -o errexit", "set -o pipefail"', errors.next()[2])
         self.assertEqual('script should not use backticks for command substitution', errors.next()[2])
+        self.assertEqual('variable substitutions should use the full \"${VAR}\" form', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_rust(self):

From f07d8f188a0621545957a0f348271d29b08704ec Mon Sep 17 00:00:00 2001
From: Aneesh Agrawal <aneeshusa@gmail.com>
Date: Fri, 5 Aug 2016 09:42:04 -0400
Subject: [PATCH 3/3] Add lint for `[` instead of `[[` in shells scripts

---
 ports/geckolib/gecko_bindings/tools/regen.sh         | 8 ++++----
 ports/geckolib/gecko_bindings/tools/setup_bindgen.sh | 8 ++++----
 python/tidy/servo_tidy/tidy.py                       | 3 +++
 python/tidy/servo_tidy_tests/shell_tidy.sh           | 4 ++++
 python/tidy/servo_tidy_tests/test_tidy.py            | 2 ++
 5 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/ports/geckolib/gecko_bindings/tools/regen.sh b/ports/geckolib/gecko_bindings/tools/regen.sh
index 7bbd908c1f3..fc2ed08a833 100755
--- a/ports/geckolib/gecko_bindings/tools/regen.sh
+++ b/ports/geckolib/gecko_bindings/tools/regen.sh
@@ -8,25 +8,25 @@ set -o errexit
 set -o nounset
 set -o pipefail
 
-if [ ${#} -eq 0 ]; then
+if [[ ${#} -eq 0 ]]; then
   echo "Usage: ${0} /path/to/gecko/objdir [other-regen.py-flags]"
   exit 1
 fi
 
 # Check for rust-bindgen
-if [ ! -d rust-bindgen ]; then
+if [[ ! -d rust-bindgen ]]; then
   echo "rust-bindgen not found. Run setup_bindgen.sh first."
   exit 1
 fi
 
 # Check for /usr/include
-if [ ! -d /usr/include ]; then
+if [[ ! -d /usr/include ]]; then
   echo "/usr/include doesn't exist." \
        "Mac users may need to run xcode-select --install."
   exit 1
 fi
 
-if [ "$(uname)" == "Linux" ]; then
+if [[ "$(uname)" == "Linux" ]]; then
   LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
   LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
diff --git a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
index 2c6c8831697..4d1b67e3f43 100755
--- a/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
+++ b/ports/geckolib/gecko_bindings/tools/setup_bindgen.sh
@@ -12,14 +12,14 @@ set -o pipefail
 cd "$(dirname ${0})"
 
 # Setup and build bindgen.
-if [ "$(uname)" == "Linux" ]; then
+if [[ "$(uname)" == "Linux" ]]; then
   export LIBCLANG_PATH=/usr/lib/llvm-3.8/lib
 else
   export LIBCLANG_PATH="$(brew --prefix llvm38)/lib/llvm-3.8/lib"
 fi
 
 # Make sure we have llvm38.
-if [ ! -x "$(command -v clang-3.8)" ]; then
+if [[ ! -x "$(command -v clang-3.8)" ]]; then
     echo "llmv38 must be installed." \
          "Mac users should |brew install llvm38|, Linux varies by distro."
     exit 1
@@ -29,13 +29,13 @@ export LD_LIBRARY_PATH="${LIBCLANG_PATH}"
 export DYLD_LIBRARY_PATH="${LIBCLANG_PATH}"
 
 # Check for multirust
-if [ ! -x "$(command -v multirust)" ]; then
+if [[ ! -x "$(command -v multirust)" ]]; then
     echo "multirust must be installed."
     exit 1
 fi
 
 # Don't try to clone twice.
-if [ ! -d rust-bindgen ]; then
+if [[ ! -d rust-bindgen ]]; then
   git clone https://github.com/servo/rust-bindgen.git
 fi
 
diff --git a/python/tidy/servo_tidy/tidy.py b/python/tidy/servo_tidy/tidy.py
index 50fd7ca8a5c..3433e8265ed 100644
--- a/python/tidy/servo_tidy/tidy.py
+++ b/python/tidy/servo_tidy/tidy.py
@@ -347,6 +347,9 @@ def check_shell(file_name, lines):
         if "`" in stripped:
             yield (idx + 1, "script should not use backticks for command substitution")
 
+        if " [ " in stripped or stripped.startswith("[ "):
+            yield (idx + 1, "script should use `[[` instead of `[` for conditional testing")
+
         for dollar in re.finditer('\$', stripped):
             next_idx = dollar.end()
             if next_idx < len(stripped):
diff --git a/python/tidy/servo_tidy_tests/shell_tidy.sh b/python/tidy/servo_tidy_tests/shell_tidy.sh
index e24b361fe16..e38358fc3b6 100644
--- a/python/tidy/servo_tidy_tests/shell_tidy.sh
+++ b/python/tidy/servo_tidy_tests/shell_tidy.sh
@@ -8,3 +8,7 @@ set -o nounset
 echo "hello world"
 some_var=`echo "command substitution"`
 another_var="$some_var"
+if [ -z "${some_var}" ]; then
+  echo "should have used [["
+fi
+[ -z "${another_var}" ]
diff --git a/python/tidy/servo_tidy_tests/test_tidy.py b/python/tidy/servo_tidy_tests/test_tidy.py
index 2eee04a19a3..92b304a5c1a 100644
--- a/python/tidy/servo_tidy_tests/test_tidy.py
+++ b/python/tidy/servo_tidy_tests/test_tidy.py
@@ -54,6 +54,8 @@ class CheckTidiness(unittest.TestCase):
         self.assertEqual('script is missing options "set -o errexit", "set -o pipefail"', errors.next()[2])
         self.assertEqual('script should not use backticks for command substitution', errors.next()[2])
         self.assertEqual('variable substitutions should use the full \"${VAR}\" form', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
+        self.assertEqual('script should use `[[` instead of `[` for conditional testing', errors.next()[2])
         self.assertNoMoreErrors(errors)
 
     def test_rust(self):