diff --git a/git_cl.py b/git_cl.py index bf949133c..9bf60ea29 100755 --- a/git_cl.py +++ b/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. diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index 1e16d8fff..e21d08675 100644 --- a/presubmit_canned_checks.py +++ b/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): diff --git a/presubmit_support.py b/presubmit_support.py index 57cea9b83..07cf7b24b 100755 --- a/presubmit_support.py +++ b/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, 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: diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 256ed5016..d0c9c10ed 100755 --- a/tests/presubmit_unittest.py +++ b/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",