diff --git a/gerrit_util.py b/gerrit_util.py index 24fd323db0..6045de7676 100755 --- a/gerrit_util.py +++ b/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 diff --git a/git_cl.py b/git_cl.py index a935c63c10..fdc671a80d 100755 --- a/git_cl.py +++ b/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 diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py index cb7228ebbc..b966c06716 100755 --- a/tests/git_cl_test.py +++ b/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__':