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

This is a reland of 968b1fe7

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>

Change-Id: I9f93d755b10517c5296f7095f735ec2295be34e9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/2632840
Auto-Submit: Edward Lesmes <ehmaldonado@chromium.org>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>

git_cl.py

@@ -42,7 +42,6 @@ import git_footers
 import git_new_branch
 import metrics
 import metrics_utils
-import owners
 import owners_client
 import owners_finder
 import presubmit_canned_checks
@@ -1371,11 +1370,29 @@ class Changelist(object):
 
     change_description = ChangeDescription(description, bug, fixed)
 
+    # Fill gaps in OWNERS coverage to tbrs/reviewers if requested.
+    if options.add_owners_to:
+      assert options.add_owners_to in ('TBR', 'R'), options.add_owners_to
+      client = owners_client.DepotToolsClient(
+          root=settings.GetRoot(),
+          branch=self.GetCommonAncestorWithUpstream())
+      status = client.GetFilesApprovalStatus(
+          files, [], options.tbrs + options.reviewers)
+      missing_files = [
+        f for f in files
+        if status[f] == owners_client.OwnersClient.INSUFFICIENT_REVIEWERS
+      ]
+      owners = client.SuggestOwners(missing_files)
+      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)
+
     # Set the reviewer list now so that presubmit checks can access it.
-    if options.reviewers or options.tbrs or options.add_owners_to:
-      change_description.update_reviewers(
-          options.reviewers, options.tbrs, options.add_owners_to, files,
-          self.GetAuthor())
+    if options.reviewers or options.tbrs:
+      change_description.update_reviewers(options.reviewers, options.tbrs)
 
     return change_description
 
@@ -2614,25 +2631,14 @@ 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):
     """Rewrites the R=/TBR= 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.
-      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.
     """
-    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
-
-    if not reviewers and not tbrs and not add_owners_to:
+    if not reviewers and not tbrs:
       return
 
     reviewers = set(reviewers)
@@ -2657,15 +2663,6 @@ class ChangeDescription(object):
         continue
       LOOKUP[match.group(1)].update(cleanup_list([match.group(2).strip()]))
 
-    # 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))
-
     # If any folks ended up in both groups, remove them from tbrs.
     tbrs -= reviewers
 

tests/git_cl_test.py

@@ -592,9 +592,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',
@@ -737,17 +734,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 = {
@@ -774,7 +767,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
@@ -788,8 +781,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 []
@@ -817,11 +809,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,
@@ -1087,7 +1076,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:
@@ -1139,6 +1128,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()
@@ -1158,7 +1150,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()
@@ -1177,7 +1169,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)
@@ -1477,6 +1469,142 @@ class TestGitCl(unittest.TestCase):
     change_id = git_cl.GenerateGerritChangeId('line1\nline2\n')
     self.assertEqual(change_id, 'Ihashchange')
 
+  @mock.patch('git_cl.Settings.GetBugPrefix')
+  @mock.patch('git_cl.Settings.GetRoot')
+  @mock.patch('git_cl.Changelist.FetchDescription')
+  @mock.patch('git_cl.Changelist.GetBranch')
+  @mock.patch('git_cl.Changelist.GetCommonAncestorWithUpstream')
+  @mock.patch('owners_client.OwnersClient.BatchListOwners')
+  def getDescriptionForUploadTest(
+      self, mockBatchListOwners=None, mockGetCommonAncestorWithUpstream=None,
+      mockGetBranch=None, mockFetchDescription=None, mockGetRoot=None,
+      mockGetBugPrefix=None,
+      initial_description='desc', bug=None, fixed=None, branch='branch',
+      reviewers=None, tbrs=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'],
+      'c': ['c@example.com'],
+    }
+    mockGetRoot.return_value = 'root'
+    mockGetBranch.return_value = branch
+    mockGetBugPrefix.return_value = 'prefix'
+    mockGetCommonAncestorWithUpstream.return_value = 'upstream'
+    mockFetchDescription.return_value = 'desc'
+    mockBatchListOwners.side_effect = lambda ps: {
+        p: owners_by_path.get(p)
+        for p in ps
+    }
+
+    cl = git_cl.Changelist(issue=1234)
+    actual = cl._GetDescriptionForUpload(
+        options=mock.Mock(
+            bug=bug,
+            fixed=fixed,
+            reviewers=reviewers,
+            tbrs=tbrs,
+            add_owners_to=add_owners_to),
+        git_diff_args=None,
+        files=list(owners_by_path))
+    self.assertEqual(expected_description, actual.description)
+
+  def testGetDescriptionForUpload(self):
+    self.getDescriptionForUploadTest()
+
+  def testGetDescriptionForUpload_Bug(self):
+    self.getDescriptionForUploadTest(
+        bug='1234',
+        expected_description='\n'.join([
+          'desc',
+          '',
+          'Bug: prefix:1234',
+        ]))
+
+  def testGetDescriptionForUpload_Fixed(self):
+    self.getDescriptionForUploadTest(
+        fixed='1234',
+        expected_description='\n'.join([
+          'desc',
+          '',
+          'Fixed: prefix:1234',
+        ]))
+
+
+  def testGetDescriptionForUpload_BugFromBranch(self):
+    self.getDescriptionForUploadTest(
+        branch='bug-1234',
+        expected_description='\n'.join([
+          'desc',
+          '',
+          'Bug: prefix:1234',
+        ]))
+
+  def testGetDescriptionForUpload_FixedFromBranch(self):
+    self.getDescriptionForUploadTest(
+        branch='fix-1234',
+        expected_description='\n'.join([
+          'desc',
+          '',
+          'Fixed: prefix:1234',
+        ]))
+
+  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',
+        ]))
+
+  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):
+    self.getDescriptionForUploadTest(
+        reviewers=['a@example.com', 'b@example.com'],
+        expected_description='\n'.join([
+          'desc',
+          '',
+          '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):
     for init_desc, footer_line, expected_desc in [
       # Use unique desc first lines for easy test failure identification.
@@ -1538,7 +1666,7 @@ class TestGitCl(unittest.TestCase):
     actual = []
     for orig, reviewers, tbrs, _expected in data:
       obj = git_cl.ChangeDescription(orig)
-      obj.update_reviewers(reviewers, tbrs, None, None, None)
+      obj.update_reviewers(reviewers, tbrs)
       actual.append(obj.description)
     self.assertEqual(expected, actual)
 
@@ -2776,7 +2904,7 @@ class TestGitCl(unittest.TestCase):
         squash=False,
         squash_mode='override_nosquash',
         change_id='I123456789',
-        new_default=True)
+        default_branch='main')
 
 
 class ChangelistTest(unittest.TestCase):