From 98b52d4e1ca7022e9b53e4631d1cecdbcc2c25f1 Mon Sep 17 00:00:00 2001 From: Aleksey Khoroshilov Date: Mon, 2 Dec 2024 14:42:34 +0700 Subject: [PATCH 1/2] Update presubmits a bit. --- PRESUBMIT.py | 1 - chromium_presubmit_config.json5 | 2 ++ chromium_src/PRESUBMIT.py | 1 - tools/perf/PRESUBMIT.py | 1 - 4 files changed, 2 insertions(+), 3 deletions(-) diff --git a/PRESUBMIT.py b/PRESUBMIT.py index d57b0976e1b2..ebb04e4d55db 100644 --- a/PRESUBMIT.py +++ b/PRESUBMIT.py @@ -13,7 +13,6 @@ import chromium_presubmit_overrides import override_utils -USE_PYTHON3 = True PRESUBMIT_VERSION = '2.0.0' diff --git a/chromium_presubmit_config.json5 b/chromium_presubmit_config.json5 index 5b15456237c1..a8662a447c8b 100644 --- a/chromium_presubmit_config.json5 +++ b/chromium_presubmit_config.json5 @@ -30,6 +30,8 @@ "CheckTreeIsOpen", // We run our own format check. "CheckPatchFormatted", + // We don't use Chromium LSC process for Brave repository. + "CheckLargeScaleChange", ], }, diff --git a/chromium_src/PRESUBMIT.py b/chromium_src/PRESUBMIT.py index ef14eb2d1354..68e20aee063c 100644 --- a/chromium_src/PRESUBMIT.py +++ b/chromium_src/PRESUBMIT.py @@ -6,7 +6,6 @@ import brave_chromium_utils import chromium_presubmit_overrides -USE_PYTHON3 = True PRESUBMIT_VERSION = '2.0.0' diff --git a/tools/perf/PRESUBMIT.py b/tools/perf/PRESUBMIT.py index f906db229d8a..6b665bbd2a89 100644 --- a/tools/perf/PRESUBMIT.py +++ b/tools/perf/PRESUBMIT.py @@ -7,7 +7,6 @@ import platform import os -USE_PYTHON3 = True PRESUBMIT_VERSION = '2.0.0' From 936c4fc0c3486fa2843744b929dcaae3c56e375f Mon Sep 17 00:00:00 2001 From: Aleksey Khoroshilov Date: Mon, 2 Dec 2024 14:50:00 +0700 Subject: [PATCH 2/2] Prevent unnecessary empty lines modification in patches. --- patches/PRESUBMIT.py | 54 +++++++++++++++++ patches/PRESUBMIT_test.py | 119 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 173 insertions(+) create mode 100644 patches/PRESUBMIT.py create mode 100644 patches/PRESUBMIT_test.py diff --git a/patches/PRESUBMIT.py b/patches/PRESUBMIT.py new file mode 100644 index 000000000000..2e3648e565a1 --- /dev/null +++ b/patches/PRESUBMIT.py @@ -0,0 +1,54 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +import chromium_presubmit_overrides + +PRESUBMIT_VERSION = '2.0.0' + + +# Adds support for chromium_presubmit_config.json5 and some helpers. +def CheckToModifyInputApi(input_api, _output_api): + chromium_presubmit_overrides.modify_input_api(input_api) + return [] + + +def CheckPatchFile(input_api, output_api): + 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()) + for lineno, line in enumerate(contents, 1): + # Prevent removing empty lines, it's a guaranteed git conflict. + if line == '-': + items.append(f'{f.LocalPath()}:{lineno} (removed empty line)') + continue + + # Prevent adding empty lines at diff block boundaries. This doesn't + # affect git conflict resolution, but adds unnecessary noise. It's + # okay to have empty lines in the middle of a diff block. + if line != '+': + continue + + # Look at previous and next lines to determine if this empty line is + # at diff block boundary. + prev_line = contents[lineno - 2] if lineno > 1 else '' + next_line = contents[lineno] if lineno < len(contents) else '' + + # Empty line at block boundary or single-line block. + if not (prev_line.startswith('+') and next_line.startswith('+')): + items.append(f'{f.LocalPath()}:{lineno} (added empty line)') + + if not items: + return [] + + return [ + output_api.PresubmitError( + 'Patch file should not add or remove empty lines', items) + ] diff --git a/patches/PRESUBMIT_test.py b/patches/PRESUBMIT_test.py new file mode 100644 index 000000000000..78bbc17c488d --- /dev/null +++ b/patches/PRESUBMIT_test.py @@ -0,0 +1,119 @@ +# Copyright (c) 2024 The Brave Authors. All rights reserved. +# This Source Code Form is subject to the terms of the Mozilla Public +# License, v. 2.0. If a copy of the MPL was not distributed with this file, +# You can obtain one at https://mozilla.org/MPL/2.0/. + +import unittest +import brave_chromium_utils + +# pylint: disable=import-error,no-member +import PRESUBMIT + +with brave_chromium_utils.sys_path("//"): + from PRESUBMIT_test_mocks import MockFile, MockAffectedFile + from PRESUBMIT_test_mocks import MockInputApi, MockOutputApi + + +class PatchFileTest(unittest.TestCase): + + def testEmptyLinesInPatch(self): + cases = [ + { + 'name': 'valid patch without empty lines', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+void func() {', + '+ DoSomething();', + '+}', + ], + 'expected_errors': 0 + }, + { + 'name': 'added single empty line', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+', + ], + 'expected_errors': 1 + }, + { + 'name': 'added multiple empty lines', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+', + '+', + ], + 'expected_errors': 1 + }, + { + 'name': 'removed empty line', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '-', + ], + 'expected_errors': 1 + }, + { + 'name': 'empty line at start of block', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+', + '+void func() {', + '+ DoSomething();', + '+}', + ], + 'expected_errors': 1 + }, + { + 'name': 'empty line at end of block', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+void func() {', + '+ DoSomething();', + '+}', + '+', + ], + 'expected_errors': 1 + }, + { + 'name': 'empty line in middle is ok', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + '+void func() {', + '+', + '+ DoSomething();', + '+}', + ], + 'expected_errors': 0 + }, + { + 'name': 'removed empty line', + 'contents': [ + 'diff --git a/file.cc b/file.cc', + ' void func() {', + '-', + ' DoSomething();', + ' }', + ], + 'expected_errors': 1 + }, + ] + + for case in cases: + with self.subTest(name=case['name']): + affected_file = MockAffectedFile('test.patch', + case['contents']) + input_api = MockInputApi() + input_api.files = [affected_file] + + results = PRESUBMIT.CheckPatchFile(input_api, MockOutputApi()) + + actual_errors = len(results) + self.assertEqual( + case['expected_errors'], actual_errors, + f"Expected {case['expected_errors']} errors, got {actual_errors}" + ) + + +if __name__ == '__main__': + unittest.main()