Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add minimal presubmit patch check #26807

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import chromium_presubmit_overrides
import override_utils

USE_PYTHON3 = True
PRESUBMIT_VERSION = '2.0.0'


Expand Down
2 changes: 2 additions & 0 deletions chromium_presubmit_config.json5
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@
"CheckTreeIsOpen",
// We run our own format check.
"CheckPatchFormatted",
// We don't use Chromium LSC process for Brave repository.
"CheckLargeScaleChange",
],
},

Expand Down
1 change: 0 additions & 1 deletion chromium_src/PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import brave_chromium_utils
import chromium_presubmit_overrides

USE_PYTHON3 = True
PRESUBMIT_VERSION = '2.0.0'


Expand Down
54 changes: 54 additions & 0 deletions patches/PRESUBMIT.py
Original file line number Diff line number Diff line change
@@ -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)
]
119 changes: 119 additions & 0 deletions patches/PRESUBMIT_test.py
Original file line number Diff line number Diff line change
@@ -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()
1 change: 0 additions & 1 deletion tools/perf/PRESUBMIT.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
import platform
import os

USE_PYTHON3 = True
PRESUBMIT_VERSION = '2.0.0'


Expand Down
Loading