pre-upload: reject +x on files that should be -x

People sometimes add files with +x bits that don't make sense.
Add a check for many common data files and reject attempts to
add them with +x bits.

BUG=None
TEST=unittest passes
TEST=ran on some old bad commits and they were rejected

Change-Id: Id8de69b986e61b10ea17a4b4e4ceb18b09659259
Reviewed-on: https://chromium-review.googlesource.com/445983
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Prathmesh Prabhu <pprabhu@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index 734b626..14214a2 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -1321,6 +1321,79 @@
                        % (project_name, project_name))
 
 
+def _check_exec_files(_project, commit):
+  """Make +x bits on files."""
+  # List of files that should never be +x.
+  NO_EXEC = (
+      'ChangeLog*',
+      'COPYING',
+      'make.conf',
+      'make.defaults',
+      'Manifest',
+      'OWNERS',
+      'package.use',
+      'package.keywords',
+      'package.mask',
+      'parent',
+      'README',
+      'TODO',
+      '.gitignore',
+      '*.[achly]',
+      '*.[ch]xx',
+      '*.boto',
+      '*.cc',
+      '*.cfg',
+      '*.conf',
+      '*.config',
+      '*.cpp',
+      '*.css',
+      '*.ebuild',
+      '*.eclass',
+      '*.gyp',
+      '*.gypi',
+      '*.htm',
+      '*.html',
+      '*.ini',
+      '*.js',
+      '*.json',
+      '*.md',
+      '*.mk',
+      '*.patch',
+      '*.policy',
+      '*.proto',
+      '*.raw',
+      '*.rules',
+      '*.service',
+      '*.target',
+      '*.txt',
+      '*.xml',
+      '*.yaml',
+  )
+
+  def FinalName(obj):
+    # If the file is being deleted, then the dst_file is not set.
+    if obj.dst_file is None:
+      return obj.src_file
+    else:
+      return obj.dst_file
+
+  bad_files = []
+  files = _get_affected_files(commit, relative=True, full_details=True)
+  for f in files:
+    mode = int(f.dst_mode, 8)
+    if not mode & 0o111:
+      continue
+    name = FinalName(f)
+    for no_exec in NO_EXEC:
+      if fnmatch.fnmatch(name, no_exec):
+        bad_files.append(name)
+        break
+
+  if bad_files:
+    return HookFailure('These files should not be executable.  '
+                       'Please `chmod -x` them.', bad_files)
+
+
 # Base
 
 # A list of hooks which are not project specific and check patch description
@@ -1343,6 +1416,7 @@
     _check_ebuild_keywords,
     _check_ebuild_licenses,
     _check_ebuild_virtual_pv,
+    _check_exec_files,
     _check_for_uprev,
     _check_gofmt,
     _check_layout_conf,
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 44b47fd..bc03c27 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -1105,6 +1105,50 @@
     ])
     self.assertEquals(pre_upload._get_affected_files('HEAD', relative=True), [])
 
+
+class ExecFilesTest(cros_test_lib.MockTestCase):
+  """Tests for _check_exec_files."""
+
+  def setUp(self):
+    self.diff_mock = self.PatchObject(git, 'RawDiff')
+
+  def testNotExec(self):
+    """Do not flag files that are not executable."""
+    self.diff_mock.return_value = [
+        DiffEntry(src_file='make.conf', dst_mode='100644', status='A'),
+    ]
+    self.assertEqual(pre_upload._check_exec_files('proj', 'commit'), None)
+
+  def testExec(self):
+    """Flag files that are executable."""
+    self.diff_mock.return_value = [
+        DiffEntry(src_file='make.conf', dst_mode='100755', status='A'),
+    ]
+    self.assertNotEqual(pre_upload._check_exec_files('proj', 'commit'), None)
+
+  def testDeletedExec(self):
+    """Ignore bad files that are being deleted."""
+    self.diff_mock.return_value = [
+        DiffEntry(src_file='make.conf', dst_mode='100755', status='D'),
+    ]
+    self.assertEqual(pre_upload._check_exec_files('proj', 'commit'), None)
+
+  def testModifiedExec(self):
+    """Flag bad files that weren't exec, but now are."""
+    self.diff_mock.return_value = [
+        DiffEntry(src_file='make.conf', src_mode='100644', dst_mode='100755',
+                  status='M'),
+    ]
+    self.assertNotEqual(pre_upload._check_exec_files('proj', 'commit'), None)
+
+  def testNormalExec(self):
+    """Don't flag normal files (e.g. scripts) that are executable."""
+    self.diff_mock.return_value = [
+        DiffEntry(src_file='foo.sh', dst_mode='100755', status='A'),
+    ]
+    self.assertEqual(pre_upload._check_exec_files('proj', 'commit'), None)
+
+
 class CheckForUprev(cros_test_lib.MockTempDirTestCase):
   """Tests for _check_for_uprev."""