From b5cded6951df7a3c55453111b7a4b1187d2048a0 Mon Sep 17 00:00:00 2001 From: "isherman@chromium.org" Date: Tue, 25 Mar 2014 17:47:57 +0000 Subject: [PATCH] Infer CL author and reviewer list from local state if the issue has not previously been uploaded. R=agable@chromium.org BUG=none Review URL: https://codereview.chromium.org/195793021 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@259250 0039d316-1c4b-4281-b951-d872f2087c98 --- git_cl.py | 5 +++++ presubmit_canned_checks.py | 15 ++++++++++++++- presubmit_support.py | 38 +++++++++++++++++++++++-------------- tests/presubmit_unittest.py | 30 +++++++++++++++++++++++++++-- 4 files changed, 71 insertions(+), 17 deletions(-) diff --git a/git_cl.py b/git_cl.py index 82d5a78ab..1a1c64847 100755 --- a/git_cl.py +++ b/git_cl.py @@ -1694,6 +1694,11 @@ def CMDupload(parser, args): cl.SetWatchers(watchlist.GetWatchersForPaths(files)) if not options.bypass_hooks: + if options.reviewers: + # Set the reviewer list now so that presubmit checks can access it. + change_description = ChangeDescription(change.FullDescriptionText()) + change_description.update_reviewers(options.reviewers) + change.SetDescriptionText(change_description.description) hook_results = cl.RunHook(committing=False, may_prompt=not options.force, verbose=options.verbose, diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py index d0376bd79..a48a0c627 100644 --- a/presubmit_canned_checks.py +++ b/presubmit_canned_checks.py @@ -852,6 +852,16 @@ def _GetRietveldIssueProps(input_api, messages): issue=int(issue), messages=messages) +def _ReviewersFromChange(change): + """Return the reviewers specified in the |change|, if any.""" + reviewers = set() + if change.R: + reviewers.update(set([r.strip() for r in change.R.split(',')])) + if change.TBR: + reviewers.update(set([r.strip() for r in change.TBR.split(',')])) + return reviewers + + def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): """Return the owner and reviewers of a change, if any. @@ -860,7 +870,10 @@ def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False): """ issue_props = _GetRietveldIssueProps(input_api, True) if not issue_props: - return None, set() + 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']) diff --git a/presubmit_support.py b/presubmit_support.py index ebcf14e01..ae56b068b 100755 --- a/presubmit_support.py +++ b/presubmit_support.py @@ -794,27 +794,16 @@ class Change(object): if files is None: files = [] self._name = name - self._full_description = description # Convert root into an absolute path. self._local_root = os.path.abspath(local_root) self.issue = issue self.patchset = patchset self.author_email = author - # From the description text, build up a dictionary of key/value pairs - # plus the description minus all key/value or "tag" lines. - description_without_tags = [] + self._full_description = '' self.tags = {} - for line in self._full_description.splitlines(): - m = self.TAG_LINE_RE.match(line) - if m: - self.tags[m.group('key')] = m.group('value') - else: - description_without_tags.append(line) - - # Change back to text and remove whitespace at end. - self._description_without_tags = ( - '\n'.join(description_without_tags).rstrip()) + self._description_without_tags = '' + self.SetDescriptionText(description) assert all( (isinstance(f, (list, tuple)) and len(f) == 2) for f in files), files @@ -842,6 +831,27 @@ class Change(object): """Returns the complete changelist description including tags.""" return self._full_description + def SetDescriptionText(self, description): + """Sets the full description text (including tags) to |description|. + + Also updates the list of tags.""" + self._full_description = description + + # From the description text, build up a dictionary of key/value pairs + # plus the description minus all key/value or "tag" lines. + description_without_tags = [] + self.tags = {} + for line in self._full_description.splitlines(): + m = self.TAG_LINE_RE.match(line) + if m: + self.tags[m.group('key')] = m.group('value') + else: + description_without_tags.append(line) + + # Change back to text and remove whitespace at end. + self._description_without_tags = ( + '\n'.join(description_without_tags).rstrip()) + def RepositoryRoot(self): """Returns the repository (checkout) root directory for this change, as an absolute path. diff --git a/tests/presubmit_unittest.py b/tests/presubmit_unittest.py index 6c2c8b3a6..4658a698a 100755 --- a/tests/presubmit_unittest.py +++ b/tests/presubmit_unittest.py @@ -1768,7 +1768,7 @@ class ChangeUnittest(PresubmitTestsBase): 'AbsoluteLocalPaths', 'AffectedFiles', 'AffectedTextFiles', 'DescriptionText', 'FullDescriptionText', 'LocalPaths', 'Name', 'RepositoryRoot', 'RightHandSideLines', 'ServerPaths', - 'TAG_LINE_RE', + 'SetDescriptionText', 'TAG_LINE_RE', 'author_email', 'issue', 'patchset', 'scm', 'tags', ] # If this test fails, you should add the relevant test. @@ -1791,6 +1791,19 @@ class ChangeUnittest(PresubmitTestsBase): self.assertEquals(1, len(change.AffectedFiles(include_dirs=True))) self.assertEquals('Y', change.AffectedFiles(include_dirs=True)[0].Action()) + def testSetDescriptionText(self): + change = presubmit.Change( + '', 'foo\nDRU=ro', self.fake_root_dir, [], 3, 5, '') + self.assertEquals('foo', change.DescriptionText()) + self.assertEquals('foo\nDRU=ro', change.FullDescriptionText()) + self.assertEquals('ro', change.DRU) + + change.SetDescriptionText('bar\nWHIZ=bang') + self.assertEquals('bar', change.DescriptionText()) + self.assertEquals('bar\nWHIZ=bang', change.FullDescriptionText()) + self.assertEquals('bang', change.WHIZ) + self.assertFalse(change.DRU) + def CommHelper(input_api, cmd, ret=None, **kwargs): ret = ret or (('', None), 0) @@ -2574,6 +2587,8 @@ class CannedChecksUnittest(PresubmitTestsBase): change = self.mox.CreateMock(presubmit.Change) change.issue = issue change.author_email = 'john@example.com' + change.R = '' + change.TBR = '' affected_file = self.mox.CreateMock(presubmit.SvnAffectedFile) input_api = self.MockInputApi(change, False) fake_db = self.mox.CreateMock(owners.Database) @@ -2585,7 +2600,7 @@ class CannedChecksUnittest(PresubmitTestsBase): if not is_committing or (not tbr and issue): affected_file.LocalPath().AndReturn('foo/xyz.cc') change.AffectedFiles(file_filter=None).AndReturn([affected_file]) - if not rietveld_response: + if issue and not rietveld_response: rietveld_response = { "owner_email": change.author_email, "messages": [ @@ -2599,6 +2614,7 @@ class CannedChecksUnittest(PresubmitTestsBase): people = approvers else: people = reviewers + change.R = ','.join(reviewers) if issue: input_api.rietveld.get_issue_properties( @@ -2710,6 +2726,16 @@ class CannedChecksUnittest(PresubmitTestsBase): expected_output='Missing OWNER reviewers for these files:\n' ' foo\n') + def testCannedCheckOwners_NoIssueLocalReviewers(self): + self.AssertOwnersWorks(issue=None, + reviewers=set(['jane@example.com']), + expected_output="OWNERS check failed: this change has no Rietveld " + "issue number, so we can't check it for approvals.\n") + self.AssertOwnersWorks(issue=None, + reviewers=set(['jane@example.com']), + is_committing=False, + expected_output='') + def testCannedCheckOwners_NoLGTM(self): self.AssertOwnersWorks(expected_output='Missing LGTM from someone ' 'other than john@example.com\n')