From 0ffdf2de157ed4252983254913f8c0b5e231d99c Mon Sep 17 00:00:00 2001 From: Aaron Gable Date: Mon, 5 Jun 2017 13:01:17 -0700 Subject: [PATCH] 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 Reviewed-by: Andrii Shyshkalov --- gerrit_util.py | 6 ++++ git_cl.py | 76 ++++++++++++++++++++++++++++++++++------ tests/git_cl_test.py | 82 +++++++++++++++++++++++++++++++++++--------- 3 files changed, 136 insertions(+), 28 deletions(-) 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__':