New issue
Advanced search Search tips

Issue 726514 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: ----
Type: ----



Sign in to add a comment

'git cl comments' doesn't give the reviewer's comments

Project Member Reported by piman@chromium.org, May 25 2017

Issue description

'git cl comments' shows the top-level Patch Set comments, but not the individual line-by-line reviewer comments. The latter is really the useful part! This is a regression compared to rietveld.

Example:

| $ git cl issue
| Issue number: 516422 (https://chromium-review.googlesource.com/516422)
| $ git cl comments
| 
| 2017-05-25 21:31:03 UTC   piman@chromium.org
|   Uploaded patch set 1.
|   Initial upload
| 
| 2017-05-25 21:31:11 UTC   piman@chromium.org
|   Patch Set 2: Commit message was updated.
| 
| 2017-05-25 21:35:01 UTC   piman@chromium.org
|   Patch Set 2: Commit-Queue+1
|   
|   (2 comments)
| 
| 2017-05-25 21:35:14 UTC   commit-bot@chromium.org
|   Patch Set 2:
|   
|   Dry run: CQ is trying da patch.
|   
|   Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/516422/2
|   
|   Bot data: {"action": "start", "triggered_at": "2017-05-25T21:35:01.0Z", "revision": "df87a54b7899c39157d89fbadeeaaacdb9d1827a"}
| 
| 2017-05-25 22:04:40 UTC   piman@chromium.org
|   Patch Set 2: -Commit-Queue
| 
| 2017-05-25 22:04:41 UTC   commit-bot@chromium.org
|   Patch Set 2:
|   
|   Dry run: Try jobs failed on following builders:
|     android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_swarming_rel/builds/186961)
|   
|   Bot data: {"action": "cancel", "triggered_at": "2017-05-25T21:35:01.0Z", "revision": "df87a54b7899c39157d89fbadeeaaacdb9d1827a"}
| 
| 2017-05-25 22:31:25 UTC   zmo@chromium.org
|   Patch Set 2: Code-Review+1
|   
|   (5 comments)
|   
|   LGTM with a few nits.
|   
|   Sounds good to rename the filename in this CL.


What is missing is:

| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_buffer_service_unittest.cc
| File gpu/command_buffer/service/command_buffer_service_unittest.cc:
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_buffer_service_unittest.cc@a1
| PS2, Line 1:
| > Almost all these tests are redundant with the CommandExecutorTests. I  
| > added
| Ack
| 
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_executor_unittest.cc
| File gpu/command_buffer/service/command_executor_unittest.cc:
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_executor_unittest.cc@62
| PS2, Line 62: command_buffer_
| nit: you need to rename it to command_buffer_service_ to comply to the  
| naming convention of the getter function.
| 
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_executor_unittest.cc@458
| PS2, Line 458: EXPECT_EQ
| EXPECT_EQ(false, ...) seems to fail on Android build.  Should we use  
| EXPECT_TRUE and EXPECT_FALSE for these?
| 
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/command_buffer/service/command_executor_unittest.cc@469
| PS2, Line 469: 2
| nit: can me make 2 a enum and use it in Unschedule? Easier to read.
| 
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/ipc/in_process_command_buffer.cc
| File gpu/ipc/in_process_command_buffer.cc:
| 
| https://chromium-review.googlesource.com/#/c/516422/2/gpu/ipc/in_process_command_buffer.cc@313
| PS2, Line 313: command_buffer_
| Now we merge the two classes, we could actually merge  
| SetParserErrorCallBack into the constructor, could we?


The above came in e-mail. Note however that rietveld displays comments with a filename:lineno header (i.e. similar to gcc error output), and that is critical for editor/tool support.
 

Comment 1 by aga...@chromium.org, May 26 2017

Owner: aga...@chromium.org
Status: Assigned (was: Unconfirmed)
Good catch! Thanks, I'll definitely bring file- and line-level comments into the gerrit version of git-cl-comments.

Comment 2 by aga...@chromium.org, May 31 2017

Status: Started (was: Assigned)
I have a CL to fix this here: https://chromium-review.googlesource.com/c/520722/

The output is meant to mimic the formatting of Gerrit messages and emails, while providing helpful links and patchset/line numbers. On my test CL (https://chromium-review.googlesource.com/c/518987) looks like this:

± git cl comments --gerrit --issue 518987

2017-05-31 18:59:53 UTC   agable@chromium.org
  Patch Set 1:
  
  (1 comment)
  
  WATCHLISTS
    Base, Line 10: https://chromium-review.googlesource.com/c/518987/1/WATCHLISTS#b10
    This is a file comment that I save and publish

2017-05-31 19:01:29 UTC   agable@chromium.org
  Patch Set 2:
  
  (2 comments)
  
  WATCHLISTS
    Base, Line 14: https://chromium-review.googlesource.com/c/518987/1/WATCHLISTS#b14
    This is a comment that I save but don't publish
  
    Base, Line 22: https://chromium-review.googlesource.com/c/518987/1/WATCHLISTS#b22
    This is a comment that I won't save until after I upload a new patchset

2017-05-31 21:56:04 UTC   agable@chromium.org
  Patch Set 2:
  
  (1 comment)
  
  codereview.settings
    PS2, File comment: https://chromium-review.googlesource.com/c/518987/2/codereview.settings#
    This is a file comment on the right hand side in PS2

2017-05-31 22:12:25 UTC   agable@chromium.org
  Patch Set 2:
  
  (2 comments)
  
  WATCHLISTS
    Base, File comment: https://chromium-review.googlesource.com/c/518987/1/WATCHLISTS#b
    left hand file comment on Base while viewing PS1 but PS2 is uploaded
  
    PS1, File comment: https://chromium-review.googlesource.com/c/518987/1/WATCHLISTS#
    right hand file comment on PS1 while PS2 is uploaded

2017-05-31 22:43:00 UTC   agable@chromium.org
  Patch Set 2:
  
  (3 comments)
  
  /COMMIT_MSG
    PS2, Line 7: https://chromium-review.googlesource.com/c/518987/2//COMMIT_MSG#7
    deletion counts!
  
  WATCHLISTS
    Base, Line 15: https://chromium-review.googlesource.com/c/518987/2/WATCHLISTS#b15
    yeah whatever
  
    PS2, File comment: https://chromium-review.googlesource.com/c/518987/2/WATCHLISTS#
    something

2017-05-31 22:43:59 UTC   agable@chromium.org
  Patch Set 2:
  
  (2 comments)
  
  This one has a top-level comment too since I've totally failed to test that so far.
  
  WATCHLISTS
    Base, Line 2: https://chromium-review.googlesource.com/c/518987/2/WATCHLISTS#b2
    heh
  
  codereview.settings
    Base, Line 3: https://chromium-review.googlesource.com/c/518987/2/codereview.settings#b3
    Lorem ipsum dolor sit amet, consectetur adipiscing elit. Proin eu rutrum dolor. In dignissim metus a volutpat egestas. Maecenas at erat sit amet libero pulvinar faucibus. Aenean pharetra pharetra mauris eget aliquam. Sed commodo purus massa, eget finibus est vehicula fringilla. In suscipit pharetra viverra. Mauris pharetra mollis varius. Class aptent taciti sociosqu ad litora torquent per conubia nostra, per inceptos himenaeos. Donec gravida ullamcorper consequat. Quisque lacinia vel eros ut posuere. Nullam varius nibh id nulla vehicula porta hendrerit a dolor. Aenean volutpat blandit quam eget efficitur. Fusce bibendum euismod leo sit amet viverra. Praesent semper eros in ex consectetur, vel dapibus nisl volutpat. Vivamus fringilla eu dui non ultrices.

Comment 3 by piman@chromium.org, Jun 1 2017

@#2: the rietveld version of 'git cl comments' contains in the output lines like 'path/to/file.cc:123' which parse like compiler warnings/errors (or 'git gs'/'git grep'), and allows editors to quick-jump to the line in the source that the comment references. Is it possible to restore that feature for the gerrit version?
Can your editor extract that information from the url, which uses a # instead of a : to represent the exact same information?

Comment 5 by piman@chromium.org, Jun 1 2017

I would have to add a parser/converter script. Trim the URL to extract the file path, and extract the line number, and recombine? It's not rocket science, but  everybody else who wants the same feature would have to do it too - seems like a waste.

IIRC file:line was explicitly added to the rietveld version to enable editor/tooling support. Any particular reason why we would want to remove the feature on the gerrit version?
Oh, that's surprising to me. It exists in Rietveld because that's the form that all of the email messages took -- "git cl comments" for rietveld does literally nothing except print the email bodies in their raw form.

I would like to remove the gerrit version for three reasons:
1) It makes the output less readable by humans, especially since it is so close to the url which contains the same information
2) Machines can use the machine-readable version, the json from requesting the /changes/<number>/comments endpoint
3) It isn't strictly correct -- the file/line combination from the comment is the file/line from that patchset, which does not necessarily match the file/line in your current checkout

Comment 7 by piman@chromium.org, Jun 1 2017

If you look at rietveld comments, they contain both the url and the file:line, so that it can be used for both purposes. There's no reason for them to include the (redundant) file:line otherwise. Removing it makes the tool less useful. ¯\_(ツ)_/¯
Yes, I'm aware that Rietveld contains both, and it's ugly and hard for humans to read :)

But ok, I've uploaded a version of the CL which includes both.
Latest patchset has '-m' flag, which switches output from:

± git cl comments

2017-05-31 23:18:47 UTC   agable@chromium.org
  Patch Set 1:
  
  PTAL!

2017-06-01 18:24:18 UTC   tandrii@chromium.org
  Patch Set 1: Code-Review+1
  
  (2 comments)
  
  Quite nice, thanks! Please add line comment to test.
  
  git_cl.py
    PS1, Line 2608: https://chromium-review.googlesource.com/c/520722/1/git_cl.py#2608
    nit: consider adding human readable patchset name



To



± git cl comments -m

2017-05-31 23:18:47 UTC   agable@chromium.org
  Patch Set 1:
  
  PTAL!

2017-06-01 18:24:18 UTC   tandrii@chromium.org
  Patch Set 1: Code-Review+1
  
  (2 comments)
  
  Quite nice, thanks! Please add line comment to test.
  
    git_cl.py:2608:
    nit: consider adding human readable patchset name

Project Member

Comment 10 by bugdroid1@chromium.org, Jun 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/tools/depot_tools/+/0ffdf2de157ed4252983254913f8c0b5e231d99c

commit 0ffdf2de157ed4252983254913f8c0b5e231d99c
Author: Aaron Gable <agable@chromium.org>
Date: Mon Jun 05 20:30:56 2017

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>

[modify] https://crrev.com/0ffdf2de157ed4252983254913f8c0b5e231d99c/tests/git_cl_test.py
[modify] https://crrev.com/0ffdf2de157ed4252983254913f8c0b5e231d99c/git_cl.py
[modify] https://crrev.com/0ffdf2de157ed4252983254913f8c0b5e231d99c/gerrit_util.py

Status: Fixed (was: Started)

Sign in to add a comment