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'],),