toolchain-utils: swap git hooks to use yapf

This updates a few things:
- Migrates our git hooks to directly use yapf for formatting (which
  appears to be the direction that Chromium is heading in, and is
  available by default both inside and outside of the chroot: Chromium's
  depot_tools has a copy of it. pyformat is only available internally,
  and is basically a wrapper around yapf, except for a few things.
  Please see CL 1392445 for more),
- Splits out the format checks into its own script, so it's easier to
  potentially run from e.g. repo (don't quote me on that quite yet,
  though ;) )
- Removes the now-seemingly-unused tc_format script

BUG=chromium:918755
TEST=- ./check-format on a poorly formatted Python file
     - ./check-format on a python file with a #! that shouldn't have had
       it
     - ./check-format on a python file without a #! that should have had
       it
     - ./check-format on toolchain-utils/*.py; results were overall
       reasonable
     All fixes it suggested worked as intended.
CQ-DEPEND=CL:1392445

Change-Id: Idabc2658c5dedcc70b6d3cc6c1acfd6c83eabc29
Reviewed-on: https://chromium-review.googlesource.com/1394285
Commit-Ready: George Burgess <gbiv@chromium.org>
Tested-by: George Burgess <gbiv@chromium.org>
Reviewed-by: Manoj Gupta <manojgupta@chromium.org>
Reviewed-by: Luis Lozano <llozano@chromium.org>

3 files changed, 109 insertions, 59 deletions

diff --git a/bin/tc_pyformat b/bin/tc_pyformat
deleted file mode 100755
index 21eceebe..00000000
--- a/bin/tc_pyformat
+++ /dev/null
@@ -1,36 +0,0 @@
-#!/bin/bash
-# Usage: tc_pyformat <list of pyformat options> file1.py file2.py ...
-#
-# Most common option is -i, which makes formatting changes in place.
-set -u
-
-PF=pyformat
-PF_OPTIONS="--yapf --force_quote_type=single"
-PF_USER_OPTIONS=""
-
-if [[ -z "$(type -t ${PF})" ]]; then
-  echo "Error: ${PF} not in your path."
-  exit 1
-fi
-
-while [[ "$1" == -* ]]; do
-  PF_USER_OPTIONS+=" $1"
-  shift
-done
-
-FILES=$*
-PF_OPTIONS+=${PF_USER_OPTIONS}
-
-for f in ${FILES}; do
-  if [[ $f != *.py ]]; then
-    echo "Error: File $f is not a python file"
-    exit 2
-  elif [[ -x $f ]]; then
-    ${PF} ${PF_OPTIONS} $f
-  elif [[ -f $f ]]; then
-    ${PF} --remove_shebang ${PF_OPTIONS} $f
-  else
-    echo "Error: File $f does not exist"
-    exit 2
-  fi
-done
diff --git a/toolchain_utils_githooks/check-format b/toolchain_utils_githooks/check-format
new file mode 100755
index 00000000..3868c338
--- /dev/null
+++ b/toolchain_utils_githooks/check-format
@@ -0,0 +1,102 @@
+#!/bin/bash -e
+# Copyright 2019 The Chromium OS Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+#
+# This script checks the format of the given files. If any look incorrectly
+# formatted, this will complain about them and exit. At the moment, it only
+# checks the format of Python.
+#
+# FIXME: It would be nice if YAPF supported tweaking quotes:
+# https://github.com/google/yapf/issues/399
+
+if test $# -eq 0; then
+  echo "No files were given to check the format of." >&2
+  echo "Usage: $0 file1 file2 ..." >&2
+  exit 1
+fi
+
+yapf=yapf
+
+if ! type "${yapf}" >/dev/null 2>&1; then
+  echo "${yapf} isn't on your \$PATH. Please either enter a chroot, or place" \
+    "depot_tools on your \$PATH." >&2
+  exit 1
+fi
+
+status_to_tf() {
+  if "$@" >& /dev/null; then
+    echo true
+  else
+    echo false
+  fi
+}
+
+complain_about_missing=$(status_to_tf test -z "${IGNORE_MISSING}")
+
+check_python_file_header() {
+  local py_file="$1"
+  local needs_hashbang=$(status_to_tf test -x "${py_file}")
+  local has_hashbang=$(status_to_tf grep -q '^#!' <(head -n1 "${py_file}"))
+
+  if test "${needs_hashbang}" = "${has_hashbang}"; then
+    return 0
+  fi
+
+  if "${needs_hashbang}"; then
+    echo "File ${py_file} needs a #!; please run" \
+      "\`sed -i '1i#!/usr/bin/env python' ${py_file}\`"
+  else
+    echo "File ${py_file} has an unnecessary #!; please run" \
+      "\`sed -i 1d ${py_file}\`"
+  fi
+  return 1
+}
+
+everything_passed=true
+python_files=()
+
+for f in "$@"; do
+  [[ "${f}" == *.py ]] || continue
+  if ! test -e "${f}"; then
+    if "${complain_about_missing}"; then
+      echo "error: no such file: ${f}" >&2
+      everything_passed=false
+    fi
+    continue
+  fi
+
+  python_files+=( "${f}" )
+
+  if ! check_python_file_header "${f}"; then
+    everything_passed=false
+  fi
+done
+
+bad_files=()
+if test "${#python_files[@]}" -ne 0; then
+  # yapf will give us a full unified (git-like) diff. We parse out the file
+  # names, e.g.,
+  #
+  # --- foo (original)
+  #
+  # Sed makes it so that bad_files consists only of those lines, but with the
+  # leading '--- ' and trailing ' (original)' removed.
+  #
+  # FIXME: Ideally, we should pass yapf the `-p` arg, so it'll format things in
+  # parallel. This requires concurrent.futures in python2 (which isn't
+  # available in the chroot by default), and is purely an optimization, so we
+  # can handle it later.
+  bad_files=(
+    $("${yapf}" -d "${python_files[@]}" |
+      sed -n '/^--- /{ s/^--- //; s/ *(original)$//p }')
+  )
+fi
+
+if test "${#bad_files[@]}" -ne 0; then
+  echo "One or more files appear to be incorrectly formatted."
+  echo "Please run \`${yapf} -i ${bad_files[@]}\` to rectify this."
+  everything_passed=false
+fi
+
+$everything_passed
diff --git a/toolchain_utils_githooks/pre-push.real b/toolchain_utils_githooks/pre-push.real
index 0f6856ee..7b65b175 100755
--- a/toolchain_utils_githooks/pre-push.real
+++ b/toolchain_utils_githooks/pre-push.real
@@ -4,9 +4,11 @@
 #
 # This is a pre-push hook that does the following before uploading a
 # CL for review:
-# 1) check that python sources have been formatted with pyformat.
+# 1) check that python sources have been formatted with yapf.
 # 2) allows the user to run the unit tests.
 
+mydir="$(dirname "$(readlink -m "$0")")"
+
 # This redirects stdin. Make sure to run after stdin has been read.
 run_UnitTests() {
   save_dir=$(pwd)
@@ -30,27 +32,6 @@ run_UnitTests() {
   fi
 }
 
-run_PyFormat() {
-  pyformat="./bin/tc_pyformat"
-  range=$1
-  files=$(git show --pretty="format:" --name-only $range)
-  for f in $files; do
-    [[ $f == *.py ]] || continue
-    # File could have been removed as part of the commit.
-    [[ -e $f ]] || continue
-    diffs=$($pyformat -d $f)
-    if [[ $? -ne 0 ]]; then
-      echo "Error: $pyformat $f returned with error code $?"
-      exit 1
-    fi
-    if [[ -n "$diffs" ]]; then
-      echo -e "Error: $f is not formatted correctly. Run $pyformat -i $f\n"
-      echo -e "diffs:\n$diffs\n"
-      exit 2
-    fi
-  done
-}
-
 z40=0000000000000000000000000000000000000000
 
 while IFS=' ' read local_ref local_sha remote_ref remote_sha; do
@@ -62,7 +43,10 @@ while IFS=' ' read local_ref local_sha remote_ref remote_sha; do
       # Update to existing branch, examine new commits
       range="$remote_sha..$local_sha"
     fi
-    run_PyFormat $range
+    all_files="$(git show --pretty="format:" --name-only "${range}")"
+    # Note that ${all_files} may include files that were deleted. Hence, we
+    # ignore any complaints about missing files.
+    IGNORE_MISSING=1 "${mydir}/check-format" ${all_files} || exit 1
   fi
 done