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."""