Issue metadata
Sign in to add a comment
|
'git cl comments' doesn't give the reviewer's comments |
||||||||||||||||||||||
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.
,
May 31 2017
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.
,
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?
,
Jun 1 2017
Can your editor extract that information from the url, which uses a # instead of a : to represent the exact same information?
,
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?
,
Jun 1 2017
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
,
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. ¯\_(ツ)_/¯
,
Jun 1 2017
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.
,
Jun 5 2017
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
,
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
,
Jun 5 2017
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by aga...@chromium.org
, May 26 2017Status: Assigned (was: Unconfirmed)