diff --git a/git_footers.py b/git_footers.py index fd77c9d25f..8e9aaff9bc 100755 --- a/git_footers.py +++ b/git_footers.py @@ -127,6 +127,17 @@ def add_footer(message, key, value, after_keys=None, before_keys=None): return '\n'.join(top_lines + footer_lines) +def remove_footer(message, key): + """Returns a message with all instances of given footer removed.""" + key = normalize_name(key) + top_lines, footer_lines, _ = split_footers(message) + if not footer_lines: + return message + new_footer_lines = [ + l for l in footer_lines if normalize_name(parse_footer(l)[0]) != key] + return '\n'.join(top_lines + new_footer_lines) + + def get_unique(footers, key): key = normalize_name(key) values = footers[key] diff --git a/presubmit_support.py b/presubmit_support.py index 856f110886..b089c0d0a9 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -43,6 +43,7 @@ from warnings import warn import auth import fix_encoding import gclient_utils +import git_footers import gerrit_util import owners import owners_finder @@ -286,40 +287,38 @@ class OutputApi(object): CQ_INCLUDE_TRYBOTS was updated. """ description = cl.GetDescription(force=True) - all_bots = [] - include_re = re.compile(r'^CQ_INCLUDE_TRYBOTS=(.*)', re.M | re.I) - m = include_re.search(description) - if m: - all_bots = [i.strip() for i in m.group(1).split(';') if i.strip()] - if set(all_bots) >= set(bots_to_include): + include_re = re.compile(r'^CQ_INCLUDE_TRYBOTS=(.*)$', re.M | re.I) + + prior_bots = [] + if cl.IsGerrit(): + trybot_footers = git_footers.parse_footers(description).get( + git_footers.normalize_name('Cq-Include-Trybots'), []) + for f in trybot_footers: + prior_bots += [b.strip() for b in f.split(';') if b.strip()] + else: + trybot_tags = include_re.finditer(description) + for t in trybot_tags: + prior_bots += [b.strip() for b in t.group(1).split(';') if b.strip()] + + if set(prior_bots) >= set(bots_to_include): return [] - # Sort the bots to keep them in some consistent order -- not required. - all_bots = sorted(set(all_bots) | set(bots_to_include)) - new_include_trybots = 'CQ_INCLUDE_TRYBOTS=%s' % ';'.join(all_bots) - if m: - new_description = include_re.sub(new_include_trybots, description) + all_bots = ';'.join(sorted(set(prior_bots) | set(bots_to_include))) + + if cl.IsGerrit(): + description = git_footers.remove_footer( + description, 'Cq-Include-Trybots') + description = git_footers.add_footer( + description, 'Cq-Include-Trybots', all_bots, + before_keys=['Change-Id']) else: - # If we're adding a new CQ_INCLUDE_TRYBOTS line then make - # absolutely sure to add it before any Change-Id: line, to avoid - # breaking Gerrit. - # - # The use of \n outside the capture group causes the last - # newline before Change-Id and any extra newlines after it to be - # consumed. They are re-added during the join operation. - # - # The filter operation drops the trailing empty string after the - # original string is split. - split_desc = filter( - None, re.split('\n(Change-Id: \w*)\n*', description, 1, re.M)) - # Make sure to insert this before the last entry. For backward - # compatibility, ensure the CL description ends in a newline. - if len(split_desc) == 1: - insert_idx = 1 + new_include_trybots = 'CQ_INCLUDE_TRYBOTS=%s' % all_bots + m = include_re.search(description) + if m: + description = include_re.sub(new_include_trybots, description) else: - insert_idx = len(split_desc) - 1 - split_desc.insert(insert_idx, new_include_trybots) - new_description = '\n'.join(split_desc) + '\n' - cl.UpdateDescription(new_description, force=True) + description = '%s\n%s\n' % (description, new_include_trybots) + + cl.UpdateDescription(description, force=True) return [self.PresubmitNotifyResult(message)] diff --git a/tests/git_footers_test.py b/tests/git_footers_test.py index 900cedb9ac..8bc06bb2c0 100755 --- a/tests/git_footers_test.py +++ b/tests/git_footers_test.py @@ -105,6 +105,7 @@ My commit message is my best friend. It is my life. I must master it. self.assertEqual( git_footers.add_footer('', 'Key', 'Value'), '\nKey: Value') + self.assertEqual( git_footers.add_footer('Header with empty line.\n\n', 'Key', 'Value'), 'Header with empty line.\n\nKey: Value') @@ -145,6 +146,25 @@ My commit message is my best friend. It is my life. I must master it. 'Key', 'value', after_keys=['Other'], before_keys=['Some']), 'Top\n\nSome: footer\nOther: footer\nKey: value\nFinal: footer') + def testRemoveFooter(self): + self.assertEqual( + git_footers.remove_footer('message', 'Key'), + 'message') + + self.assertEqual( + git_footers.remove_footer('message\n\nSome: footer', 'Key'), + 'message\n\nSome: footer') + + self.assertEqual( + git_footers.remove_footer('message\n\nSome: footer\nKey: value', 'Key'), + 'message\n\nSome: footer') + + self.assertEqual( + git_footers.remove_footer( + 'message\n\nKey: value\nSome: footer\nKey: value', 'Key'), + 'message\n\nSome: footer') + + def testReadStdin(self): self.mock(git_footers.sys, 'stdin', StringIO.StringIO( 'line\r\notherline\r\n\r\n\r\nFoo: baz')) diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 3d17c08170..272e0b6708 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -155,15 +155,14 @@ class PresubmitUnittest(PresubmitTestsBase): def testMembersChanged(self): self.mox.ReplayAll() members = [ - 'AffectedFile', 'Change', - 'DoPostUploadExecuter', 'DoPresubmitChecks', 'GetPostUploadExecuter', - 'GitAffectedFile', 'CallCommand', 'CommandData', + 'AffectedFile', 'Change', 'DoPostUploadExecuter', 'DoPresubmitChecks', + 'GetPostUploadExecuter', 'GitAffectedFile', 'CallCommand', 'CommandData', 'GitChange', 'InputApi', 'ListRelevantPresubmitFiles', 'main', 'NonexistantCannedCheckFilter', 'OutputApi', 'ParseFiles', 'PresubmitFailure', 'PresubmitExecuter', 'PresubmitOutput', 'ScanSubDirs', - 'auth', 'cPickle', 'cpplint', 'cStringIO', - 'contextlib', 'canned_check_filter', 'fix_encoding', 'fnmatch', - 'gclient_utils', 'glob', 'inspect', 'json', 'load_files', 'logging', + 'auth', 'cPickle', 'cpplint', 'cStringIO', 'contextlib', + 'canned_check_filter', 'fix_encoding', 'fnmatch', 'gclient_utils', + 'git_footers', 'glob', 'inspect', 'json', 'load_files', 'logging', 'marshal', 'normpath', 'optparse', 'os', 'owners', 'owners_finder', 'pickle', 'presubmit_canned_checks', 'random', 're', 'rietveld', 'scm', 'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest', @@ -1406,10 +1405,12 @@ class OutputApiUnittest(PresubmitTestsBase): self.failIf(output.should_continue()) self.failUnless(output.getvalue().count('???')) - def _testIncludingCQTrybots(self, cl_text, new_trybots, updated_cl_text): + def _testIncludingCQTrybots(self, cl_text, new_trybots, updated_cl_text, + is_gerrit=False): class FakeCL(object): def __init__(self, description): self._description = description + self._is_gerrit = is_gerrit def GetDescription(self, force=False): return self._description @@ -1417,6 +1418,9 @@ class OutputApiUnittest(PresubmitTestsBase): def UpdateDescription(self, description, force=False): self._description = description + def IsGerrit(self): + return self._is_gerrit + def FakePresubmitNotifyResult(message): return message @@ -1435,11 +1439,11 @@ class OutputApiUnittest(PresubmitTestsBase): # We need long lines in this test. # pylint: disable=line-too-long - # Deliberately has a space at the end to exercise space-stripping code. + # Deliberately has a spaces to exercise space-stripping code. self._testIncludingCQTrybots( """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +CQ_INCLUDE_TRYBOTS= master.tryserver.blink:linux_trusty_blink_rel ;master.tryserver.chromium.win:win_optional_gpu_tests_rel """, [ 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', @@ -1461,42 +1465,39 @@ CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel;master.tryserve CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel """) - # Starting without any CQ_INCLUDE_TRYBOTS line, but with a - # Change-Id: line (with no trailing newline), which must continue - # to be the last line. + # All pre-existing bots are already in output set. self._testIncludingCQTrybots( """A change to GPU-related code. -Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", + +CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel +""", [ 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', - 'master.tryserver.chromium.mac:mac_optional_gpu_tests_rel', + 'master.tryserver.chromium.win:win_optional_gpu_tests_rel' ], """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel -Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 + +CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel """) - # Starting without any CQ_INCLUDE_TRYBOTS line, but with a - # Change-Id: line (with a trailing newline), which must continue - # to be the last line. + # Equivalent tests for Gerrit (pre-existing Change-Id line). self._testIncludingCQTrybots( """A change to GPU-related code. -Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 -""", + +Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", [ + 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', 'master.tryserver.chromium.mac:mac_optional_gpu_tests_rel', - 'master.tryserver.chromium.win:win_optional_gpu_tests_rel', ], """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel -Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 -""") - # Starting with a CQ_INCLUDE_TRYBOTS line and a Change-Id: line, - # the latter of which must continue to be the last line. +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel +Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True) + self._testIncludingCQTrybots( """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel + +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 """, [ @@ -1504,25 +1505,25 @@ Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 'master.tryserver.chromium.win:win_optional_gpu_tests_rel', ], """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel -Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 -""") - # All pre-existing bots are already in output set. +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True) + self._testIncludingCQTrybots( """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_rel +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_dbg +Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2 """, [ 'master.tryserver.chromium.linux:linux_optional_gpu_tests_rel', - 'master.tryserver.chromium.win:win_optional_gpu_tests_rel' + 'master.tryserver.chromium.win:win_optional_gpu_tests_rel', ], """A change to GPU-related code. -CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel -""") - +Cq-Include-Trybots: master.tryserver.chromium.linux:linux_optional_gpu_tests_dbg;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel +Change-Id: Idaeacea9cdbe912c24c8388147a8a767c7baa5f2""", is_gerrit=True) class AffectedFileUnittest(PresubmitTestsBase):