aiden
/
depot_tools
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Remove TBR from git cl

Browse Source 
Bug: b/266235601
Change-Id: I8122cf38d5a2c6879c32eafc6abd8fa306095125
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4185277
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Joanna Wang <jojwang@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@chromium.org>
changes/77/4185277/2
Joanna Wang 2 years ago committed by LUCI CQ
parent eb2866e654
commit 39811b1915
2 changed files with 63 additions and 152 deletions
Show Stats Download Patch File Download Diff File

66
git_cl.py
Unescape Escape View File

@ -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:

149
tests/git_cl_test.py
Unescape Escape View File

@ -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)

Write Preview
Loading…
Cancel
Save