[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>

git_cl.py

@@ -1372,9 +1372,24 @@ class Changelist(object):
 
     # 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 @@ class ChangeDescription(object):
       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 @@ class ChangeDescription(object):
       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 @@ class ChangeDescription(object):
 
     # 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

tests/git_cl_test.py

@@ -562,9 +562,6 @@ class TestGitCl(unittest.TestCase):
         '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 @@ class TestGitCl(unittest.TestCase):
   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 @@ class TestGitCl(unittest.TestCase):
     ]
     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 @@ class TestGitCl(unittest.TestCase):
                            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 @@ class TestGitCl(unittest.TestCase):
       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 @@ class TestGitCl(unittest.TestCase):
       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:
@@ -1109,6 +1098,9 @@ class TestGitCl(unittest.TestCase):
         return_value=post_amend_description or description).start()
     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 @@ class TestGitCl(unittest.TestCase):
         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 @@ class TestGitCl(unittest.TestCase):
           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 @@ class TestGitCl(unittest.TestCase):
 
   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 @@ class TestGitCl(unittest.TestCase):
         squash=False,
         squash_mode='override_nosquash',
         change_id='I123456789',
-        new_default=True)
+        default_branch='main')
 
 
 class ChangelistTest(unittest.TestCase):