From e52678e0edb82fc2faa292f402b665c5e3e86c06 Mon Sep 17 00:00:00 2001 From: "maruel@chromium.org" Date: Fri, 26 Apr 2013 18:34:44 +0000 Subject: [PATCH] Update the R= line with the actual list of reviewers that approved the CL. This makes the commit logs much more useful for a build sheriff. Not only he sees who committed the CL but see who approved it directly at the log. This should help build sheriffs when they fail to contact the author and want to fallback on the reviewer for quick questions. R=dpranke@chromium.org BUG=76730 Review URL: https://chromiumcodereview.appspot.com/13800021 git-svn-id: svn://svn.chromium.org/chrome/trunk/tools/depot_tools@196786 0039d316-1c4b-4281-b951-d872f2087c98 --- gcl.py | 10 ++++++++++ git_cl.py | 24 ++++++++++++++++++++++++ tests/gcl_unittest.py | 9 +++++++-- tests/git_cl_test.py | 14 ++++++++------ 4 files changed, 49 insertions(+), 8 deletions(-) diff --git a/gcl.py b/gcl.py index 6691a601ca..11d58e07ba 100755 --- a/gcl.py +++ b/gcl.py @@ -299,6 +299,9 @@ class ChangeInfo(object): def get_reviewers(self): return self._desc.get_reviewers() + def update_reviewers(self, reviewers): + self._desc.update_reviewers(reviewers) + def NeedsUpload(self): return self.needs_upload @@ -383,6 +386,11 @@ class ChangeInfo(object): self._desc = git_cl.ChangeDescription( self.SendToRietveld('/%d/description' % self.issue)) + def GetApprovingReviewers(self): + """Returns the issue reviewers list from Rietveld.""" + return git_cl.get_approving_reviewers( + self.rietveld.get_issue_properties(self.issue, False)) + def AddComment(self, comment): """Adds a comment for an issue on Rietveld. As a side effect, this will email everyone associated with the issue.""" @@ -995,6 +1003,8 @@ def CMDcommit(change_info, args): # Get the latest description from Rietveld. change_info.UpdateDescriptionFromIssue() + change_info.update_reviewers(change_info.GetApprovingReviewers()) + commit_desc = git_cl.ChangeDescription(change_info.description) if change_info.issue: server = change_info.rietveld diff --git a/git_cl.py b/git_cl.py index ce7c6df125..7b708fc5b3 100755 --- a/git_cl.py +++ b/git_cl.py @@ -647,6 +647,10 @@ or verify this branch is set up to track another (via the --track argument to return self.RpcServer().get( '/download/issue%s_%s.diff' % (issue, patchset)) + def GetApprovingReviewers(self, issue): + return get_approving_reviewers( + self.RpcServer().get_issue_properties(int(issue), True)) + def SetIssue(self, issue): """Set this branch's issue. If issue=0, clears the issue.""" if issue: @@ -873,6 +877,22 @@ class ChangeDescription(object): return cleanup_list(reviewers) +def get_approving_reviewers(props): + """Retrieves the reviewers that approved a CL from the issue properties with + messages. + + Note that the list may contain reviewers that are not committer, thus are not + considered by the CQ. + """ + return sorted( + set( + message['sender'] + for message in props['messages'] + if message['approval'] and message['sender'] in props['reviewers'] + ) + ) + + def FindCodereviewSettingsFile(filename='codereview.settings'): """Finds the given file starting in the cwd and going up. @@ -1486,6 +1506,10 @@ def SendUpstream(parser, args, cmd): # Keep a separate copy for the commit message, because the commit message # contains the link to the Rietveld issue, while the Rietveld message contains # the commit viewvc url. + # Keep a separate copy for the commit message. + if cl.GetIssue(): + change_desc.update_reviewers(cl.GetApprovingReviewers(cl.GetIssue())) + commit_desc = ChangeDescription(change_desc.description) if cl.GetIssue(): commit_desc.append_footer('Review URL: %s' % cl.GetIssueURL()) diff --git a/tests/gcl_unittest.py b/tests/gcl_unittest.py index 1030c5b757..81823ab738 100755 --- a/tests/gcl_unittest.py +++ b/tests/gcl_unittest.py @@ -192,14 +192,15 @@ class ChangeInfoUnittest(GclTestsBase): self.mox.ReplayAll() members = [ 'AddComment', 'CloseIssue', 'Delete', 'Exists', 'GetFiles', - 'GetFileNames', 'GetLocalRoot', 'GetIssueDescription', 'Load', + 'GetApprovingReviewers', 'GetFileNames', 'GetIssueDescription', + 'GetLocalRoot', 'Load', 'MissingTests', 'NeedsUpload', 'PrimeLint', 'RpcServer', 'Save', 'SendToRietveld', 'SEPARATOR', 'UpdateDescriptionFromIssue', 'UpdateRietveldDescription', 'append_footer', 'description', 'force_description', 'get_reviewers', 'issue', 'name', - 'needs_upload', 'patch', 'patchset', 'rietveld', + 'needs_upload', 'patch', 'patchset', 'rietveld', 'update_reviewers', ] # If this test fails, you should add the relevant test. self.compareMembers( @@ -576,6 +577,8 @@ class CMDCommitUnittest(GclTestsBase): self.mockCommit( change_info, 'deescription\n\nReview URL: https://my_server/1', '') change_info.UpdateDescriptionFromIssue() + change_info.GetApprovingReviewers().AndReturn(['a@c']) + change_info.update_reviewers(['a@c']) self.mox.ReplayAll() retval = gcl.CMDcommit(['naame']) @@ -594,6 +597,8 @@ class CMDCommitUnittest(GclTestsBase): 'deescription\n\nReview URL: https://my_server/1', '\nCommitted revision 12345') change_info.UpdateDescriptionFromIssue() + change_info.GetApprovingReviewers().AndReturn(['a@c']) + change_info.update_reviewers(['a@c']) change_info.append_footer('Committed: http://view/12345') self.mox.ReplayAll() diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index 797c4ccaee..d6610653a0 100755 --- a/tests/git_cl_test.py +++ b/tests/git_cl_test.py @@ -98,11 +98,13 @@ class TestGitCl(TestCase): self.calls, '@%d Expected: Actual: %r' % (self._calls_done, args)) expected_args, result = self.calls.pop(0) - self.assertEquals( - expected_args, - args, - '@%d Expected: %r Actual: %r' % ( - self._calls_done, expected_args, args)) + # Also logs otherwise it could get caught in a try/finally and be hard to + # diagnose. + if expected_args != args: + msg = '@%d Expected: %r Actual: %r' % ( + self._calls_done, expected_args, args) + git_cl.logging.error(msg) + self.fail(msg) self._calls_done += 1 return result @@ -284,7 +286,7 @@ class TestGitCl(TestCase): ((['git', 'checkout', '-q', '-b', 'git-cl-commit'],), ''), ((['git', 'reset', '--soft', 'fake_ancestor_sha'],), ''), ((['git', 'commit', '-m', - 'Issue: 12345\n\n' + 'Issue: 12345\n\nR=john@chromium.org\n\n' 'Review URL: https://codereview.example.com/12345'],), ''), ((['git', 'svn', 'dcommit', '-C50', '--no-rebase', '--rmdir'],),