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

@@ -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

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())

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()
 

tests/git_cl_test.py

@@ -98,11 +98,13 @@ class TestGitCl(TestCase):
         self.calls,
         '@%d  Expected: <Missing>   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'],),