diff --git a/patches/PRESUBMIT.py b/patches/PRESUBMIT.py index f5ae88cb31c..62209b58da5 100644 --- a/patches/PRESUBMIT.py +++ b/patches/PRESUBMIT.py @@ -5,8 +5,13 @@ import chromium_presubmit_overrides import os +import re + +# Full 40-char SHA hashes as produced by `git diff --full-index`. +_FULL_INDEX_RE = re.compile(r'^index ([0-9a-f]{40})\.\.([0-9a-f]{40})( \d+)?$') PRESUBMIT_VERSION = '2.0.0' +TEST_FILE_PATTERN = [r'.+_test\.py$'] # Adds support for chromium_presubmit_config.json5 and some helpers. @@ -15,6 +20,11 @@ def CheckToModifyInputApi(input_api, _output_api): return [] +def CheckTests(input_api, output_api): + return input_api.canned_checks.RunUnitTestsInDirectory( + input_api, output_api, '.', files_to_check=TEST_FILE_PATTERN) + + def CheckPatchFile(input_api, output_api): files_to_check = (r'.+\.patch$', ) files_to_skip = () @@ -98,3 +108,91 @@ def CheckPatchFileSource(input_api, output_api): output_api.PresubmitError( 'Patch filename does not match source file in diff header', items) ] + + +def CheckPatchFileIndexHeader(input_api, output_api): + """Checks that patch files use full SHA hashes in the index line. + + Patches must be generated with `git diff --full-index` so that index lines + contain full 40-character SHA hashes rather than abbreviated ones. + + Expected header format: + diff --git a/path/to/file.cc b/path/to/file.cc + index a1b2c3d4e5f6...(40 hex chars)..f6e5d4c3b2a1 100644 + --- a/path/to/file.cc + +++ b/path/to/file.cc + """ + files_to_check = (r'.+\.patch$', ) + files_to_skip = () + + file_filter = lambda f: input_api.FilterSourceFile( + f, files_to_check=files_to_check, files_to_skip=files_to_skip) + + items = [] + for f in input_api.AffectedSourceFiles(file_filter): + contents = list(f.NewContents()) + if not contents: + continue + + # The index line should appear within the first few header lines. + index_line = None + for i, line in enumerate(contents[:5]): + if line.startswith('index '): + index_line = (i + 1, line) + break + + if index_line is None: + items.append( + f'{f.LocalPath()}: missing index line in patch header') + continue + + lineno, line = index_line + if not _FULL_INDEX_RE.match(line): + items.append(f'{f.LocalPath()}:{lineno}: index line must use full' + ' 40-character SHA hashes (generate patches with' + ' `git diff --full-index`).\n' + f' Found: {line}') + + if not items: + return [] + + return [ + output_api.PresubmitError( + 'Patch index line does not use full SHA hashes', items) + ] + + +def CheckPatchFileSingleSource(input_api, output_api): + """Checks that each patch file patches exactly one source file. + + Each .patch file must contain a single `diff --git` section. Patching + multiple files in one .patch file is not allowed; use one patch file per + source file instead. + """ + files_to_check = (r'.+\.patch$', ) + files_to_skip = () + + file_filter = lambda f: input_api.FilterSourceFile( + f, files_to_check=files_to_check, files_to_skip=files_to_skip) + + items = [] + for f in input_api.AffectedSourceFiles(file_filter): + contents = list(f.NewContents()) + + diff_headers = [(i + 1, line) for i, line in enumerate(contents) + if line.startswith('diff --git ')] + + if len(diff_headers) != 1: + count = len(diff_headers) + detail = (f' Found {count} diff --git headers' + if count > 1 else ' No diff --git header found') + items.append(f'{f.LocalPath()}: patch must contain exactly one' + f' source file.\n{detail}') + + if not items: + return [] + + return [ + output_api.PresubmitError( + 'Patch file must patch exactly one source file', items) + ] diff --git a/patches/PRESUBMIT_test.py b/patches/PRESUBMIT_test.py index 78372b72057..086e7e222b6 100644 --- a/patches/PRESUBMIT_test.py +++ b/patches/PRESUBMIT_test.py @@ -10,7 +10,7 @@ import brave_chromium_utils import PRESUBMIT with brave_chromium_utils.sys_path("//"): - from PRESUBMIT_test_mocks import MockFile, MockAffectedFile + from PRESUBMIT_test_mocks import MockAffectedFile from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi @@ -134,8 +134,200 @@ class PatchFileTest(unittest.TestCase): actual_errors = len(results) self.assertEqual( case['expected_errors'], actual_errors, - f"Expected {case['expected_errors']} errors, got {actual_errors}" - ) + f"Expected {case['expected_errors']} errors," + f" got {actual_errors}") + + +class PatchFileSourceTest(unittest.TestCase): + + def _run(self, contents, filename): + affected_file = MockAffectedFile(filename, contents) + input_api = MockInputApi() + input_api.files = [affected_file] + return PRESUBMIT.CheckPatchFileSource(input_api, MockOutputApi()) + + def testFilenameMatchesConventionPasses(self): + # base/metrics/foo.h -> base-metrics-foo.h.patch + results = self._run( + contents=['diff --git a/base/metrics/foo.h b/base/metrics/foo.h'], + filename='base-metrics-foo.h.patch') + self.assertEqual(0, len(results)) + + def testFilenameDeepPathPasses(self): + # chrome/browser/ui/views/frame/bar.cc + # -> chrome-browser-ui-views-frame-bar.cc.patch + results = self._run( + contents=[ + 'diff --git a/chrome/browser/ui/views/frame/bar.cc' + ' b/chrome/browser/ui/views/frame/bar.cc' + ], + filename='chrome-browser-ui-views-frame-bar.cc.patch') + self.assertEqual(0, len(results)) + + def testWrongFilenameFails(self): + results = self._run( + contents=['diff --git a/base/metrics/foo.h b/base/metrics/foo.h'], + filename='wrong-name.patch') + self.assertEqual(1, len(results)) + + def testMissingDirectoryInFilenameFails(self): + # Header says base/metrics/foo.h but filename omits the directory level. + results = self._run( + contents=['diff --git a/base/metrics/foo.h b/base/metrics/foo.h'], + filename='base-foo.h.patch') + self.assertEqual(1, len(results)) + + def testEmptyPatchFails(self): + results = self._run(contents=[], filename='base-foo.h.patch') + self.assertEqual(1, len(results)) + + def testMissingDiffHeaderFails(self): + results = self._run(contents=['--- a/base/foo.h', '+++ b/base/foo.h'], + filename='base-foo.h.patch') + self.assertEqual(1, len(results)) + + def testNonPatchFileIgnored(self): + results = self._run(contents=['diff --git a/base/foo.h b/base/foo.h'], + filename='base-foo.h.txt') + self.assertEqual(0, len(results)) + + +_FULL_SHA = 'a' * 40 +_FULL_SHA2 = 'b' * 40 +_SHORT_SHA = 'abc1234' +_SHORT_SHA2 = 'def5678' + + +class PatchFileIndexHeaderTest(unittest.TestCase): + + def _run(self, contents, filename='foo-bar.patch'): + affected_file = MockAffectedFile(filename, contents) + input_api = MockInputApi() + input_api.files = [affected_file] + return PRESUBMIT.CheckPatchFileIndexHeader(input_api, MockOutputApi()) + + def testValidFullIndexHeader(self): + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_FULL_SHA}..{_FULL_SHA2} 100644', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + '@@ -1 +1 @@', + '+// brave', + ]) + self.assertEqual(0, len(results)) + + def testAbbreviatedShaFails(self): + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_SHORT_SHA}..{_SHORT_SHA2} 100644', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + ]) + self.assertEqual(1, len(results)) + + def testMissingIndexLineFails(self): + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + ]) + self.assertEqual(1, len(results)) + + def testEmptyPatchSkipped(self): + results = self._run([]) + self.assertEqual(0, len(results)) + + def testExecutableModeInIndexLine(self): + results = self._run([ + 'diff --git a/foo/bar.py b/foo/bar.py', + f'index {_FULL_SHA}..{_FULL_SHA2} 100755', + '--- a/foo/bar.py', + '+++ b/foo/bar.py', + ]) + self.assertEqual(0, len(results)) + + def testIndexLineWithoutMode(self): + # Some git diff outputs omit the mode when it doesn't change. + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_FULL_SHA}..{_FULL_SHA2}', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + ]) + self.assertEqual(0, len(results)) + + def testNonPatchFileIgnored(self): + affected_file = MockAffectedFile('foo.txt', [ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_SHORT_SHA}..{_SHORT_SHA2} 100644', + ]) + input_api = MockInputApi() + input_api.files = [affected_file] + results = PRESUBMIT.CheckPatchFileIndexHeader(input_api, + MockOutputApi()) + self.assertEqual(0, len(results)) + + +class PatchFileSingleSourceTest(unittest.TestCase): + + def _run(self, contents, filename='foo-bar.patch'): + affected_file = MockAffectedFile(filename, contents) + input_api = MockInputApi() + input_api.files = [affected_file] + return PRESUBMIT.CheckPatchFileSingleSource(input_api, MockOutputApi()) + + def testSingleSourcePasses(self): + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_FULL_SHA}..{_FULL_SHA2} 100644', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + '@@ -1 +1 @@', + '+// brave', + ]) + self.assertEqual(0, len(results)) + + def testMultipleSourcesFails(self): + results = self._run([ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + f'index {_FULL_SHA}..{_FULL_SHA2} 100644', + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + '@@ -1 +1 @@', + '+// brave', + 'diff --git a/foo/baz.cc b/foo/baz.cc', + f'index {_FULL_SHA}..{_FULL_SHA2} 100644', + '--- a/foo/baz.cc', + '+++ b/foo/baz.cc', + '@@ -1 +1 @@', + '+// also brave', + ]) + self.assertEqual(1, len(results)) + + def testNoDiffHeaderFails(self): + results = self._run([ + '--- a/foo/bar.cc', + '+++ b/foo/bar.cc', + '@@ -1 +1 @@', + '+// brave', + ]) + self.assertEqual(1, len(results)) + + def testEmptyPatchFails(self): + results = self._run([]) + self.assertEqual(1, len(results)) + + def testNonPatchFileIgnored(self): + affected_file = MockAffectedFile('foo.txt', [ + 'diff --git a/foo/bar.cc b/foo/bar.cc', + 'diff --git a/foo/baz.cc b/foo/baz.cc', + ]) + input_api = MockInputApi() + input_api.files = [affected_file] + results = PRESUBMIT.CheckPatchFileSingleSource(input_api, + MockOutputApi()) + self.assertEqual(0, len(results)) if __name__ == '__main__':