From e5d0a56e04a57c19c6f8ce2c79a3a60bbe3d922c Mon Sep 17 00:00:00 2001 From: Dominic Battre Date: Wed, 15 Sep 2021 20:09:55 +0000 Subject: [PATCH] Warn user in case they use commit messages with "Bug=" instead of "Bug:" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Chrome uses the syntax "Bug: xyz" in commit messages to attribute CLs to bugs while Critique uses "BUG=xyz". As per ``` git log --grep '\(Bug\|Fixed\)=' --since=2021-01-01 --format=oneline \ | wc -l ``` we've had 27 CLs this year using the wrong syntax which led to crbug.com not learning about the CLs <-> Bug attribution. Yours truly caused one of them and wants to prevent the problem in the future. We might relax the requirments in crbug.com or chromium-review.googlesource.com but then anyone grepping through the commit logs would need to deal with the ambiguity. Therefore, we can just enforce the right syntax here. https://chromium.googlesource.com/chromium/src/+/refs/heads/main/docs/contributing.md contains the correct syntax. Change-Id: I60ee579deac50a74e1a014ceb1d9744cbc10ab41 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/3141567 Reviewed-by: Josip Sokcevic Commit-Queue: Dominic Battré --- presubmit_canned_checks.py | 16 +++++++++++ presubmit_canned_checks_test.py | 41 +++++++++++++++++++++++++++ presubmit_canned_checks_test_mocks.py | 7 +++-- 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 47d186d8d..af4d0ac13 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -109,6 +109,22 @@ def CheckChangeWasUploaded(input_api, output_api): return [] +def CheckDescriptionUsesColonInsteadOfEquals(input_api, output_api): + """Checks that the CL description uses a colon after 'Bug' and 'Fixed' tags + instead of equals. + + crbug.com only interprets the lines "Bug: xyz" and "Fixed: xyz" but not + "Bug=xyz" or "Fixed=xyz". + """ + text = input_api.change.DescriptionText() + if input_api.re.search(r'^(Bug|Fixed)=', + text, + flags=input_api.re.IGNORECASE + | input_api.re.MULTILINE): + return [output_api.PresubmitError('Use Bug:/Fixed: instead of Bug=/Fixed=')] + return [] + + ### Content checks diff --git a/presubmit_canned_checks_test.py b/presubmit_canned_checks_test.py index 46c8022d8..163a8cd57 100644 --- a/presubmit_canned_checks_test.py +++ b/presubmit_canned_checks_test.py @@ -9,6 +9,7 @@ import unittest from presubmit_canned_checks_test_mocks import MockFile, MockAffectedFile from presubmit_canned_checks_test_mocks import MockInputApi, MockOutputApi +from presubmit_canned_checks_test_mocks import MockChange import presubmit_canned_checks @@ -200,5 +201,45 @@ class InclusiveLanguageCheckTest(unittest.TestCase): self.assertEqual([], errors) +class DescriptionChecksTest(unittest.TestCase): + def testCheckDescriptionUsesColonInsteadOfEquals(self): + input_api = MockInputApi() + input_api.change.RepositoryRoot = lambda: '' + input_api.presubmit_local_path = '' + + # Verify error in case of the attempt to use "Bug=". + input_api.change = MockChange([], 'Broken description\nBug=123') + errors = presubmit_canned_checks.CheckDescriptionUsesColonInsteadOfEquals( + input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertTrue('Bug=' in errors[0].message) + + # Verify error in case of the attempt to use "Fixed=". + input_api.change = MockChange([], 'Broken description\nFixed=123') + errors = presubmit_canned_checks.CheckDescriptionUsesColonInsteadOfEquals( + input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertTrue('Fixed=' in errors[0].message) + + # Verify error in case of the attempt to use the lower case "bug=". + input_api.change = MockChange([], 'Broken description lowercase\nbug=123') + errors = presubmit_canned_checks.CheckDescriptionUsesColonInsteadOfEquals( + input_api, MockOutputApi()) + self.assertEqual(1, len(errors)) + self.assertTrue('Bug=' in errors[0].message) + + # Verify no error in case of "Bug:" + input_api.change = MockChange([], 'Correct description\nBug: 123') + errors = presubmit_canned_checks.CheckDescriptionUsesColonInsteadOfEquals( + input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + # Verify no error in case of "Fixed:" + input_api.change = MockChange([], 'Correct description\nFixed: 123') + errors = presubmit_canned_checks.CheckDescriptionUsesColonInsteadOfEquals( + input_api, MockOutputApi()) + self.assertEqual(0, len(errors)) + + if __name__ == '__main__': unittest.main() diff --git a/presubmit_canned_checks_test_mocks.py b/presubmit_canned_checks_test_mocks.py index 8b26d42b7..80ab3407d 100644 --- a/presubmit_canned_checks_test_mocks.py +++ b/presubmit_canned_checks_test_mocks.py @@ -119,7 +119,7 @@ class MockInputApi(object): def ReadFile(self, filename, mode='rU'): if hasattr(filename, 'AbsoluteLocalPath'): - filename = filename.AbsoluteLocalPath() + filename = filename.AbsoluteLocalPath() for file_ in self.files: if file_.LocalPath() == filename: return '\n'.join(file_.NewContents()) @@ -243,9 +243,10 @@ class MockChange(object): current change. """ - def __init__(self, changed_files): + def __init__(self, changed_files, description=''): self._changed_files = changed_files self.footers = defaultdict(list) + self._description = description def LocalPaths(self): return self._changed_files @@ -257,3 +258,5 @@ class MockChange(object): def GitFootersFromDescription(self): return self.footers + def DescriptionText(self): + return self._description