Revert of Implement owners check in presubmit for Gerrit. (patchset #3 id:40001 of https://codereview.chromium.org/1927773002/ )

Reason for revert:
Breaks presubmit:
https://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/175120/steps/presubmit/logs/stdio

Original issue's description:
> Implement owners check in presubmit for Gerrit.
> 
> R=machenbach@chromium.org,phajdan.jr@chromium.org
> BUG=605563
> 
> Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=300320

TBR=phajdan.jr@chromium.org,tandrii@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=605563

Review-Url: https://codereview.chromium.org/1928343002

git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@300326 0039d316-1c4b-4281-b951-d872f2087c98

git_cl.py

@@ -1317,8 +1317,7 @@ class Changelist(object):
       return presubmit_support.DoPresubmitChecks(change, committing,
           verbose=verbose, output_stream=sys.stdout, input_stream=sys.stdin,
           default_presubmit=None, may_prompt=may_prompt,
-          rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit(),
-          gerrit_obj=self._codereview_impl.GetGerritObjForPresubmit())
+          rietveld_obj=self._codereview_impl.GetRieveldObjForPresubmit())
     except presubmit_support.PresubmitFailure, e:
       DieWithError(
           ('%s\nMaybe your depot_tools is out of date?\n'
@@ -1502,10 +1501,6 @@ class _ChangelistCodereviewBase(object):
     # For non-Rietveld codereviews, this probably should return a dummy object.
     raise NotImplementedError()
 
-  def GetGerritObjForPresubmit(self):
-    # None is valid return value, otherwise presubmit_support.GerritAccessor.
-    return None
-
   def UpdateDescriptionRemote(self, description):
     """Update the description on codereview site."""
     raise NotImplementedError()
@@ -2112,9 +2107,6 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
         raise NotImplementedError()
     return ThisIsNotRietveldIssue()
 
-  def GetGerritObjForPresubmit(self):
-    return presubmit_support.GerritAccessor(self._GetGerritHost())
-
   def GetStatus(self):
     """Apply a rough heuristic to give a simple summary of an issue's review
     or CQ status, assuming adherence to a common workflow.

presubmit_canned_checks.py

@@ -875,7 +875,8 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
       input_api.change.AffectedFiles(file_filter=source_file_filter)])
 
   owners_db = input_api.owners_db
-  owner_email, reviewers = _CodereviewOwnersAndReviewers(
+  # TODO(tandrii): this will always return None, set() in case of Gerrit.
+  owner_email, reviewers = _RietveldOwnerAndReviewers(
       input_api,
       owners_db.email_regexp,
       approval_needed=input_api.is_committing)
@@ -904,26 +905,6 @@ def CheckOwners(input_api, output_api, source_file_filter=None):
     return [output('Missing LGTM from someone other than %s' % owner_email)]
   return []
 
-def _CodereviewOwnersAndReviewers(input_api, email_regexp, approval_needed):
-  """Return the owner and reviewers of a change, if any.
-
-  If approval_needed is True, only reviewers who have approved the change
-  will be returned.
-  """
-  if input_api.change.issue:
-    if input_api.gerrit:
-      res = _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed)
-    else:
-      # Rietveld is default.
-      res = _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed)
-    if res:
-      return res
-
-  reviewers = set()
-  if not approval_needed:
-    reviewers = _ReviewersFromChange(input_api.change)
-  return None, reviewers
-
 
 def _GetRietveldIssueProps(input_api, messages):
   """Gets the issue properties from rietveld."""
@@ -945,49 +926,33 @@ def _ReviewersFromChange(change):
   return set(reviewer for reviewer in reviewers if '@' in reviewer)
 
 
-def _match_reviewer_email(r, owner_email, email_regexp):
-  return email_regexp.match(r) and r != owner_email
-
 def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
   """Return the owner and reviewers of a change, if any.
 
   If approval_needed is True, only reviewers who have approved the change
   will be returned.
-  Returns None if can't fetch issue properties from codereview.
   """
   issue_props = _GetRietveldIssueProps(input_api, True)
   if not issue_props:
-    return None
+    reviewers = set()
+    if not approval_needed:
+      reviewers = _ReviewersFromChange(input_api.change)
+    return None, reviewers
 
   if not approval_needed:
     return issue_props['owner_email'], set(issue_props['reviewers'])
 
   owner_email = issue_props['owner_email']
 
+  def match_reviewer(r):
+    return email_regexp.match(r) and r != owner_email
+
   messages = issue_props.get('messages', [])
   approvers = set(
       m['sender'] for m in messages
-      if m.get('approval') and _match_reviewer_email(m['sender'], owner_email,
-                                                     email_regexp))
-  return owner_email, approvers
-
-
-def _GerritOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
-  """Return the owner and reviewers of a change, if any.
+      if m.get('approval') and match_reviewer(m['sender']))
 
-  If approval_needed is True, only reviewers who have approved the change
-  will be returned.
-  Returns None if can't fetch issue properties from codereview.
-  """
-  issue = input_api.change.issue
-  if not issue:
-    return None
-
-  owner_email = input_api.gerrit.GetChangeOwner(issue)
-  reviewers = set(
-      r for r in input_api.gerrit.GetChangeReviewers(issue, approval_needed)
-      if _match_reviewer_email(r, owner_email, email_regexp))
-  return owner_email, reviewers
+  return owner_email, approvers
 
 
 def _CheckConstNSObject(input_api, output_api, source_file_filter):

presubmit_support.py

@@ -197,73 +197,6 @@ class _MailTextResult(_PresubmitResult):
     super(_MailTextResult, self).__init__()
     raise NotImplementedError()
 
-class GerritAccessor(object):
-  """Limited Gerrit functionality for canned presubmit checks to work.
-
-  To avoid excessive Gerrit calls, caches the results.
-  """
-
-  def __init__(self, host):
-    self.host = host
-    self.cache = {}
-
-  def _FetchChangeDetail(self, issue):
-    # Separate function to be easily mocked in tests.
-    return gerrit_util.GetChangeDetail(
-        self.host, str(issue),
-        ['ALL_REVISIONS', 'DETAILED_LABELS'])
-
-  def GetChangeInfo(self, issue):
-    """Returns labels and all revisions (patchsets) for this issue.
-
-    The result is a dictionary according to Gerrit REST Api.
-    https://gerrit-review.googlesource.com/Documentation/rest-api.html
-
-    However, API isn't very clear what's inside, so see tests for example.
-    """
-    assert issue
-    cache_key = int(issue)
-    if cache_key not in self.cache:
-      self.cache[cache_key] = self._FetchChangeDetail(issue)
-    return self.cache[cache_key]
-
-  def GetChangeDescription(self, issue, patchset=None):
-    """If patchset is none, fetches current patchset."""
-    info = self.GetChangeInfo(issue)
-    # info is a reference to cache. We'll modify it here adding description to
-    # it to the right patchset, if it is not yet there.
-
-    # Find revision info for the patchset we want.
-    if patchset is not None:
-      for rev, rev_info in info['revisions'].iteritems():
-        if str(rev_info['_number']) == str(patchset):
-          break
-      else:
-        raise Exception('patchset %s doesn\'t exist in issue %s' % (
-            patchset, issue))
-    else:
-      rev = info['current_revision']
-      rev_info = info['revisions'][rev]
-
-    # Updates revision info, which is part of cached issue info.
-    if 'real_description' not in rev_info:
-      rev_info['real_description'] = (
-          gerrit_util.GetChangeDescriptionFromGitiles(
-              rev_info['fetch']['http']['url'], rev))
-    return rev_info['real_description']
-
-  def GetChangeOwner(self, issue):
-    return self.GetChangeInfo(issue)['owner']['email']
-
-  def GetChangeReviewers(self, issue, approving_only=True):
-    # Gerrit has 'approved' sub-section, but it only lists 1 approver.
-    # So, if we look only for approvers, we have to look at all anyway.
-    # Also, assume LGTM means Code-Review label == 2. Other configurations
-    # aren't supported.
-    return [r['email']
-            for r in self.GetChangeInfo(issue)['labels']['Code-Review']['all']
-            if not approving_only or '2' == str(r.get('value', 0))]
-
 
 class OutputApi(object):
   """An instance of OutputApi gets passed to presubmit scripts so that they
@@ -332,7 +265,7 @@ class InputApi(object):
   )
 
   def __init__(self, change, presubmit_path, is_committing,
-      rietveld_obj, verbose, gerrit_obj=None, dry_run=None):
+      rietveld_obj, verbose, dry_run=None):
     """Builds an InputApi object.
 
     Args:
@@ -340,15 +273,12 @@ class InputApi(object):
       presubmit_path: The path to the presubmit script being processed.
       is_committing: True if the change is about to be committed.
       rietveld_obj: rietveld.Rietveld client object
-      gerrit_obj: provides basic Gerrit codereview functionality.
-      dry_run: if true, some Checks will be skipped.
     """
     # Version number of the presubmit_support script.
     self.version = [int(x) for x in __version__.split('.')]
     self.change = change
     self.is_committing = is_committing
     self.rietveld = rietveld_obj
-    self.gerrit = gerrit_obj
     self.dry_run = dry_run
     # TBD
     self.host_url = 'http://codereview.chromium.org'
@@ -1419,19 +1349,16 @@ def DoPostUploadExecuter(change,
 
 class PresubmitExecuter(object):
   def __init__(self, change, committing, rietveld_obj, verbose,
-               gerrit_obj=None, dry_run=None):
+               dry_run=None):
     """
     Args:
       change: The Change object.
       committing: True if 'gcl commit' is running, False if 'gcl upload' is.
       rietveld_obj: rietveld.Rietveld client object.
-      gerrit_obj: provides basic Gerrit codereview functionality.
-      dry_run: if true, some Checks will be skipped.
     """
     self.change = change
     self.committing = committing
     self.rietveld = rietveld_obj
-    self.gerrit = gerrit_obj
     self.verbose = verbose
     self.dry_run = dry_run
 
@@ -1454,7 +1381,7 @@ class PresubmitExecuter(object):
     # Load the presubmit script into context.
     input_api = InputApi(self.change, presubmit_path, self.committing,
                          self.rietveld, self.verbose,
-                         self.dry_run, self.gerrit)
+                         dry_run=self.dry_run)
     context = {}
     try:
       exec script_text in context
@@ -1497,7 +1424,6 @@ def DoPresubmitChecks(change,
                       default_presubmit,
                       may_prompt,
                       rietveld_obj,
-                      gerrit_obj=None,
                       dry_run=None):
   """Runs all presubmit checks that apply to the files in the change.
 
@@ -1517,7 +1443,6 @@ def DoPresubmitChecks(change,
     default_presubmit: A default presubmit script to execute in any case.
     may_prompt: Enable (y/n) questions on warning or error.
     rietveld_obj: rietveld.Rietveld object.
-    gerrit_obj: provides basic Gerrit codereview functionality.
     dry_run: if true, some Checks will be skipped.
 
   Warning:
@@ -1546,7 +1471,7 @@ def DoPresubmitChecks(change,
       output.write("Warning, no PRESUBMIT.py found.\n")
     results = []
     executer = PresubmitExecuter(change, committing, rietveld_obj, verbose,
-                                 gerrit_obj, dry_run)
+                                 dry_run=dry_run)
     if default_presubmit:
       if verbose:
         output.write("Running default presubmit script.\n")
@@ -1771,8 +1696,7 @@ def main(argv=None):
     parser.error('For unversioned directory, <files> is not optional.')
   logging.info('Found %d file(s).' % len(files))
 
-  rietveld_obj, gerrit_obj = None, None
-
+  rietveld_obj = None
   if options.rietveld_url:
     # The empty password is permitted: '' is not None.
     if options.rietveld_private_key_file:
@@ -1794,11 +1718,21 @@ def main(argv=None):
       logging.info('Got description: """\n%s\n"""', options.description)
 
   if options.gerrit_url and options.gerrit_fetch:
-    assert options.issue and options.patchset
     rietveld_obj = None
-    gerrit_obj = GerritAccessor(urlparse.urlparse(options.gerrit_url).netloc)
-    options.author = gerrit_obj.GetChangeOwner(options.issue)
-    options.description = gerrit_obj.GetChangeDescription(options.patchset)
+    assert options.issue and options.patchset
+    props = gerrit_util.GetChangeDetail(
+        urlparse.urlparse(options.gerrit_url).netloc, str(options.issue),
+        ['ALL_REVISIONS'])
+    options.author = props['owner']['email']
+    for rev, rev_info in props['revisions'].iteritems():
+      if str(rev_info['_number']) == str(options.patchset):
+        options.description = gerrit_util.GetChangeDescriptionFromGitiles(
+            rev_info['fetch']['http']['url'], rev)
+        break
+    else:
+      print >> sys.stderr, ('Patchset %d was not found in Gerrit issue %d' %
+                            options.patchset, options.issue)
+      return 2
     logging.info('Got author: "%s"', options.author)
     logging.info('Got description: """\n%s\n"""', options.description)
 
@@ -1820,7 +1754,6 @@ def main(argv=None):
           options.default_presubmit,
           options.may_prompt,
           rietveld_obj,
-          gerrit_obj,
           options.dry_run)
     return not results.should_continue()
   except NonexistantCannedCheckFilter, e:

tests/presubmit_unittest.py

@@ -184,7 +184,6 @@ class PresubmitUnittest(PresubmitTestsBase):
       'subprocess', 'sys', 'tempfile', 'time', 'traceback', 'types', 'unittest',
       'urllib2', 'warn', 'multiprocessing', 'DoGetTryMasters',
       'GetTryMastersExecuter', 'itertools', 'urlparse', 'gerrit_util',
-      'GerritAccessor',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(presubmit, members)
@@ -829,8 +828,7 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     output = presubmit.DoPresubmitChecks(
-        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None, None,
-        None)
+        change, False, True, None, input_buf, DEFAULT_SCRIPT, False, None)
     self.failIf(output.should_continue())
     text = (
         'Running presubmit upload checks ...\n'
@@ -912,8 +910,7 @@ def CheckChangeOnCommit(input_api, output_api):
         0,
         None)
     self.failUnless(presubmit.DoPresubmitChecks(
-        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None,
-        None))
+        change, False, True, output, input_buf, DEFAULT_SCRIPT, False, None))
     self.assertEquals(output.getvalue(),
                       ('Running presubmit upload checks ...\n'
                        'Warning, no PRESUBMIT.py found.\n'
@@ -1156,7 +1153,7 @@ def CheckChangeOnCommit(input_api, output_api):
     presubmit.DoPresubmitChecks(mox.IgnoreArg(), False, False,
                                 mox.IgnoreArg(),
                                 mox.IgnoreArg(),
-                                None, False, None, None, None).AndReturn(output)
+                                None, False, None, None).AndReturn(output)
     self.mox.ReplayAll()
 
     self.assertEquals(
@@ -1200,7 +1197,7 @@ class InputApiUnittest(PresubmitTestsBase):
       'os_walk', 'os_path', 'os_stat', 'owners_db', 'pickle', 'platform',
       'python_executable', 're', 'rietveld', 'subprocess', 'tbr', 'tempfile',
       'time', 'traceback', 'unittest', 'urllib2', 'version', 'verbose',
-      'dry_run', 'gerrit',
+      'dry_run',
     ]
     # If this test fails, you should add the relevant test.
     self.compareMembers(
@@ -1842,7 +1839,6 @@ class CannedChecksUnittest(PresubmitTestsBase):
     input_api.os_path = presubmit.os.path
     input_api.re = presubmit.re
     input_api.rietveld = self.mox.CreateMock(rietveld.Rietveld)
-    input_api.gerrit = None
     input_api.traceback = presubmit.traceback
     input_api.urllib2 = self.mox.CreateMock(presubmit.urllib2)
     input_api.unittest = unittest
@@ -2565,8 +2561,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
         presubmit.OutputApi.PresubmitNotifyResult)
 
   def AssertOwnersWorks(self, tbr=False, issue='1', approvers=None,
-      reviewers=None, is_committing=True,
-      rietveld_response=None, gerrit_response=None,
+      reviewers=None, is_committing=True, rietveld_response=None,
       uncovered_files=None, expected_output='',
       manually_specified_reviewers=None, dry_run=None):
     if approvers is None:
@@ -2589,11 +2584,6 @@ class CannedChecksUnittest(PresubmitTestsBase):
     change.TBR = ''
     affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile)
     input_api = self.MockInputApi(change, False)
-    if gerrit_response:
-      assert not rietveld_response
-      input_api.rietveld = None
-      input_api.gerrit = presubmit.GerritAccessor('host')
-
     fake_db = self.mox.CreateMock(owners.Database)
     fake_db.email_regexp = input_api.re.compile(owners.BASIC_EMAIL_REGEXP)
     input_api.owners_db = fake_db
@@ -2605,7 +2595,7 @@ class CannedChecksUnittest(PresubmitTestsBase):
       if not dry_run:
         affected_file.LocalPath().AndReturn('foo/xyz.cc')
         change.AffectedFiles(file_filter=None).AndReturn([affected_file])
-      if issue and not rietveld_response and not gerrit_response:
+      if issue and not rietveld_response:
         rietveld_response = {
           "owner_email": change.author_email,
           "messages": [
@@ -2622,12 +2612,9 @@ class CannedChecksUnittest(PresubmitTestsBase):
 
       if not dry_run:
         if issue:
-          if rietveld_response:
-            input_api.rietveld.get_issue_properties(
-                issue=int(input_api.change.issue), messages=True).AndReturn(
-                    rietveld_response)
-          elif gerrit_response:
-            input_api.gerrit._FetchChangeDetail = lambda _: gerrit_response
+          input_api.rietveld.get_issue_properties(
+              issue=int(input_api.change.issue), messages=True).AndReturn(
+                  rietveld_response)
 
         people.add(change.author_email)
         fake_db.files_not_covered_by(set(['foo/xyz.cc']),
@@ -2660,39 +2647,6 @@ class CannedChecksUnittest(PresubmitTestsBase):
         rietveld_response=response,
         expected_output='')
 
-  def testCannedCheckOwners_Approved_Gerrit(self):
-    response = {
-      "owner": {"email": "john@example.com"},
-      "labels": {"Code-Review": {
-        u'all': [
-          {
-            u'email': u'john@example.com',  # self +1 :)
-            u'value': 1
-          },
-          {
-            u'email': u'ben@example.com',
-            u'value': 2
-          },
-        ],
-        u'approved': {u'email': u'ben@example.org'},
-        u'default_value': 0,
-        u'values': {u' 0': u'No score',
-                    u'+1': u'Looks good to me, but someone else must approve',
-                    u'+2': u'Looks good to me, approved',
-                    u'-1': u"I would prefer that you didn't submit this",
-                    u'-2': u'Do not submit'}
-      }},
-    }
-    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
-        gerrit_response=response,
-        is_committing=True,
-        expected_output='')
-
-    self.AssertOwnersWorks(approvers=set(['ben@example.com']),
-        is_committing=False,
-        gerrit_response=response,
-        expected_output='')
-
   def testCannedCheckOwners_Approved(self):
     response = {
       "owner_email": "john@example.com",