Refactor hooks, add opt-in pre-push hook.

See hooks/README.md for implementation notes.

Motivtion: the old hooks are hard to run manually, and code reuse among
hooks is difficult. Goal of this refactor is to cleanly split the steps
involved:

- determine set of changed files
- run checks on specific files
- report issues based on check results

The old logic was based on running checks on individual files, printing
"1" on STDOUT for bad files. Now, check-* scripts take a list of files
as args and print filenames of bad files on STDOUT. A toplevel "check"
script drives these individual checks and contains common hook logic
such as problem reporting.  The toplevel hooks themselves use the
"check" script to take appropriate actions.

Also change checks to use strict `-euo pipefail` error handling where
possible, and standardize variable naming. (Uppercase for top-of-file
constants or vars exported to the environment, otherwise lowercase.)

hooks/README.md

@@ -10,6 +10,44 @@ Install them by running:
 
 from the root of the repository. This will symlink the hooks into `.git/hooks/`.
 
+The pre-push hook is currently opt-in. To enable it, set the `PKB_BASE`
+environment variable to the name of a base branch, for example
+`PKB_BASE=upstream/dev` for a fork, or `PKB_BASE=origin/dev` when
+working on the main repository.
+
+You can also run checks manually:
+
+```bash
+# Run all checks on all files under version control.
+hooks/check-everything
+
+# Run all checks on a specific set of files.
+hooks/check FILE ...
+```
+
 [1]: http://git-scm.com/docs/githooks
 [2]: https://github.com/GoogleCloudPlatform/kubernetes
 [3]: https://pypi.python.org/pypi/flake8
+
+
+# Implementation notes
+
+The hook implementation is split into three layers:
+
+- Toplevel hook scripts determine which files need to be checked, run the
+  `hooks/check` script on them as needed, and report back errors. They use a
+  nonzero return code when actions should be blocked (`commit-msg` or
+  `pre-push`), and do other actions where needed. (`prepare-commit-msg` modifies
+  the message.)
+
+- The `hooks/check` script expects a list of files to check as arguments, and
+  runs individual `lib/check-*` scripts on them. It prints optional
+  human-readable diagnostics on STDERR, a formatted report on STDOUT (including
+  offending filenames where available), and returns a nonzero exit code if
+  any checks failed.
+
+- `lib/check-*` scripts take a list of files to check as arguments. They can
+  report file-specific issues by printing a list of bad files on STDOUT along
+  with a zero exit code, or exit with a nonzero exit code if they find issues
+  that can't be associated with a specific file such as a failed unit test or
+  invalid testing setup.

hooks/check

@@ -0,0 +1,74 @@
+#!/bin/bash
+
+# Copyright 2015 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -euo pipefail
+
+HOOKS_DIR="$(dirname "$(test -L "$0" && echo "$(dirname $0)/$(readlink "$0")" || echo "$0")")"
+
+# The prepare-commit-msg hook sets an environment variable to indicate
+# that we should add details to the report. Set this to an empty
+# string if unset to avoid "unbound variable" failures.
+PREPARE_COMMIT_HOOK="${PREPARE_COMMIT_HOOK:-}"
+
+# Execute all the checks first, they'll print informational messages on stderr.
+status=0
+
+if ! "${HOOKS_DIR}/lib/check-unittest.sh"; then
+  echo "*** ERROR: *** Unit test failure."
+  if [ -n "$PREPARE_COMMIT_HOOK" ]; then
+    echo "Your commit will be aborted unless you fix these."
+    echo "  COMMIT_BLOCKED_ON_UNIT_TESTS"
+  fi
+  status=1
+fi
+
+files_need_boilerplate=($("${HOOKS_DIR}/lib/check-boilerplate.sh" "$@"))
+
+files_with_lint_errors=($("${HOOKS_DIR}/lib/check-lint.sh" "$@"))
+
+# Now print reports for all collected errors to ensure that they are
+# all visible together. This way, they won't be scrolled offscreen
+# by diagnostics.
+
+if [[ "${#files_need_boilerplate[@]}" -ne 0 ]]; then
+  echo
+  echo "*** ERROR: *** Some files are missing the required boilerplate"
+  echo "header from hooks/boilerplate.txt:"
+  for file in "${files_need_boilerplate[@]}"; do
+    echo "  ${file}"
+  done
+  echo "See hooks/boilerplate.*.txt for required headers."
+  if [ -n "$PREPARE_COMMIT_HOOK" ]; then
+    echo "Your commit will be aborted unless you fix these."
+    echo "  COMMIT_BLOCKED_ON_BOILERPLATE"
+  fi
+  status=1
+fi
+
+if [[ "${#files_with_lint_errors[@]}" -ne 0 ]]; then
+  echo
+  echo "*** ERROR: *** Some files have lint errors:"
+  for file in "${files_with_lint_errors[@]}"; do
+    echo "  ${file}"
+  done
+  if [ -n "$PREPARE_COMMIT_HOOK" ]; then
+    echo "Your commit will be aborted unless you fix these."
+    echo "  COMMIT_BLOCKED_ON_LINT"
+  fi
+  status=1
+fi
+
+exit $status

hooks/lint.py.sh → hooks/check-everything

@@ -1,6 +1,6 @@
 #!/bin/bash
 
-# Copyright 2014 Google Inc. All rights reserved.
+# Copyright 2015 Google Inc. All rights reserved.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -14,15 +14,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-set -o errexit
-set -o nounset
-set -o pipefail
+set -euo pipefail
 
-readonly FILE=$1
+HOOKS_DIR="$(dirname "$(test -L "$0" && echo "$(dirname $0)/$(readlink "$0")" || echo "$0")")"
 
-if [[ -z "$(command -v flake8)" ]]; then
-  >&2 echo "Missing flake8. Install it via 'pip' to enable linting."
-  exit 1
+all_files=($(git ls-files))
+if [[ "${#all_files[0]}" -ne 0 ]]; then
+  "${HOOKS_DIR}/check" "${all_files[@]}"
 fi
-
-flake8 $1 2>&1

hooks/commit-msg

@@ -14,10 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-if [[ "$(grep -c "COMMIT_BLOCKED" $1)" -gt 0 ]]; then
+if [[ "$(grep -c "COMMIT_BLOCKED" "$1")" -gt 0 ]]; then
   echo "FAILED: Unresolved errors - aborting the commit."
   echo "The message of your attempted commit was:"
-  cat $1
+  cat "$1"
   exit 1
 fi
 

hooks/install.sh

@@ -21,7 +21,7 @@ set -o nounset
 set -o pipefail
 
 readonly HOOKS_DIR="$(dirname "$(test -L "$0" && echo "$(dirname $0)/$(readlink "$0")" || echo "$0")")"
-readonly HOOK_FILES=(prepare-commit-msg commit-msg)
+readonly HOOK_FILES=(prepare-commit-msg commit-msg pre-push)
 
 pushd $HOOKS_DIR/.. > /dev/null
 

hooks/lib/check-boilerplate.sh

@@ -0,0 +1,68 @@
+#!/bin/bash
+
+# Copyright 2015 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -euo pipefail
+
+HOOKS_LIB="$(dirname "$(test -L "$0" && echo "$(dirname $0)/$(readlink "$0")" || echo "$0")")"
+
+# Check for files without the required boilerplate.
+for file in "$@"; do
+  ext=${file##*.}
+  ref_file="${HOOKS_LIB}/boilerplate.${ext}.txt"
+
+  if [ ! -e $ref_file ]; then
+    continue
+  fi
+
+  lines=$(cat "${ref_file}" | wc -l | tr -d ' ')
+  if [[ "${ext}" == "py" && -x "${file}" ]]; then
+    # remove shabang and blank line from top of executable python files.
+    header=$(cat "${file}" | tail --lines=+3 | head "-${lines}")
+  else
+    header=$(head "-${lines}" "${file}")
+  fi
+
+  # Treat errors from diff other than the usual "files differ"
+  # code 1 as fatal problems.
+  differ=$(echo "${header}" | diff - "${ref_file}" || [[ $? -le 1 ]])
+
+  if [[ -z "${differ}" ]]; then
+    # Header does not differ.
+    continue
+  fi
+
+  if [ "$(echo "${differ}" | wc -l)" -eq 4 ]; then
+    # Header differs by one line. Check if only copyright date differs. If so,
+    # the contents of "${differ}" should resemble:
+    #     1c1
+    #     < # Copyright 2015 Google Inc. All rights reserved.
+    #     ---
+    #     > # Copyright 2014 Google Inc. All rights reserved.
+    header_line='# Copyright [0-9]+ Google Inc[.] All rights reserved[.]'
+    diff_prefix='[[:alnum:]]+[[:space:]]*< '
+    diff_middle='[[:space:]]*---[[:space:]]*> '
+    diff_pattern="^${diff_prefix}${header_line}${diff_middle}${header_line}\$"
+    if [[ "${differ}" =~ $diff_pattern ]]; then
+      # Change matches acceptable variation.
+      continue
+    fi
+  fi
+
+  # Report file as invalid boilerplate.
+  echo "$file"
+done
+
+exit 0

hooks/lib/check-lint.sh

@@ -0,0 +1,36 @@
+#!/bin/bash
+
+# Copyright 2015 Google Inc. All rights reserved.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+set -euo pipefail
+
+if [[ -z "$(command -v flake8)" ]]; then
+  >&2 echo "Missing flake8. Install it via 'pip' to enable linting."
+  exit 1
+fi
+
+# Treat errors from grep other than the "not found" code 1 as fatal problems.
+modified_py=($(printf -- '%s\n' "$@" | grep '\.py$' || [[ $? -le 1 ]]))
+
+# Don't run linter with no arguments, that would check all files.
+if [[ "${#modified_py[@]}" -ne 0 ]]; then
+  # First, run flake with normal output so that the user sees errors.
+  # If there are problems, re-run flake8, printing filenames only.
+  if ! flake8 "${modified_py[@]}" >&2; then
+    flake8 -q "${modified_py[@]}" || true
+  fi
+fi
+
+exit 0

hooks/lint.sh → hooks/lib/check-unittest.sh

@@ -1,6 +1,6 @@
 #!/bin/bash
 
-# Copyright 2014 Google Inc. All rights reserved.
+# Copyright 2015 Google Inc. All rights reserved.
 #
 # Licensed under the Apache License, Version 2.0 (the "License");
 # you may not use this file except in compliance with the License.
@@ -14,23 +14,14 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-# Print 1 if the file in $1 passes linting, 0 otherwise
-FILE=$1
-EXT=${FILE##*.}
+set -euo pipefail
 
-LINTER="$(dirname $0)/lint.${EXT}.sh"
-
-if [ ! -e "$LINTER" ]; then
-  echo "1"
-  exit 0
-fi
-
-LINT=$($LINTER $FILE)
-if [[ ! -z "$LINT" ]]; then
-  >&2 echo "$LINT"
-  echo "0"
-  exit 0
+if [[ -z $(command -v tox) ]]; then
+  >&2 echo "Missing tox. Install it via 'pip' to enable unit testing."
+  exit 1
 fi
 
-echo "1"
-exit 0
+# Unit test failures can't easily be mapped to specific input files.
+# Always run all tests, and report a failure via nonzero exit code if
+# any test fails.
+tox -e py27 >&2