pre-upload: check_manifests: improve check significantly
A number of improvements:
- Read the Manifest from the commit, not from the filesystem
- Allow comments/blank lines in the middle
- Make sure there is always a newline at the end of the file
- Disallow blank lines at the start/end of the file
BUG=None
TEST=unittests pass
Change-Id: I91d4d7b283ac08410e83d653dacb981a07524fca
Reviewed-on: https://chromium-review.googlesource.com/849136
Commit-Ready: Mike Frysinger <vapier@chromium.org>
Tested-by: Mike Frysinger <vapier@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
diff --git a/pre-upload.py b/pre-upload.py
index 7db459e..a04678b 100755
--- a/pre-upload.py
+++ b/pre-upload.py
@@ -1249,24 +1249,47 @@
def _check_manifests(_project, commit):
- """Make sure Manifest files only have DIST lines"""
- paths = []
+ """Make sure Manifest files only have comments & DIST lines."""
+ ret = []
- for path in _get_affected_files(commit):
- if os.path.basename(path) != 'Manifest':
- continue
- if not os.path.exists(path):
+ manifests = _filter_files(_get_affected_files(commit, relative=True),
+ [r'.*/Manifest$'])
+ for path in manifests:
+ data = _get_file_content(path, commit)
+
+ # Disallow blank files.
+ if not data.strip():
+ ret.append('%s: delete empty file' % (path,))
continue
- with open(path, 'r') as f:
- for line in f.readlines():
- if not line.startswith('DIST '):
- paths.append(path)
- break
+ # Make sure the last newline isn't omitted.
+ if data[-1] != '\n':
+ ret.append('%s: missing trailing newline' % (path,))
- if paths:
- return HookFailure('Please remove lines that do not start with DIST:\n%s' %
- ('\n'.join(paths),))
+ # Do not allow leading or trailing blank lines.
+ lines = data.splitlines()
+ if not lines[0]:
+ ret.append('%s: delete leading blank lines' % (path,))
+ if not lines[-1]:
+ ret.append('%s: delete trailing blank lines' % (path,))
+
+ for line in lines:
+ # Disallow leading/trailing whitespace.
+ if line != line.strip():
+ ret.append('%s: remove leading/trailing whitespace: %s' % (path, line))
+
+ # Allow blank lines & comments.
+ line = line.split('#', 1)[0]
+ if not line:
+ continue
+
+ # All other linse should start with DIST.
+ if not line.startswith('DIST '):
+ ret.append('%s: remove non-DIST lines: %s' % (path, line))
+ break
+
+ if ret:
+ return HookFailure('\n'.join(ret))
def _check_change_has_branch_field(_project, commit):
diff --git a/pre-upload_unittest.py b/pre-upload_unittest.py
index 5d43166..81bee92 100755
--- a/pre-upload_unittest.py
+++ b/pre-upload_unittest.py
@@ -222,6 +222,79 @@
self.assertFalse(failure)
+class CheckManifests(cros_test_lib.MockTestCase):
+ """Tests _check_manifests."""
+
+ def setUp(self):
+ self.file_mock = self.PatchObject(pre_upload, '_get_affected_files')
+ self.content_mock = self.PatchObject(pre_upload, '_get_file_content')
+
+ def testNoManifests(self):
+ """Nothing should be checked w/no Manifest files."""
+ self.file_mock.return_value = [
+ '/foo/bar.txt',
+ '/readme.md',
+ '/manifest',
+ '/Manifest.txt',
+ ]
+ ret = pre_upload._check_manifests('PROJECT', 'COMMIT')
+ self.assertIsNone(ret)
+
+ def testValidManifest(self):
+ """Accept valid Manifest files."""
+ self.file_mock.return_value = [
+ '/foo/bar.txt',
+ '/cat/pkg/Manifest',
+ ]
+ self.content_mock.return_value = '''# Comment and blank lines.
+
+DIST lines
+'''
+ ret = pre_upload._check_manifests('PROJECT', 'COMMIT')
+ self.assertIsNone(ret)
+ self.content_mock.assert_called_once_with('/cat/pkg/Manifest', 'COMMIT')
+
+ def _testReject(self, content):
+ """Make sure |content| is rejected."""
+ self.file_mock.return_value = ('/Manifest',)
+ self.content_mock.reset_mock()
+ self.content_mock.return_value = content
+ ret = pre_upload._check_manifests('PROJECT', 'COMMIT')
+ self.assertIsNotNone(ret)
+ self.content_mock.assert_called_once_with('/Manifest', 'COMMIT')
+
+ def testRejectBlank(self):
+ """Reject empty manifests."""
+ self._testReject('')
+
+ def testNoTrailingNewLine(self):
+ """Reject manifests w/out trailing newline."""
+ self._testReject('DIST foo')
+
+ def testNoLeadingBlankLines(self):
+ """Reject manifests w/leading blank lines."""
+ self._testReject('\nDIST foo\n')
+
+ def testNoTrailingBlankLines(self):
+ """Reject manifests w/trailing blank lines."""
+ self._testReject('DIST foo\n\n')
+
+ def testNoLeadingWhitespace(self):
+ """Reject manifests w/lines w/leading spaces."""
+ self._testReject(' DIST foo\n')
+ self._testReject(' # Comment\n')
+
+ def testNoTrailingWhitespace(self):
+ """Reject manifests w/lines w/trailing spaces."""
+ self._testReject('DIST foo \n')
+ self._testReject('# Comment \n')
+ self._testReject(' \n')
+
+ def testOnlyDistLines(self):
+ """Only allow DIST lines in here."""
+ self._testReject('EBUILD foo\n')
+
+
class CheckConfigParsing(cros_test_lib.MockTestCase):
"""Tests _check_cq_ini_well_formed."""
