[git-cl] Use owners client when processing --[tb]r-owners.
Change-Id: Id094bce2aa731359cd8af16f10ce79ae7e02bd85
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2572809
Commit-Queue: Edward Lesmes <ehmaldonado@chromium.org>
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
diff --git a/git_cl.py b/git_cl.py
index bca2656..90427ef 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1372,9 +1372,24 @@
# Set the reviewer list now so that presubmit checks can access it.
if options.reviewers or options.tbrs or options.add_owners_to:
+ add_owners = []
+ if options.add_owners_to:
+ # Fill gaps in OWNERS coverage to tbrs/reviewers if requested.
+ project = self.GetGerritProject()
+ branch = self.GetCommonAncestorWithUpstream()
+ client = owners_client.DepotToolsClient(
+ host=self.GetGerritHost(),
+ root=settings.GetRoot(),
+ branch=branch)
+ status = client.GetFilesApprovalStatus(
+ project, branch, files, [], options.tbrs + options.reviewers)
+ missing_files = [
+ f for f in files
+ if status[f] == owners_client.INSUFFICIENT_REVIEWERS
+ ]
+ add_owners = client.SuggestOwners(project, branch, missing_files)
change_description.update_reviewers(
- options.reviewers, options.tbrs, options.add_owners_to, files,
- self.GetAuthor())
+ options.reviewers, options.tbrs, options.add_owners_to, add_owners)
return change_description
@@ -2607,8 +2622,7 @@
description = git_footers.add_footer_change_id(description, change_id)
self.set_description(description)
- def update_reviewers(
- self, reviewers, tbrs, add_owners_to, affected_files, author_email):
+ def update_reviewers(self, reviewers, tbrs, add_owners_to, add_owners):
"""Rewrites the R=/TBR= line(s) as a single line each.
Args:
@@ -2616,14 +2630,14 @@
tbrs (list(str)) - list of additional emails to use for TBRs.
add_owners_to (None|'R'|'TBR') - Pass to do an OWNERS lookup for files in
the change that are missing OWNER coverage. If this is not None, you
- must also pass a value for `change`.
- change (Change) - The Change that should be used for OWNERS lookups.
+ must also pass a value for `add_owners`.
+ add_owners (list(str)) - Owners to add to R or TBR if requested.
"""
assert isinstance(reviewers, list), reviewers
assert isinstance(tbrs, list), tbrs
assert add_owners_to in (None, 'TBR', 'R'), add_owners_to
- assert not add_owners_to or affected_files, add_owners_to
+ assert not add_owners_to or add_owners, add_owners_to
if not reviewers and not tbrs and not add_owners_to:
return
@@ -2652,12 +2666,7 @@
# Next, maybe fill in OWNERS coverage gaps to either tbrs/reviewers.
if add_owners_to:
- owners_db = owners.Database(settings.GetRoot(),
- fopen=open, os_path=os.path)
- missing_files = owners_db.files_not_covered_by(affected_files,
- (tbrs | reviewers))
- LOOKUP[add_owners_to].update(
- owners_db.reviewers_for(missing_files, author_email))
+ LOOKUP[add_owners_to].update(add_owners)
# If any folks ended up in both groups, remove them from tbrs.
tbrs -= reviewers
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index 6ec82b3..480678b 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -562,9 +562,6 @@
'git_cl.gclient_utils.CheckCallAndFilter',
self._mocked_call).start()
mock.patch('git_common.is_dirty_git_tree', lambda x: False).start()
- mock.patch(
- 'git_common.get_or_create_merge_base',
- lambda *a: self._mocked_call('get_or_create_merge_base', *a)).start()
mock.patch('git_cl.FindCodereviewSettingsFile', return_value='').start()
mock.patch(
'git_cl.SaveDescriptionBackup',
@@ -707,17 +704,13 @@
def _gerrit_base_calls(cls, issue=None, fetched_description=None,
fetched_status=None, other_cl_owner=None,
custom_cl_base=None, short_hostname='chromium',
- change_id=None, new_default=False):
+ change_id=None, default_branch='master'):
calls = []
if custom_cl_base:
ancestor_revision = custom_cl_base
else:
# Determine ancestor_revision to be merge base.
- ancestor_revision = 'fake_ancestor_sha'
- calls += [
- (('get_or_create_merge_base', 'master',
- 'refs/remotes/origin/master'), ancestor_revision),
- ]
+ ancestor_revision = 'origin/' + default_branch
if issue:
gerrit_util.GetChangeDetail.return_value = {
@@ -744,7 +737,7 @@
]
calls += [
((['git', 'show-branch', 'refs/remotes/origin/main'], ),
- '1' if new_default else callError(1)),
+ '1' if default_branch == 'main' else callError(1)),
]
return calls
@@ -758,8 +751,7 @@
labels=None, change_id=None,
final_description=None, gitcookies_exists=True,
force=False, edit_description=None,
- new_default=False):
- default_branch = 'main' if new_default else 'master';
+ default_branch='master'):
if post_amend_description is None:
post_amend_description = description
cc = cc or []
@@ -787,11 +779,8 @@
ref_to_push = 'abcdef0123456789'
if custom_cl_base is None:
- calls += [
- (('get_or_create_merge_base', 'master',
- 'refs/remotes/origin/master'), 'origin/' + default_branch),
- ]
parent = 'origin/' + default_branch
+ git_common.get_or_create_merge_base.return_value = parent
else:
calls += [
((['git', 'merge-base', '--is-ancestor', custom_cl_base,
@@ -1057,7 +1046,7 @@
log_description=None,
edit_description=None,
fetched_description=None,
- new_default=False):
+ default_branch='master'):
"""Generic gerrit upload test framework."""
if squash_mode is None:
if '--no-squash' in upload_args:
@@ -1110,6 +1099,9 @@
mock.patch(
'git_cl.GenerateGerritChangeId', return_value=change_id).start()
mock.patch(
+ 'git_common.get_or_create_merge_base',
+ return_value='origin/' + default_branch).start()
+ mock.patch(
'gclient_utils.AskForData',
lambda prompt: self._mocked_call('ask_for_data', prompt)).start()
@@ -1128,7 +1120,7 @@
custom_cl_base=custom_cl_base,
short_hostname=short_hostname,
change_id=change_id,
- new_default=new_default)
+ default_branch=default_branch)
if fetched_status != 'ABANDONED':
mock.patch(
'gclient_utils.temporary_file', TemporaryFileMock()).start()
@@ -1147,7 +1139,7 @@
gitcookies_exists=gitcookies_exists,
force=force,
edit_description=edit_description,
- new_default=new_default)
+ default_branch=default_branch)
# Uncomment when debugging.
# print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls))))
git_cl.main(['upload'] + upload_args)
@@ -1468,47 +1460,53 @@
def test_update_reviewers(self):
data = [
- ('foo', [], [],
+ ('foo', [], [], None, None,
'foo'),
- ('foo\nR=xx', [], [],
+ ('foo\nR=xx', [], [], None, None,
'foo\nR=xx'),
- ('foo\nTBR=xx', [], [],
+ ('foo\nTBR=xx', [], [], None, None,
'foo\nTBR=xx'),
- ('foo', ['a@c'], [],
+ ('foo', ['a@c'], [], None, None,
'foo\n\nR=a@c'),
- ('foo\nR=xx', ['a@c'], [],
+ ('foo\nR=xx', ['a@c'], [], None, None,
'foo\n\nR=a@c, xx'),
- ('foo\nTBR=xx', ['a@c'], [],
+ ('foo\nTBR=xx', ['a@c'], [], None, None,
'foo\n\nR=a@c\nTBR=xx'),
- ('foo\nTBR=xx\nR=yy', ['a@c'], [],
+ ('foo\nTBR=xx\nR=yy', ['a@c'], [], None, None,
'foo\n\nR=a@c, yy\nTBR=xx'),
- ('foo\nBUG=', ['a@c'], [],
+ ('foo\nBUG=', ['a@c'], [], None, None,
'foo\nBUG=\nR=a@c'),
- ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [],
+ ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [], None, None,
'foo\n\nR=a@c, bar, xx\nTBR=yy'),
- ('foo', ['a@c', 'b@c'], [],
+ ('foo', ['a@c', 'b@c'], [], None, None,
'foo\n\nR=a@c, b@c'),
- ('foo\nBar\n\nR=\nBUG=', ['c@c'], [],
+ ('foo\nBar\n\nR=\nBUG=', ['c@c'], [], None, None,
'foo\nBar\n\nR=c@c\nBUG='),
- ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [],
+ ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [], None, None,
'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\n R = \n BUG = \n R = ', ['c@c'], [], None, None,
'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 ', ['c@c'], [], None, None,
'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\nTBR=t@c', ['b@c', 'a@c'], ['a@c', 't@c'], None, None,
'foo\n\nR=a@c, b@c\nTBR=t@c'),
+ # Add to R
+ ('foo', [], [], 'R', ['a@c'],
+ 'foo\n\nR=a@c'),
+ # Add to TBR
+ ('foo', [], [], 'TBR', ['a@c'],
+ 'foo\n\nTBR=a@c'),
]
expected = [i[-1] for i in data]
actual = []
- for orig, reviewers, tbrs, _expected in data:
+ for orig, reviewers, tbrs, add_to, add, _expected in data:
obj = git_cl.ChangeDescription(orig)
- obj.update_reviewers(reviewers, tbrs, None, None, None)
+ obj.update_reviewers(reviewers, tbrs, add_to, add)
actual.append(obj.description)
self.assertEqual(expected, actual)
@@ -2746,7 +2744,7 @@
squash=False,
squash_mode='override_nosquash',
change_id='I123456789',
- new_default=True)
+ default_branch='main')
class ChangelistTest(unittest.TestCase):
