From 39811b1915d39462a95428d4ae014e5e9bd2e1c3 Mon Sep 17 00:00:00 2001 From: Joanna Wang Date: Fri, 20 Jan 2023 23:09:48 +0000 Subject: [PATCH] Remove TBR from git cl Bug: b/266235601 Change-Id: I8122cf38d5a2c6879c32eafc6abd8fa306095125 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4185277 Reviewed-by: Gavin Mak Commit-Queue: Joanna Wang Reviewed-by: Josip Sokcevic --- git_cl.py | 66 +++++-------------- tests/git_cl_test.py | 149 +++++++++++++------------------------------ 2 files changed, 63 insertions(+), 152 deletions(-) diff --git a/git_cl.py b/git_cl.py index a1bbbeb39..eae031d5a 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1510,27 +1510,23 @@ class Changelist(object): change_description = ChangeDescription(description, bug, fixed) - # Fill gaps in OWNERS coverage to tbrs/reviewers if requested. + # Fill gaps in OWNERS coverage to reviewers if requested. if options.add_owners_to: - assert options.add_owners_to in ('TBR', 'R'), options.add_owners_to + assert options.add_owners_to in ('R'), options.add_owners_to status = self.owners_client.GetFilesApprovalStatus( - files, [], options.tbrs + options.reviewers) + files, [], options.reviewers) missing_files = [ f for f in files if status[f] == self._owners_client.INSUFFICIENT_REVIEWERS ] owners = self.owners_client.SuggestOwners( missing_files, exclude=[self.GetAuthor()]) - if options.add_owners_to == 'TBR': - assert isinstance(options.tbrs, list), options.tbrs - options.tbrs.extend(owners) - else: - assert isinstance(options.reviewers, list), options.reviewers - options.reviewers.extend(owners) + assert isinstance(options.reviewers, list), options.reviewers + options.reviewers.extend(owners) # Set the reviewer list now so that presubmit checks can access it. - if options.reviewers or options.tbrs: - change_description.update_reviewers(options.reviewers, options.tbrs) + if options.reviewers: + change_description.update_reviewers(options.reviewers) return change_description @@ -2627,12 +2623,6 @@ class Changelist(object): refspec_opts.append('l=Commit-Queue+1') refspec_opts.append('l=Quick-Run+1') - if change_desc.get_reviewers(tbr_only=True): - score = gerrit_util.GetCodeReviewTbrScore( - self.GetGerritHost(), - self.GetGerritProject()) - refspec_opts.append('l=Code-Review+%s' % score) - # Gerrit sorts hashtags, so order is not important. hashtags = {change_desc.sanitize_hash_tag(t) for t in options.hashtags} if not self.GetIssue(): @@ -2988,56 +2978,40 @@ class ChangeDescription(object): description = git_footers.add_footer_change_id(description, change_id) self.set_description(description) - def update_reviewers(self, reviewers, tbrs): - """Rewrites the R=/TBR= line(s) as a single line each. + def update_reviewers(self, reviewers): + """Rewrites the R= line(s) as a single line each. Args: reviewers (list(str)) - list of additional emails to use for reviewers. - tbrs (list(str)) - list of additional emails to use for TBRs. """ - if not reviewers and not tbrs: + if not reviewers: return reviewers = set(reviewers) - tbrs = set(tbrs) - LOOKUP = { - 'TBR': tbrs, - 'R': reviewers, - } - # Get the set of R= and TBR= lines and remove them from the description. + # Get the set of R= lines and remove them from the description. regexp = re.compile(self.R_LINE) matches = [regexp.match(line) for line in self._description_lines] new_desc = [l for i, l in enumerate(self._description_lines) if not matches[i]] self.set_description(new_desc) - # Construct new unified R= and TBR= lines. + # Construct new unified R= lines. - # First, update tbrs/reviewers with names from the R=/TBR= lines (if any). + # First, update reviewers with names from the R= lines (if any). for match in matches: if not match: continue - LOOKUP[match.group(1)].update(cleanup_list([match.group(2).strip()])) + reviewers.update(cleanup_list([match.group(2).strip()])) - # If any folks ended up in both groups, remove them from tbrs. - tbrs -= reviewers - - new_r_line = 'R=' + ', '.join(sorted(reviewers)) if reviewers else None - new_tbr_line = 'TBR=' + ', '.join(sorted(tbrs)) if tbrs else None + new_r_line = 'R=' + ', '.join(sorted(reviewers)) # Put the new lines in the description where the old first R= line was. line_loc = next((i for i, match in enumerate(matches) if match), -1) if 0 <= line_loc < len(self._description_lines): - if new_tbr_line: - self._description_lines.insert(line_loc, new_tbr_line) - if new_r_line: - self._description_lines.insert(line_loc, new_r_line) + self._description_lines.insert(line_loc, new_r_line) else: - if new_r_line: - self.append_footer(new_r_line) - if new_tbr_line: - self.append_footer(new_tbr_line) + self.append_footer(new_r_line) def set_preserve_tryjobs(self): """Ensures description footer contains 'Cq-Do-Not-Cancel-Tryjobs: true'.""" @@ -4441,9 +4415,6 @@ def CMDupload(parser, args): parser.add_option('-r', '--reviewers', action='append', default=[], help='reviewer email addresses') - parser.add_option('--tbrs', - action='append', default=[], - help='TBR email addresses') parser.add_option('--cc', action='append', default=[], help='cc email addresses') @@ -4468,8 +4439,6 @@ def CMDupload(parser, args): help='Don\'t squash multiple commits into one') parser.add_option('--topic', default=None, help='Topic to specify when uploading') - parser.add_option('--tbr-owners', dest='add_owners_to', action='store_const', - const='TBR', help='add a set of OWNERS to TBR') parser.add_option('--r-owners', dest='add_owners_to', action='store_const', const='R', help='add a set of OWNERS to R') parser.add_option('-c', @@ -4558,7 +4527,6 @@ def CMDupload(parser, args): return 1 options.reviewers = cleanup_list(options.reviewers) - options.tbrs = cleanup_list(options.tbrs) options.cc = cleanup_list(options.cc) if options.edit_description and options.force: diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 102e2451e..6706225c2 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -796,7 +796,6 @@ class TestGitCl(unittest.TestCase): issue=None, cc=None, custom_cl_base=None, - tbr=None, short_hostname='chromium', labels=None, change_id=None, @@ -936,14 +935,6 @@ class TestGitCl(unittest.TestCase): ref_suffix += ',l=%s+%d' % (k, v) metrics_arguments.append('l=%s+%d' % (k, v)) - if tbr: - calls += [ - (('GetCodeReviewTbrScore', - '%s-review.googlesource.com' % short_hostname, - 'my/repo'), - 2,), - ] - calls += [ ( ('time.time', ), @@ -1107,7 +1098,6 @@ class TestGitCl(unittest.TestCase): fetched_status=None, other_cl_owner=None, custom_cl_base=None, - tbr=None, short_hostname='chromium', labels=None, change_id=None, @@ -1221,7 +1211,6 @@ class TestGitCl(unittest.TestCase): issue=issue, cc=cc, custom_cl_base=custom_cl_base, - tbr=tbr, short_hostname=short_hostname, labels=labels, change_id=change_id, @@ -1370,18 +1359,12 @@ class TestGitCl(unittest.TestCase): change_id='Ixxx') def test_gerrit_reviewer_multiple(self): - mock.patch('git_cl.gerrit_util.GetCodeReviewTbrScore', - lambda *a: self._mocked_call('GetCodeReviewTbrScore', *a)).start() - self._run_gerrit_upload_test( - [], - 'desc ✔\nTBR=reviewer@example.com\nBUG=\nR=another@example.com\n' - 'CC=more@example.com,people@example.com\n\n' - 'Change-Id: 123456789', - ['reviewer@example.com', 'another@example.com'], - cc=['more@example.com', 'people@example.com'], - tbr='reviewer@example.com', - labels={'Code-Review': 2}, - change_id='123456789') + self._run_gerrit_upload_test([], 'desc ✔\nBUG=\nR=another@example.com\n' + 'CC=more@example.com,people@example.com\n\n' + 'Change-Id: 123456789', + ['another@example.com'], + cc=['more@example.com', 'people@example.com'], + change_id='123456789') def test_gerrit_upload_squash_first_is_default(self): self._run_gerrit_upload_test( @@ -1604,17 +1587,24 @@ class TestGitCl(unittest.TestCase): @mock.patch('git_cl.Changelist.GetGerritProject') @mock.patch('git_cl.Changelist.GetRemoteBranch') @mock.patch('owners_client.OwnersClient.BatchListOwners') - def getDescriptionForUploadTest( - self, mockBatchListOwners=None, mockGetRemoteBranch=None, - mockGetGerritProject=None, mockGetGerritHost=None, - mockGetCommonAncestorWithUpstream=None, mockGetBranch=None, - mockFetchDescription=None, mockGetBugPrefix=None, - mockIsCodeOwnersEnabledOnHost=None, - initial_description='desc', bug=None, fixed=None, branch='branch', - reviewers=None, tbrs=None, add_owners_to=None, - expected_description='desc'): + def getDescriptionForUploadTest(self, + mockBatchListOwners=None, + mockGetRemoteBranch=None, + mockGetGerritProject=None, + mockGetGerritHost=None, + mockGetCommonAncestorWithUpstream=None, + mockGetBranch=None, + mockFetchDescription=None, + mockGetBugPrefix=None, + mockIsCodeOwnersEnabledOnHost=None, + initial_description='desc', + bug=None, + fixed=None, + branch='branch', + reviewers=None, + add_owners_to=None, + expected_description='desc'): reviewers = reviewers or [] - tbrs = tbrs or [] owners_by_path = { 'a': ['a@example.com'], 'b': ['b@example.com'], @@ -1636,7 +1626,6 @@ class TestGitCl(unittest.TestCase): bug=bug, fixed=fixed, reviewers=reviewers, - tbrs=tbrs, add_owners_to=add_owners_to, message=initial_description), git_diff_args=None, @@ -1695,37 +1684,20 @@ class TestGitCl(unittest.TestCase): def testGetDescriptionForUpload_AddOwnersToR(self): self.getDescriptionForUploadTest( reviewers=['a@example.com'], - tbrs=['b@example.com'], add_owners_to='R', expected_description='\n'.join([ - 'desc', - '', - 'R=a@example.com, c@example.com', - 'TBR=b@example.com', - ])) - - def testGetDescriptionForUpload_AddOwnersToTBR(self): - self.getDescriptionForUploadTest( - reviewers=['a@example.com'], - tbrs=['b@example.com'], - add_owners_to='TBR', - expected_description='\n'.join([ - 'desc', - '', - 'R=a@example.com', - 'TBR=b@example.com, c@example.com', + 'desc', + '', + 'R=a@example.com, b@example.com, c@example.com', ])) def testGetDescriptionForUpload_AddOwnersToNoOwnersNeeded(self): self.getDescriptionForUploadTest( reviewers=['a@example.com', 'c@example.com'], - tbrs=['b@example.com'], - add_owners_to='TBR', expected_description='\n'.join([ 'desc', '', 'R=a@example.com, c@example.com', - 'TBR=b@example.com', ])) def testGetDescriptionForUpload_Reviewers(self): @@ -1737,24 +1709,13 @@ class TestGitCl(unittest.TestCase): 'R=a@example.com, b@example.com', ])) - def testGetDescriptionForUpload_TBRs(self): - self.getDescriptionForUploadTest( - tbrs=['a@example.com', 'b@example.com'], - expected_description='\n'.join([ - 'desc', - '', - 'TBR=a@example.com, b@example.com', - ])) - - def test_desecription_append_footer(self): + def test_description_append_footer(self): for init_desc, footer_line, expected_desc in [ # Use unique desc first lines for easy test failure identification. ('foo', 'R=one', 'foo\n\nR=one'), ('foo\n\nR=one', 'BUG=', 'foo\n\nR=one\nBUG='), ('foo\n\nR=one', 'Change-Id: Ixx', 'foo\n\nR=one\n\nChange-Id: Ixx'), ('foo\n\nChange-Id: Ixx', 'R=one', 'foo\n\nR=one\n\nChange-Id: Ixx'), - ('foo\n\nR=one\n\nChange-Id: Ixx', 'TBR=two', - 'foo\n\nR=one\nTBR=two\n\nChange-Id: Ixx'), ('foo\n\nR=one\n\nChange-Id: Ixx', 'Foo-Bar: baz', 'foo\n\nR=one\n\nChange-Id: Ixx\nFoo-Bar: baz'), ('foo\n\nChange-Id: Ixx', 'Foo-Bak: baz', @@ -1767,47 +1728,29 @@ class TestGitCl(unittest.TestCase): def test_update_reviewers(self): data = [ - ('foo', [], [], - 'foo'), - ('foo\nR=xx', [], [], - 'foo\nR=xx'), - ('foo\nTBR=xx', [], [], - 'foo\nTBR=xx'), - ('foo', ['a@c'], [], - 'foo\n\nR=a@c'), - ('foo\nR=xx', ['a@c'], [], - 'foo\n\nR=a@c, xx'), - ('foo\nTBR=xx', ['a@c'], [], - 'foo\n\nR=a@c\nTBR=xx'), - ('foo\nTBR=xx\nR=yy', ['a@c'], [], - 'foo\n\nR=a@c, yy\nTBR=xx'), - ('foo\nBUG=', ['a@c'], [], - 'foo\nBUG=\nR=a@c'), - ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [], - 'foo\n\nR=a@c, bar, xx\nTBR=yy'), - ('foo', ['a@c', 'b@c'], [], - 'foo\n\nR=a@c, b@c'), - ('foo\nBar\n\nR=\nBUG=', ['c@c'], [], - 'foo\nBar\n\nR=c@c\nBUG='), - ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [], - 'foo\nBar\n\nR=c@c\nBUG='), - # Same as the line before, but full of whitespaces. - ( - 'foo\nBar\n\n R = \n BUG = \n R = ', ['c@c'], [], - 'foo\nBar\n\nR=c@c\n BUG =', - ), - # Whitespaces aren't interpreted as new lines. - ('foo BUG=allo R=joe ', ['c@c'], [], - 'foo BUG=allo R=joe\n\nR=c@c'), - # Redundant TBRs get promoted to Rs - ('foo\n\nR=a@c\nTBR=t@c', ['b@c', 'a@c'], ['a@c', 't@c'], - 'foo\n\nR=a@c, b@c\nTBR=t@c'), + ('foo', [], 'foo'), + ('foo\nR=xx', [], 'foo\nR=xx'), + ('foo', ['a@c'], 'foo\n\nR=a@c'), + ('foo\nR=xx', ['a@c'], 'foo\n\nR=a@c, xx'), + ('foo\nBUG=', ['a@c'], 'foo\nBUG=\nR=a@c'), + ('foo\nR=xx\nR=bar', ['a@c'], 'foo\n\nR=a@c, bar, xx'), + ('foo', ['a@c', 'b@c'], 'foo\n\nR=a@c, b@c'), + ('foo\nBar\n\nR=\nBUG=', ['c@c'], 'foo\nBar\n\nR=c@c\nBUG='), + ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], 'foo\nBar\n\nR=c@c\nBUG='), + # Same as the line before, but full of whitespaces. + ( + 'foo\nBar\n\n R = \n BUG = \n R = ', + ['c@c'], + 'foo\nBar\n\nR=c@c\n BUG =', + ), + # Whitespaces aren't interpreted as new lines. + ('foo BUG=allo R=joe ', ['c@c'], 'foo BUG=allo R=joe\n\nR=c@c'), ] expected = [i[-1] for i in data] actual = [] - for orig, reviewers, tbrs, _expected in data: + for orig, reviewers, _expected in data: obj = git_cl.ChangeDescription(orig) - obj.update_reviewers(reviewers, tbrs) + obj.update_reviewers(reviewers) actual.append(obj.description) self.assertEqual(expected, actual)