Add support for running Checkstyle on a given SHA-1.

Usage e.g.: ./checkstyle.py -s 3aca02ca91816a86febb5ed6e380ec2122adea47

Additionally this CL adds a pre-push script that can be used a git
hook to run Checkstyle on 'repo upload'

Bug: 25852971
Change-Id: Ia00d48df80b2b024de0899d9590868016c5b7bf0

tools/checkstyle/checkstyle.py

@@ -21,8 +21,10 @@
 import argparse
 import errno
 import os
+import shutil
 import subprocess
 import sys
+import tempfile
 import xml.dom.minidom
 import gitlint.git as git
 
@@ -35,46 +37,94 @@ FORCED_RULES = ['com.puppycrawl.tools.checkstyle.checks.imports.ImportOrderCheck
 ERROR_UNCOMMITTED = 'You need to commit all modified files before running Checkstyle\n'
 ERROR_UNTRACKED = 'You have untracked java files that are not being checked:\n'
 
-def RunCheckstyle(java_files):
-  if not os.path.exists(CHECKSTYLE_STYLE):
-    print 'Java checkstyle configuration file is missing'
-    sys.exit(1)
 
-  if java_files:
+def RunCheckstyleOnFiles(java_files):
-    # Files to check were specified via command line.
+  """Runs Checkstyle checks on a given set of java_files.
+
+  Args:
+    java_files: A list of files to check.
+
+  Returns:
+    A tuple of errors and warnings.
+  """
   print 'Running Checkstyle on inputted files'
-    sha = ''
-    last_commit_modified_files = {}
   java_files = map(os.path.abspath, java_files)
-  else:
-    # Files where not specified exclicitly. Let's use last commit files.
-    sha, last_commit_modified_files = _GetModifiedFiles()
-    print 'Running Checkstyle on %s commit' % sha
-    java_files = last_commit_modified_files.keys()
-
-    if not java_files:
-      print 'No files to check'
-      return
-
   stdout = _ExecuteCheckstyle(java_files)
-  (errors, warnings) = _ParseAndFilterOutput(stdout,
+  (errors, warnings) = _ParseAndFilterOutput(stdout)
-                                             sha,
+  _PrintErrorsAndWarnings(errors, warnings)
-                                             last_commit_modified_files)
+  return errors, warnings
 
+
+def RunCheckstyleOnACommit(commit):
+  """Runs Checkstyle checks on a given commit.
+
+  It will run Checkstyle on the changed Java files in a specified commit SHA-1
+  and if that is None it will fallback to check the latest commit of the
+  currently checked out branch.
+
+  Args:
+    commit: A full 40 character SHA-1 of a commit to check.
+
+  Returns:
+    A tuple of errors and warnings.
+  """
+  if not commit:
+    _WarnIfUntrackedFiles()
+    commit = git.last_commit()
+  print 'Running Checkstyle on %s commit' % commit
+  commit_modified_files = _GetModifiedFiles(commit)
+  if not commit_modified_files.keys():
+    print 'No Java files to check'
+    return [], []
+
+  (tmp_dir, tmp_file_map) = _GetTempFilesForCommit(
+      commit_modified_files.keys(), commit)
+
+  java_files = tmp_file_map.keys()
+  stdout = _ExecuteCheckstyle(java_files)
+
+  # Remove all the temporary files.
+  shutil.rmtree(tmp_dir)
+
+  (errors, warnings) = _ParseAndFilterOutput(stdout,
+                                             commit,
+                                             commit_modified_files,
+                                             tmp_file_map)
+  _PrintErrorsAndWarnings(errors, warnings)
+  return errors, warnings
+
+
+def _WarnIfUntrackedFiles(out=sys.stdout):
+  """Prints a warning and a list of untracked files if needed."""
+  root = git.repository_root()
+  untracked_files = git.modified_files(root, False)
+  untracked_files = {f for f in untracked_files if f.endswith('.java')}
+  if untracked_files:
+    out.write(ERROR_UNTRACKED)
+    for untracked_file in untracked_files:
+      out.write(untracked_file + '\n')
+    out.write('\n')
+
+
+def _PrintErrorsAndWarnings(errors, warnings):
+  """Prints given errors and warnings."""
   if errors:
     print 'ERRORS:'
     print '\n'.join(errors)
   if warnings:
     print 'WARNINGS:'
     print '\n'.join(warnings)
-  if errors or warnings:
-    sys.exit(1)
-
-  print 'SUCCESS! NO ISSUES FOUND'
-  sys.exit(0)
 
 
 def _ExecuteCheckstyle(java_files):
+  """Runs Checkstyle to check give Java files for style errors.
+
+  Args:
+    java_files: A list of Java files that needs to be checked.
+
+  Returns:
+    Checkstyle output in XML format.
+  """
   # Run checkstyle
   checkstyle_env = os.environ.copy()
   checkstyle_env['JAVA_CMD'] = 'java'
@@ -90,32 +140,38 @@ def _ExecuteCheckstyle(java_files):
       print 'Error running Checkstyle!'
       sys.exit(1)
 
-  # Work-around for Checkstyle printing error count to stdio.
+  # A work-around for Checkstyle printing error count to stdio.
   if 'Checkstyle ends with' in stdout.splitlines()[-1]:
     stdout = '\n'.join(stdout.splitlines()[:-1])
   return stdout
 
-def _ParseAndFilterOutput(stdout, sha, last_commit_modified_files):
+
+def _ParseAndFilterOutput(stdout,
+                          sha=None,
+                          commit_modified_files=None,
+                          tmp_file_map=None):
   result_errors = []
   result_warnings = []
   root = xml.dom.minidom.parseString(stdout)
   for file_element in root.getElementsByTagName('file'):
     file_name = file_element.attributes['name'].value
-    if last_commit_modified_files:
+    if tmp_file_map:
+      file_name = tmp_file_map[file_name]
+    if commit_modified_files:
       modified_lines = git.modified_lines(file_name,
-                                          last_commit_modified_files[file_name],
+                                          commit_modified_files[file_name],
                                           sha)
     file_name = os.path.relpath(file_name)
     errors = file_element.getElementsByTagName('error')
     for error in errors:
       line = int(error.attributes['line'].value)
       rule = error.attributes['source'].value
-      if last_commit_modified_files and _ShouldSkip(modified_lines, line, rule):
+      if commit_modified_files and _ShouldSkip(modified_lines, line, rule):
         continue
 
       column = ''
       if error.hasAttribute('column'):
-        column = '%s:' % (error.attributes['column'].value)
+        column = '%s:' % error.attributes['column'].value
       message = error.attributes['message'].value
       result = '  %s:%s:%s %s' % (file_name, line, column, message)
 
@@ -124,44 +180,99 @@ def _ParseAndFilterOutput(stdout, sha, last_commit_modified_files):
         result_errors.append(result)
       elif severity == 'warning':
         result_warnings.append(result)
-  return (result_errors, result_warnings)
+  return result_errors, result_warnings
 
 
-# Returns whether an error on a given line should be skipped
-# based on the modified_lines list and the rule.
 def _ShouldSkip(modified_lines, line, rule):
+  """Returns whether an error on a given line should be skipped.
+
+  Args:
+    modified_lines: A list of lines that has been modified.
+    line: The line that has a rule violation.
+    rule: The type of rule that a given line is violating.
+
+  Returns:
+    A boolean whether a given line should be skipped in the reporting.
+  """
   # None modified_lines means checked file is new and nothing should be skipped.
   if modified_lines is None:
     return False
   return line not in modified_lines and rule not in FORCED_RULES
 
 
-def _GetModifiedFiles(out = sys.stdout):
+def _GetModifiedFiles(commit, out=sys.stdout):
   root = git.repository_root()
-  sha = git.last_commit()
   pending_files = git.modified_files(root, True)
   if pending_files:
     out.write(ERROR_UNCOMMITTED)
     sys.exit(1)
-  untracked_files = git.modified_files(root, False)
-  untracked_files = {f for f in untracked_files if f.endswith('.java')}
-  if untracked_files:
-    out.write(ERROR_UNTRACKED)
-    for untracked_file in untracked_files:
-      out.write(untracked_file + '\n')
-    out.write('\n')
 
-  modified_files = git.modified_files(root, True, sha)
+  modified_files = git.modified_files(root, True, commit)
   modified_files = {f: modified_files[f] for f
                     in modified_files if f.endswith('.java')}
-  return (sha, modified_files)
+  return modified_files
+
+
+def _GetTempFilesForCommit(file_names, commit):
+  """Creates a temporary snapshot of the files in at a commit.
+
+  Retrieves the state of every file in file_names at a given commit and writes
+  them all out to a temporary directory.
+
+  Args:
+    file_names: A list of files that need to be retrieved.
+    commit: A full 40 character SHA-1 of a commit.
+
+  Returns:
+    A tuple of temprorary directory name and a directionary of
+    temp_file_name: filename. For example:
+
+    ('/tmp/random/', {'/tmp/random/blarg.java': 'real/path/to/file.java' }
+  """
+  tmp_dir_name = tempfile.mkdtemp()
+  tmp_file_names = {}
+  for file_name in file_names:
+    content = subprocess.check_output(
+        ['git', 'show', commit + ':' + os.path.relpath(file_name)])
+    (fd, tmp_file_name) = tempfile.mkstemp(suffix='.java',
+                                           dir=tmp_dir_name,
+                                           text=True)
+    tmp_file = os.fdopen(fd, 'w')
+    tmp_file.write(content)
+    tmp_file.close()
+    tmp_file_names[tmp_file_name] = file_name
+  return tmp_dir_name, tmp_file_names
 
 
 def main(args=None):
+  """Runs Checkstyle checks on a given set of java files or a commit.
+
+  It will run Checkstyle on the list of java files first, if unspecified,
+  then the check will be run on a specified commit SHA-1 and if that
+  is None it will fallback to check the latest commit of the currently checked
+  out branch.
+  """
   parser = argparse.ArgumentParser()
   parser.add_argument('--file', '-f', nargs='+')
+  parser.add_argument('--sha', '-s')
   args = parser.parse_args()
-  RunCheckstyle(args.file)
+
+  if not os.path.exists(CHECKSTYLE_STYLE):
+    print 'Java checkstyle configuration file is missing'
+    sys.exit(1)
+
+  if args.file:
+    # Files to check were specified via command line.
+    (errors, warnings) = RunCheckstyleOnFiles(args.file)
+  else:
+    (errors, warnings) = RunCheckstyleOnACommit(args.sha)
+
+  if errors or warnings:
+    sys.exit(1)
+
+  print 'SUCCESS! NO ISSUES FOUND'
+  sys.exit(0)
+
 
 if __name__ == '__main__':
   main()

tools/checkstyle/gitlint/git.py

@@ -110,10 +110,7 @@ def modified_lines(filename, extra_data, commit=None):
       filename: the file to check.
       extra_data: is the extra_data returned by modified_files. Additionally, a
         value of None means that the file was not modified.
-      commit: the complete sha1 (40 chars) of the commit. Note that specifying
+      commit: the complete sha1 (40 chars) of the commit.
-        this value will only work (100%) when commit == last_commit (with
-        respect to the currently checked out revision), otherwise, we could miss
-        some lines.
 
     Returns: a list of lines that were modified, or None in case all lines are
       new.
@@ -129,7 +126,7 @@ def modified_lines(filename, extra_data, commit=None):
 
     # Split as bytes, as the output may have some non unicode characters.
     blame_lines = subprocess.check_output(
-        ['git', 'blame', '--porcelain', filename]).split(
+        ['git', 'blame', commit, '--porcelain', '--', filename]).split(
             os.linesep.encode('utf-8'))
     modified_line_numbers = utils.filter_lines(
         blame_lines,

tools/checkstyle/pre-push.py

@@ -0,0 +1,48 @@
+#!/usr/bin/python
+
+#
+# Copyright 2016, The Android Open Source Project
+#
+# 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.
+#
+
+"""Pre-push hook to run Checkstyle on changes that are about to be uploaded.
+
+  Usage: add a symbolic link from /path/to/git/repo/.git/hooks/pre-push to
+  this file.
+"""
+
+import sys
+import checkstyle
+
+
+def main():
+  print '\nStarting Checkstyle!\n'
+  sys.stdout.flush()
+
+  ins = raw_input()
+  (errors, warnings) = checkstyle.RunCheckstyleOnACommit(ins.split(' ')[1])
+
+  if errors or warnings:
+    print 'Upload anyway (y/N)?:'
+    sys.stdin = open('/dev/tty')
+    sys.stdout.flush()
+    answer = raw_input()
+    if 'y' == answer.lower():
+      sys.exit(0)
+    else:
+      sys.exit(1)
+
+
+if __name__ == '__main__':
+  main()

tools/checkstyle/tests.py

@@ -89,37 +89,39 @@ class TestCheckstyle(unittest.TestCase):
   def test_GetModifiedFiles(self):
     checkstyle.git.modified_files = mock_modified_files_good
     out = StringIO()
-    sha, files = checkstyle._GetModifiedFiles(out)
+    files = checkstyle._GetModifiedFiles(mock_last_commit(), out=out)
     output = out.getvalue()
     self.assertEqual(output, '')
-    self.assertEqual(sha, TEST_SHA)
     self.assertEqual(files, {TEST_FILE1: FILE_MODIFIED, TEST_FILE2: FILE_ADDED})
 
-  def test_GetModifiedFilesUntracked(self):
-    checkstyle.git.modified_files = mock_modified_files_untracked
-    out = StringIO()
-    sha, files = checkstyle._GetModifiedFiles(out)
-    output = out.getvalue()
-    self.assertEqual(output, checkstyle.ERROR_UNTRACKED + TEST_FILE1 + '\n\n')
-    self.assertEqual(sha, TEST_SHA)
-    self.assertEqual(files, {TEST_FILE2: FILE_ADDED})
-
   def test_GetModifiedFilesUncommitted(self):
     checkstyle.git.modified_files = mock_modified_files_uncommitted
     with self.assertRaises(SystemExit):
       out = StringIO()
-      checkstyle._GetModifiedFiles(out)
+      checkstyle._GetModifiedFiles(mock_last_commit(), out=out)
     self.assertEqual(out.getvalue(), checkstyle.ERROR_UNCOMMITTED)
 
   def test_GetModifiedFilesNonJava(self):
     checkstyle.git.modified_files = mock_modified_files_non_java
     out = StringIO()
-    sha, files = checkstyle._GetModifiedFiles(out)
+    files = checkstyle._GetModifiedFiles(mock_last_commit(), out=out)
     output = out.getvalue()
     self.assertEqual(output, '')
-    self.assertEqual(sha, TEST_SHA)
     self.assertEqual(files, {TEST_FILE1: FILE_MODIFIED})
 
+  def test_WarnIfUntrackedFiles(self):
+    checkstyle.git.modified_files = mock_modified_files_untracked
+    out = StringIO()
+    checkstyle._WarnIfUntrackedFiles(out=out)
+    output = out.getvalue()
+    self.assertEqual(output, checkstyle.ERROR_UNTRACKED + TEST_FILE1 + '\n\n')
+
+  def test_WarnIfUntrackedFilesNoUntracked(self):
+    checkstyle.git.modified_files = mock_modified_files_good
+    out = StringIO()
+    checkstyle._WarnIfUntrackedFiles(out=out)
+    output = out.getvalue()
+    self.assertEqual(output, '')
 
 if __name__ == '__main__':
   unittest.main()