Change git cl comments to list robot comments.
In particular, only show robot comments from the latest patchset.
Bug: 924780
Change-Id: I12038ddd2d90a5cb561b248de3fd37908d5b927e
Reviewed-on: https://chromium-review.googlesource.com/c/1457282
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Quinten Yearsley <qyearsley@chromium.org>
diff --git a/gerrit_util.py b/gerrit_util.py
index 985c7fb..1923b6a 100644
--- a/gerrit_util.py
+++ b/gerrit_util.py
@@ -684,6 +684,12 @@
return ReadHttpJsonResponse(CreateHttpConn(host, path))
+def GetChangeRobotComments(host, change):
+ """Get the line- and file-level robot comments on a change."""
+ path = 'changes/%s/robotcomments' % 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 72a9b92..1631be4 100755
--- a/git_cl.py
+++ b/git_cl.py
@@ -1050,7 +1050,7 @@
_CommentSummary = collections.namedtuple(
- '_CommentSummary', ['date', 'message', 'sender',
+ '_CommentSummary', ['date', 'message', 'sender', 'autogenerated',
# TODO(tandrii): these two aren't known in Gerrit.
'approval', 'disapproval'])
@@ -2112,10 +2112,23 @@
def GetCommentsSummary(self, readable=True):
# DETAILED_ACCOUNTS is to get emails in accounts.
+ # CURRENT_REVISION is included to get the latest patchset so that
+ # only the robot comments from the latest patchset can be shown.
messages = self._GetChangeDetail(
- options=['MESSAGES', 'DETAILED_ACCOUNTS']).get('messages', [])
+ options=['MESSAGES', 'DETAILED_ACCOUNTS',
+ 'CURRENT_REVISION']).get('messages', [])
file_comments = gerrit_util.GetChangeComments(
self._GetGerritHost(), self._GerritChangeIdentifier())
+ robot_file_comments = gerrit_util.GetChangeRobotComments(
+ self._GetGerritHost(), self._GerritChangeIdentifier())
+
+ # Add the robot comments onto the list of comments, but only
+ # keep those that are from the latest pachset.
+ latest_patch_set = self.GetMostRecentPatchset()
+ for path, robot_comments in robot_file_comments.iteritems():
+ line_comments = file_comments.setdefault(path, [])
+ line_comments.extend(
+ [c for c in robot_comments if c['patch_set'] == latest_patch_set])
# Build dictionary of file comments for easy access and sorting later.
# {author+date: {path: {patchset: {line: url+message}}}}
@@ -2123,7 +2136,8 @@
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'):
+ tag = comment.get('tag', '')
+ if tag.startswith('autogenerated') and 'robot_id' not in comment:
continue
key = (comment['author']['email'], comment['updated'])
if comment.get('side', 'REVISION') == 'PARENT':
@@ -2137,48 +2151,58 @@
str(line) if line else ''))
comments[key][path][patchset][line] = (url, comment['message'])
- summary = []
+ summaries = []
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 = self._BuildCommentSummary(msg, comments, readable)
+ if summary:
+ summaries.append(summary)
+ return summaries
- summary.append(_CommentSummary(
- date=date,
- 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.
- # Leaving as is until the need arises.
- approval=False,
- disapproval=False,
- ))
- return summary
+ @staticmethod
+ def _BuildCommentSummary(msg, comments, readable):
+ key = (msg['author']['email'], msg['date'])
+ # Don't bother showing autogenerated messages that don't have associated
+ # file or line comments. this will filter out most autogenerated
+ # messages, but will keep robot comments like those from Tricium.
+ is_autogenerated = msg.get('tag', '').startswith('autogenerated')
+ if is_autogenerated and not comments.get(key):
+ return None
+ message = msg['message']
+ # 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')
+ 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
+
+ return _CommentSummary(
+ date=date,
+ message=message,
+ sender=msg['author']['email'],
+ autogenerated=is_autogenerated,
+ # These could be inferred from the text messages and correlated with
+ # Code-Review label maximum, however this is not reliable.
+ # Leaving as is until the need arises.
+ approval=False,
+ disapproval=False,
+ )
def CloseIssue(self):
gerrit_util.AbandonChange(
@@ -4156,6 +4180,8 @@
color = Fore.GREEN
elif comment.sender == cl.GetIssueOwner():
color = Fore.MAGENTA
+ elif comment.autogenerated:
+ color = Fore.CYAN
else:
color = Fore.BLUE
print('\n%s%s %s%s\n%s' % (
diff --git a/tests/git_cl_test.py b/tests/git_cl_test.py
index c88131b..841a77a 100755
--- a/tests/git_cl_test.py
+++ b/tests/git_cl_test.py
@@ -655,6 +655,9 @@
self.mock(git_cl.gerrit_util, 'GetChangeComments',
lambda *args, **kwargs: self._mocked_call(
'GetChangeComments', *args, **kwargs))
+ self.mock(git_cl.gerrit_util, 'GetChangeRobotComments',
+ lambda *args, **kwargs: self._mocked_call(
+ 'GetChangeRobotComments', *args, **kwargs))
self.mock(git_cl.gerrit_util, 'AddReviewers',
lambda h, i, reviewers, ccs, notify: self._mocked_call(
'AddReviewers', h, i, reviewers, ccs, notify))
@@ -1513,8 +1516,7 @@
expected,
'GetHashTags(%r) == %r, expected %r' % (desc, actual, expected))
- def test_get_target_ref(self):
- # Check remote or remote branch not present.
+
self.assertEqual(None, git_cl.GetTargetRef('origin', None, 'master'))
self.assertEqual(None, git_cl.GetTargetRef(None,
'refs/remotes/origin/master',
@@ -2766,8 +2768,8 @@
def test_git_cl_comments_fetch_gerrit(self):
self.mock(sys, 'stdout', StringIO.StringIO())
self.calls = [
- ((['git', 'symbolic-ref', 'HEAD'],), CERR1),
- ((['git', 'symbolic-ref', 'HEAD'],), CERR1),
+ ((['git', 'config', 'branch.foo.gerritserver'],), ''),
+ ((['git', 'config', 'branch.foo.merge'],), ''),
((['git', 'config', 'rietveld.upstream-branch'],), CERR1),
((['git', 'branch', '-r'],), 'origin/HEAD -> origin/master\n'
'origin/master'),
@@ -2775,8 +2777,18 @@
'https://chromium.googlesource.com/infra/infra'),
(('GetChangeDetail', 'chromium-review.googlesource.com',
'infra%2Finfra~1',
- ['MESSAGES', 'DETAILED_ACCOUNTS']), {
+ ['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
+ 'CURRENT_COMMIT']), {
'owner': {'email': 'owner@example.com'},
+ 'current_revision': 'ba5eba11',
+ 'revisions': {
+ 'deadbeaf': {
+ '_number': 1,
+ },
+ 'ba5eba11': {
+ '_number': 2,
+ },
+ },
'messages': [
{
u'_revision_number': 1,
@@ -2835,7 +2847,10 @@
'message': 'I removed this because it is bad',
},
]
- })
+ }),
+ (('GetChangeRobotComments', 'chromium-review.googlesource.com',
+ 'infra%2Finfra~1'), {}),
+ ((['git', 'config', 'branch.foo.gerritpatchset', '2'],), ''),
] * 2 + [
(('write_json', 'output.json', [
{
@@ -2847,6 +2862,7 @@
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'autogenerated': False,
u'approval': False,
u'disapproval': False,
u'sender': u'owner@example.com'
@@ -2859,6 +2875,7 @@
u' PS2, File comment: https://chromium-review.googlesource' +
u'.com/c/1/2//COMMIT_MSG#\n' +
u' Please include a bug link\n'),
+ u'autogenerated': False,
u'approval': False,
u'disapproval': False,
u'sender': u'reviewer@example.com'
@@ -2875,6 +2892,7 @@
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),
+ autogenerated=False,
disapproval=False, approval=False, sender=u'owner@example.com'),
git_cl._CommentSummary(
message=(
@@ -2885,12 +2903,127 @@
u'c/1/2//COMMIT_MSG#\n' +
u' Please include a bug link\n'),
date=datetime.datetime(2017, 3, 17, 5, 19, 37, 500000),
+ autogenerated=False,
disapproval=False, approval=False, sender=u'reviewer@example.com'),
]
- cl = git_cl.Changelist(codereview='gerrit', issue=1)
+ cl = git_cl.Changelist(
+ codereview='gerrit', issue=1, branchref='refs/heads/foo')
self.assertEqual(cl.GetCommentsSummary(), expected_comments_summary)
- self.assertEqual(0, git_cl.main(['comment', '-i', '1',
- '-j', 'output.json']))
+ self.mock(git_cl.Changelist, 'GetBranch', lambda _: 'foo')
+ self.assertEqual(
+ 0, git_cl.main(['comments', '-i', '1', '-j', 'output.json']))
+
+ def test_git_cl_comments_robot_comments(self):
+ # git cl comments also fetches robot comments (which are considered a type
+ # of autogenerated comment), and unlike other types of comments, only robot
+ # comments from the latest patchset are shown.
+ self.mock(sys, 'stdout', StringIO.StringIO())
+ self.calls = [
+ ((['git', 'config', 'branch.foo.gerritserver'],), ''),
+ ((['git', 'config', 'branch.foo.merge'],), ''),
+ ((['git', 'config', 'rietveld.upstream-branch'],), CERR1),
+ ((['git', 'branch', '-r'],), 'origin/HEAD -> origin/master\n'
+ 'origin/master'),
+ ((['git', 'config', 'remote.origin.url'],),
+ 'https://chromium.googlesource.com/infra/infra'),
+ (('GetChangeDetail', 'chromium-review.googlesource.com',
+ 'infra%2Finfra~1',
+ ['MESSAGES', 'DETAILED_ACCOUNTS', 'CURRENT_REVISION',
+ 'CURRENT_COMMIT']), {
+ 'owner': {'email': 'owner@example.com'},
+ 'current_revision': 'ba5eba11',
+ 'revisions': {
+ 'deadbeaf': {
+ '_number': 1,
+ },
+ 'ba5eba11': {
+ '_number': 2,
+ },
+ },
+ 'messages': [
+ {
+ u'_revision_number': 1,
+ u'author': {
+ u'_account_id': 1111084,
+ u'email': u'commit-bot@chromium.org',
+ u'name': u'Commit Bot'
+ },
+ u'date': u'2017-03-15 20:08:45.000000000',
+ u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046dc50b',
+ u'message': u'Patch Set 1:\n\nDry run: CQ is trying the patch...',
+ u'tag': u'autogenerated:cq:dry-run'
+ },
+ {
+ u'_revision_number': 1,
+ u'author': {
+ u'_account_id': 123,
+ u'email': u'tricium@serviceaccount.com',
+ u'name': u'Tricium'
+ },
+ u'date': u'2017-03-16 20:00:41.000000000',
+ u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d1234',
+ u'message': u'(1 comment)',
+ u'tag': u'autogenerated:tricium',
+ },
+ {
+ u'_revision_number': 1,
+ u'author': {
+ u'_account_id': 123,
+ u'email': u'tricium@serviceaccount.com',
+ u'name': u'Tricium'
+ },
+ u'date': u'2017-03-16 20:00:41.000000000',
+ u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d1234',
+ u'message': u'(1 comment)',
+ u'tag': u'autogenerated:tricium',
+ },
+ {
+ u'_revision_number': 2,
+ u'author': {
+ u'_account_id': 123 ,
+ u'email': u'tricium@serviceaccount.com',
+ u'name': u'reviewer'
+ },
+ u'date': u'2017-03-17 05:30:37.000000000',
+ u'tag': u'autogenerated:tricium',
+ u'id': u'f5a6c25ecbd3b3b54a43ae418ed97eff046d4568',
+ u'message': u'(1 comment)',
+ },
+ ]
+ }),
+ (('GetChangeComments', 'chromium-review.googlesource.com',
+ 'infra%2Finfra~1'), {}),
+ (('GetChangeRobotComments', 'chromium-review.googlesource.com',
+ 'infra%2Finfra~1'), {
+ 'codereview.settings': [
+ {
+ u'author': {u'email': u'tricium@serviceaccount.com'},
+ u'updated': u'2017-03-17 05:30:37.000000000',
+ u'robot_run_id': u'5565031076855808',
+ u'robot_id': u'Linter/Category',
+ u'tag': u'autogenerated:tricium',
+ u'patch_set': 2,
+ u'side': u'REVISION',
+ u'message': u'Linter warning message text',
+ u'line': 32,
+ },
+ ],
+ }),
+ ((['git', 'config', 'branch.foo.gerritpatchset', '2'],), ''),
+ ]
+ expected_comments_summary = [
+ git_cl._CommentSummary(date=datetime.datetime(2017, 3, 17, 5, 30, 37),
+ message=(
+ u'(1 comment)\n\ncodereview.settings\n'
+ u' PS2, Line 32: https://chromium-review.googlesource.com/'
+ u'c/1/2/codereview.settings#32\n'
+ u' Linter warning message text\n'),
+ sender=u'tricium@serviceaccount.com',
+ autogenerated=True, approval=False, disapproval=False)
+ ]
+ cl = git_cl.Changelist(
+ codereview='gerrit', issue=1, branchref='refs/heads/foo')
+ self.assertEqual(cl.GetCommentsSummary(), expected_comments_summary)
def test_get_remote_url_with_mirror(self):
original_os_path_isdir = os.path.isdir
