git-cl-comments: Support Gerrit file- and line-comments

This brings the gerrit version of "git cl comments" into line
with the Rietveld implementation by including file- and line-level
comments as well as top-level review comments. It requires an
extra API call to do so, so this may result in some slow-down, but
the result is worth it.

It formats the comments to match the formatting used in the
PolyGerrit UI, with the addition of visible URLs linking to
the comment since we can't hyperlink text in the terminal.

This CL also causes it to ignore messages and comments with
the 'autogenerated' tag, which are generally less interesting
and clutter the output.

Bug: 726514
Change-Id: I1fd939d90259b43886ddc209c0e727eab36cc9c9
Reviewed-on: https://chromium-review.googlesource.com/520722
Commit-Queue: Aaron Gable <agable@chromium.org>
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>

gerrit_util.py

@@ -561,6 +561,12 @@ def GetChangeReview(host, change, revision=None):
   return ReadHttpJsonResponse(CreateHttpConn(host, path))
 
 
+def GetChangeComments(host, change):
+  """Get the line- and file-level comments on a change."""
+  path = 'changes/%s/comments' % change
+  return ReadHttpJsonResponse(CreateHttpConn(host, path))
+
+
 def AbandonChange(host, change, msg=''):
   """Abandon a gerrit change."""
   path = 'changes/%s/abandon' % change

git_cl.py

@@ -1713,13 +1713,13 @@ class Changelist(object):
   def AddComment(self, message):
     return self._codereview_impl.AddComment(message)
 
-  def GetCommentsSummary(self):
+  def GetCommentsSummary(self, readable=True):
     """Returns list of _CommentSummary for each comment.
 
-    Note: comments per file or per line are not included,
-    only top-level comments are returned.
+    args:
+    readable: determines whether the output is designed for a human or a machine
     """
-    return self._codereview_impl.GetCommentsSummary()
+    return self._codereview_impl.GetCommentsSummary(readable)
 
   def CloseIssue(self):
     return self._codereview_impl.CloseIssue()
@@ -1820,7 +1820,7 @@ class _ChangelistCodereviewBase(object):
     """Posts a comment to the codereview site."""
     raise NotImplementedError()
 
-  def GetCommentsSummary(self):
+  def GetCommentsSummary(self, readable=True):
     raise NotImplementedError()
 
   def CloseIssue(self):
@@ -1996,7 +1996,7 @@ class _RietveldChangelistImpl(_ChangelistCodereviewBase):
   def AddComment(self, message):
     return self.RpcServer().add_comment(self.GetIssue(), message)
 
-  def GetCommentsSummary(self):
+  def GetCommentsSummary(self, _readable=True):
     summary = []
     for message in self.GetIssueProperties().get('messages', []):
       date = datetime.datetime.strptime(message['date'], '%Y-%m-%d %H:%M:%S.%f')
@@ -2581,18 +2581,67 @@ class _GerritChangelistImpl(_ChangelistCodereviewBase):
     gerrit_util.SetReview(self._GetGerritHost(), self.GetIssue(),
                           msg=message)
 
-  def GetCommentsSummary(self):
+  def GetCommentsSummary(self, readable=True):
     # DETAILED_ACCOUNTS is to get emails in accounts.
-    data = self._GetChangeDetail(options=['MESSAGES', 'DETAILED_ACCOUNTS'])
+    messages = self._GetChangeDetail(
+        options=['MESSAGES', 'DETAILED_ACCOUNTS']).get('messages', [])
+    file_comments = gerrit_util.GetChangeComments(
+        self._GetGerritHost(), self.GetIssue())
+
+    # Build dictionary of file comments for easy access and sorting later.
+    # {author+date: {path: {patchset: {line: url+message}}}}
+    comments = collections.defaultdict(
+        lambda: collections.defaultdict(lambda: collections.defaultdict(dict)))
+    for path, line_comments in file_comments.iteritems():
+      for comment in line_comments:
+        if comment.get('tag', '').startswith('autogenerated'):
+          continue
+        key = (comment['author']['email'], comment['updated'])
+        if comment.get('side', 'REVISION') == 'PARENT':
+          patchset = 'Base'
+        else:
+          patchset = 'PS%d' % comment['patch_set']
+        line = comment.get('line', 0)
+        url = ('https://%s/c/%s/%s/%s#%s%s' %
+            (self._GetGerritHost(), self.GetIssue(), comment['patch_set'], path,
+             'b' if comment.get('side') == 'PARENT' else '',
+             str(line) if line else ''))
+        comments[key][path][patchset][line] = (url, comment['message'])
+
     summary = []
-    for msg in data.get('messages', []):
+    for msg in messages:
+      # Don't bother showing autogenerated messages.
+      if msg.get('tag') and msg.get('tag').startswith('autogenerated'):
+        continue
       # Gerrit spits out nanoseconds.
       assert len(msg['date'].split('.')[-1]) == 9
       date = datetime.datetime.strptime(msg['date'][:-3],
                                         '%Y-%m-%d %H:%M:%S.%f')
+      message = msg['message']
+      key = (msg['author']['email'], msg['date'])
+      if key in comments:
+        message += '\n'
+      for path, patchsets in sorted(comments.get(key, {}).items()):
+        if readable:
+          message += '\n%s' % path
+        for patchset, lines in sorted(patchsets.items()):
+          for line, (url, content) in sorted(lines.items()):
+            if line:
+              line_str = 'Line %d' % line
+              path_str = '%s:%d:' % (path, line)
+            else:
+              line_str = 'File comment'
+              path_str = '%s:0:' % path
+            if readable:
+              message += '\n  %s, %s: %s' % (patchset, line_str, url)
+              message += '\n  %s\n' % content
+            else:
+              message += '\n%s ' % path_str
+              message += '\n%s\n' % content
+
       summary.append(_CommentSummary(
         date=date,
-        message=msg['message'],
+        message=message,
         sender=msg['author']['email'],
         # These could be inferred from the text messages and correlated with
         # Code-Review label maximum, however this is not reliable.
@@ -4398,6 +4447,10 @@ def CMDcomments(parser, args):
   parser.add_option('-i', '--issue', dest='issue',
                     help='review issue id (defaults to current issue). '
                          'If given, requires --rietveld or --gerrit')
+  parser.add_option('-m', '--machine-readable', dest='readable',
+                    action='store_false', default=True,
+                    help='output comments in a format compatible with '
+                         'editor parsing')
   parser.add_option('-j', '--json-file',
                     help='File to write JSON summary to')
   auth.add_auth_options(parser)
@@ -4423,7 +4476,8 @@ def CMDcomments(parser, args):
     cl.AddComment(options.comment)
     return 0
 
-  summary = sorted(cl.GetCommentsSummary(), key=lambda c: c.date)
+  summary = sorted(cl.GetCommentsSummary(readable=options.readable),
+                   key=lambda c: c.date)
   for comment in summary:
     if comment.disapproval:
       color = Fore.RED

tests/git_cl_test.py

@@ -593,6 +593,9 @@ class TestGitCl(TestCase):
     self.mock(git_cl.gerrit_util, 'GetChangeDetail',
               lambda *args, **kwargs: self._mocked_call(
                   'GetChangeDetail', *args, **kwargs))
+    self.mock(git_cl.gerrit_util, 'GetChangeComments',
+              lambda *args, **kwargs: self._mocked_call(
+                  'GetChangeComments', *args, **kwargs))
     self.mock(git_cl.gerrit_util, 'AddReviewers',
               lambda h, i, reviewers, ccs, notify: self._mocked_call(
                   'AddReviewers', h, i, reviewers, ccs, notify))
@@ -3418,7 +3421,7 @@ class TestGitCl(TestCase):
        'https://chromium.googlesource.com/infra/infra'),
       (('GetChangeDetail', 'chromium-review.googlesource.com', '1',
         ['MESSAGES', 'DETAILED_ACCOUNTS']), {
-          'owner': {'email': 'owner@example.com'},
+        'owner': {'email': 'owner@example.com'},
         'messages': [
           {
              u'_revision_number': 1,
@@ -3455,19 +3458,48 @@ class TestGitCl(TestCase):
              u'message': u'Patch Set 2: Code-Review+1',
           },
         ]
-      })
+      }),
+      (('GetChangeComments', 'chromium-review.googlesource.com', 1), {
+        '/COMMIT_MSG': [
+          {
+            'author': {'email': u'reviewer@example.com'},
+            'updated': u'2017-03-17 05:19:37.500000000',
+            'patch_set': 2,
+            'side': 'REVISION',
+            'message': 'Please include a bug link',
+          },
+        ],
+        'codereview.settings': [
+          {
+            'author': {'email': u'owner@example.com'},
+            'updated': u'2017-03-16 20:00:41.000000000',
+            'patch_set': 2,
+            'side': 'PARENT',
+            'line': 42,
+            'message': 'I removed this because it is bad',
+          },
+        ]
+      }),
     ] * 2
     expected_comments_summary = [
       git_cl._CommentSummary(
-        message=u'Patch Set 1:\n\nDry run: CQ is trying da patch...',
-        date=datetime.datetime(2017, 3, 15, 20, 8, 45, 0),
-        disapproval=False, approval=False, sender=u'commit-bot@chromium.org'),
-      git_cl._CommentSummary(
-        message=u'PTAL',
+        message=(
+            u'PTAL\n' +
+            u'\n' +
+            u'codereview.settings\n' +
+            u'  Base, Line 42: https://chromium-review.googlesource.com/' +
+            u'c/1/2/codereview.settings#b42\n' +
+            u'  I removed this because it is bad\n'),
         date=datetime.datetime(2017, 3, 16, 20, 0, 41, 0),
         disapproval=False, approval=False, sender=u'owner@example.com'),
       git_cl._CommentSummary(
-        message=u'Patch Set 2: Code-Review+1',
+        message=(
+            u'Patch Set 2: Code-Review+1\n' +
+            u'\n' +
+            u'/COMMIT_MSG\n' +
+            u'  PS2, File comment: https://chromium-review.googlesource.com/' +
+            u'c/1/2//COMMIT_MSG#\n' +
+            u'  Please include a bug link\n'),
         date=datetime.datetime(2017, 3, 17, 5, 19, 37, 500000),
         disapproval=False, approval=False, sender=u'reviewer@example.com'),
     ]
@@ -3480,15 +3512,31 @@ class TestGitCl(TestCase):
                                        '-j', out_file]))
       with open(out_file) as f:
         read = json.load(f)
-      self.assertEqual(len(read), 3)
-      self.assertEqual(read[0]['date'], u'2017-03-15 20:08:45.000000')
-      self.assertEqual(read[1]['date'], u'2017-03-16 20:00:41.000000')
-      self.assertEqual(read[2], {
-          u'date': u'2017-03-17 05:19:37.500000',
-          u'message': u'Patch Set 2: Code-Review+1',
-          u'approval': False,
-          u'disapproval': False,
-          u'sender': u'reviewer@example.com'})
+      self.assertEqual(len(read), 2)
+      self.assertEqual(read[0], {
+        u'date': u'2017-03-16 20:00:41.000000',
+        u'message': (
+            u'PTAL\n' +
+            u'\n' +
+            u'codereview.settings\n' +
+            u'  Base, Line 42: https://chromium-review.googlesource.com/' +
+            u'c/1/2/codereview.settings#b42\n' +
+            u'  I removed this because it is bad\n'),
+        u'approval': False,
+        u'disapproval': False,
+        u'sender': u'owner@example.com'})
+      self.assertEqual(read[1], {
+        u'date': u'2017-03-17 05:19:37.500000',
+        u'message': (
+            u'Patch Set 2: Code-Review+1\n' +
+            u'\n' +
+            u'/COMMIT_MSG\n' +
+            u'  PS2, File comment: https://chromium-review.googlesource.com/' +
+            u'c/1/2//COMMIT_MSG#\n' +
+            u'  Please include a bug link\n'),
+        u'approval': False,
+        u'disapproval': False,
+        u'sender': u'reviewer@example.com'})
 
 
 if __name__ == '__main__':