Revert "[git-cl] Use owners client when processing --[tb]r-owners."

This reverts commit 968b1fe7.

Reason for revert: Appears to be breaking git cl upload for the recipe autoroller with https://logs.chromium.org/logs/infra-internal/buildbucket/cr-buildbucket.appspot.com/8858802226988308224/+/steps/infra/0/steps/git_cl_upload/0/stdout. 

Original change's description:
> [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>

TBR=ehmaldonado@chromium.org,apolito@google.com,infra-scoped@luci-project-accounts.iam.gserviceaccount.com,sokcevic@google.com

# Not skipping CQ checks because original CL landed > 1 day ago.

Change-Id: Id561a059eaf1419cbda52b7c0c6a45b5c6f2bd1e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2611219Reviewed-by: Stephen Martinis <martiniss@chromium.org>
Reviewed-by: Edward Lesmes <ehmaldonado@chromium.org>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Dirk Pranke <dpranke@google.com>
Commit-Queue: Stephen Martinis <martiniss@chromium.org>

git_cl.py

@@ -1373,24 +1373,9 @@ 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, add_owners)
+          options.reviewers, options.tbrs, options.add_owners_to, files,
+          self.GetAuthor())
 
     return change_description
 
@@ -2629,7 +2614,8 @@ 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, add_owners):
+  def update_reviewers(
+      self, reviewers, tbrs, add_owners_to, affected_files, author_email):
     """Rewrites the R=/TBR= line(s) as a single line each.
 
     Args:
@@ -2637,14 +2623,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 `add_owners`.
-      add_owners (list(str)) - Owners to add to R or TBR if requested.
+        must also pass a value for `change`.
+      change (Change) - The Change that should be used for OWNERS lookups.
     """
     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 add_owners, add_owners_to
+    assert not add_owners_to or affected_files, add_owners_to
 
     if not reviewers and not tbrs and not add_owners_to:
       return
@@ -2673,7 +2659,12 @@ class ChangeDescription(object):
 
     # Next, maybe fill in OWNERS coverage gaps to either tbrs/reviewers.
     if add_owners_to:
-      LOOKUP[add_owners_to].update(add_owners)
+      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))
 
     # If any folks ended up in both groups, remove them from tbrs.
     tbrs -= reviewers

tests/git_cl_test.py

@@ -591,6 +591,9 @@ 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',
@@ -733,13 +736,17 @@ 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, default_branch='master'):
+                         change_id=None, new_default=False):
     calls = []
     if custom_cl_base:
       ancestor_revision = custom_cl_base
     else:
       # Determine ancestor_revision to be merge base.
-      ancestor_revision = 'origin/' + default_branch
+      ancestor_revision = 'fake_ancestor_sha'
+      calls += [
+        (('get_or_create_merge_base', 'master',
+          'refs/remotes/origin/master'), ancestor_revision),
+      ]
 
     if issue:
       gerrit_util.GetChangeDetail.return_value = {
@@ -766,7 +773,7 @@ class TestGitCl(unittest.TestCase):
     ]
     calls += [
       ((['git', 'show-branch', 'refs/remotes/origin/main'], ),
-         '1' if default_branch == 'main' else callError(1)),
+         '1' if new_default else callError(1)),
     ]
 
     return calls
@@ -780,7 +787,8 @@ class TestGitCl(unittest.TestCase):
                            labels=None, change_id=None,
                            final_description=None, gitcookies_exists=True,
                            force=False, edit_description=None,
-                           default_branch='master'):
+                           new_default=False):
+    default_branch = 'main' if new_default else 'master';
     if post_amend_description is None:
       post_amend_description = description
     cc = cc or []
@@ -808,8 +816,11 @@ 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,
@@ -1075,7 +1086,7 @@ class TestGitCl(unittest.TestCase):
       log_description=None,
       edit_description=None,
       fetched_description=None,
-      default_branch='master'):
+      new_default=False):
     """Generic gerrit upload test framework."""
     if squash_mode is None:
       if '--no-squash' in upload_args:
@@ -1127,9 +1138,6 @@ 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()
@@ -1149,7 +1157,7 @@ class TestGitCl(unittest.TestCase):
         custom_cl_base=custom_cl_base,
         short_hostname=short_hostname,
         change_id=change_id,
-        default_branch=default_branch)
+        new_default=new_default)
     if fetched_status != 'ABANDONED':
       mock.patch(
           'gclient_utils.temporary_file', TemporaryFileMock()).start()
@@ -1168,7 +1176,7 @@ class TestGitCl(unittest.TestCase):
           gitcookies_exists=gitcookies_exists,
           force=force,
           edit_description=edit_description,
-          default_branch=default_branch)
+          new_default=new_default)
     # Uncomment when debugging.
     # print('\n'.join(map(lambda x: '%2i: %s' % x, enumerate(self.calls))))
     git_cl.main(['upload'] + upload_args)
@@ -1489,53 +1497,47 @@ class TestGitCl(unittest.TestCase):
 
   def test_update_reviewers(self):
     data = [
-      ('foo', [], [], None, None,
+      ('foo', [], [],
        'foo'),
-      ('foo\nR=xx', [], [], None, None,
+      ('foo\nR=xx', [], [],
        'foo\nR=xx'),
-      ('foo\nTBR=xx', [], [], None, None,
+      ('foo\nTBR=xx', [], [],
        'foo\nTBR=xx'),
-      ('foo', ['a@c'], [], None, None,
+      ('foo', ['a@c'], [],
        'foo\n\nR=a@c'),
-      ('foo\nR=xx', ['a@c'], [], None, None,
+      ('foo\nR=xx', ['a@c'], [],
        'foo\n\nR=a@c, xx'),
-      ('foo\nTBR=xx', ['a@c'], [], None, None,
+      ('foo\nTBR=xx', ['a@c'], [],
        'foo\n\nR=a@c\nTBR=xx'),
-      ('foo\nTBR=xx\nR=yy', ['a@c'], [], None, None,
+      ('foo\nTBR=xx\nR=yy', ['a@c'], [],
        'foo\n\nR=a@c, yy\nTBR=xx'),
-      ('foo\nBUG=', ['a@c'], [], None, None,
+      ('foo\nBUG=', ['a@c'], [],
        'foo\nBUG=\nR=a@c'),
-      ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [], None, None,
+      ('foo\nR=xx\nTBR=yy\nR=bar', ['a@c'], [],
        'foo\n\nR=a@c, bar, xx\nTBR=yy'),
-      ('foo', ['a@c', 'b@c'], [], None, None,
+      ('foo', ['a@c', 'b@c'], [],
        'foo\n\nR=a@c, b@c'),
-      ('foo\nBar\n\nR=\nBUG=', ['c@c'], [], None, None,
+      ('foo\nBar\n\nR=\nBUG=', ['c@c'], [],
        'foo\nBar\n\nR=c@c\nBUG='),
-      ('foo\nBar\n\nR=\nBUG=\nR=', ['c@c'], [], None, None,
+      ('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'], [], None, None,
+        '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'], [], None, None,
+      ('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'], None, None,
+      ('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'),
-      # 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, add_to, add, _expected in data:
+    for orig, reviewers, tbrs, _expected in data:
       obj = git_cl.ChangeDescription(orig)
-      obj.update_reviewers(reviewers, tbrs, add_to, add)
+      obj.update_reviewers(reviewers, tbrs, None, None, None)
       actual.append(obj.description)
     self.assertEqual(expected, actual)
 
@@ -2773,7 +2775,7 @@ class TestGitCl(unittest.TestCase):
         squash=False,
         squash_mode='override_nosquash',
         change_id='I123456789',
-        default_branch='main')
+        new_default=True)
 
 
 class ChangelistTest(unittest.TestCase):