Gerrit update - diff in comment notification emails

Gerrit update - diff in comment notification emails

[Simon Marchi]

Hi!

In the last few days, I have tackled the problem of making the Gerrit
comment notification emails show comments in a diff context.

Previously, we would show some context, but that context would only
include lines from the version of the file that was commented on.  This
means that if the commenter had put a comment on the left pane in the
web UI, the comment would be placed in a context containing lines from
the "before" version of the file.  If the commenter had put a comment on
the right pane, the context would show lines from the "after" version of
the file.

This was already an improvement compared to Gerrit's normal behavior of
showing just one line of context (or the multiple lines of the range,
if the comment was placed on a range), but it still was lacking the
information to understand what was actually changed.

The new format shows a portion of the diff around each comment,
resulting in something like this:

  https://sourceware.org/ml/gdb-patches/2019-11/msg00239.html

I made the output look as much as possible like "git diff" output to
make people feel at home, but we are not limited to that.  I hope that
this is a welcome change.  If you have some ideas on how to improve it
further, I would be happy to hear your suggestions.  I am sure we will
hit some corner cases for which the script doesn't give a good output,
so please report any problem you see.

This is all implemented using a Python script [1] that uses Gerrit's
REST API, so the idea is that if you'd like to propose something, you
can actually try it out yourself and propose a code change.  You can run
the script on any batch of already published comments to see what the
result would be.

If the generated email doesn't contain any of the comments on the code,
such as this:

  https://sourceware.org/ml/gdb-patches/2019-11/msg00238.html

it's probably because the script failed.  It's then possible to just
run it locally, point it to the change, and see what happens.

Note that while this shows the comments in the context of the diffs, it
doesn't show the comment you are replying to, if you were commenting in
reply to another comment.  So if we just reply "Done" in the web UI, we
will only see "Done" in the notification email, which is not very
enlightening.  So please try to use "Quote" instead of "Reply" and quote
the relevant portion of what you are replying to (just like you would by
email), so that it appears in the notification email.

Simon

[1] https://gnutoolchain-gerrit.osci.io/r/gitweb?p=gerrit.git;a=blob;f=resources/com/google/gerrit/pgm/init/generate-comment-diff.py;hb=refs/heads/stable-3.0-gnu

Re: Gerrit update - diff in comment notification emails

[Simon Marchi]

On 2019-11-08 12:16 p.m., Simon Marchi wrote:
> Hi!
> 
> In the last few days, I have tackled the problem of making the Gerrit
> comment notification emails show comments in a diff context.
> 
> Previously, we would show some context, but that context would only
> include lines from the version of the file that was commented on.  This
> means that if the commenter had put a comment on the left pane in the
> web UI, the comment would be placed in a context containing lines from
> the "before" version of the file.  If the commenter had put a comment on
> the right pane, the context would show lines from the "after" version of
> the file.
> 
> This was already an improvement compared to Gerrit's normal behavior of
> showing just one line of context (or the multiple lines of the range,
> if the comment was placed on a range), but it still was lacking the
> information to understand what was actually changed.
> 
> The new format shows a portion of the diff around each comment,
> resulting in something like this:
> 
>   https://sourceware.org/ml/gdb-patches/2019-11/msg00239.html
> 
> I made the output look as much as possible like "git diff" output to
> make people feel at home, but we are not limited to that.  I hope that
> this is a welcome change.  If you have some ideas on how to improve it
> further, I would be happy to hear your suggestions.  I am sure we will
> hit some corner cases for which the script doesn't give a good output,
> so please report any problem you see.
> 
> This is all implemented using a Python script [1] that uses Gerrit's
> REST API, so the idea is that if you'd like to propose something, you
> can actually try it out yourself and propose a code change.  You can run
> the script on any batch of already published comments to see what the
> result would be.
> 
> If the generated email doesn't contain any of the comments on the code,
> such as this:
> 
>   https://sourceware.org/ml/gdb-patches/2019-11/msg00238.html
> 
> it's probably because the script failed.  It's then possible to just
> run it locally, point it to the change, and see what happens.
> 
> Note that while this shows the comments in the context of the diffs, it
> doesn't show the comment you are replying to, if you were commenting in
> reply to another comment.  So if we just reply "Done" in the web UI, we
> will only see "Done" in the notification email, which is not very
> enlightening.  So please try to use "Quote" instead of "Reply" and quote
> the relevant portion of what you are replying to (just like you would by
> email), so that it appears in the notification email.
> 
> Simon
> 
> [1] https://gnutoolchain-gerrit.osci.io/r/gitweb?p=gerrit.git;a=blob;f=resources/com/google/gerrit/pgm/init/generate-comment-diff.py;hb=refs/heads/stable-3.0-gnu

I was told that this link was not accessible.  It was a permission problem
with Gitweb, it should be fixed now.

Sorry for the trouble.

Simon

Re: Gerrit update - diff in comment notification emails

[Carlos O'Donell]

On 11/8/19 7:03 PM, Simon Marchi wrote:
> On 2019-11-08 12:16 p.m., Simon Marchi wrote:
>> Note that while this shows the comments in the context of the diffs, it
>> doesn't show the comment you are replying to, if you were commenting in
>> reply to another comment.  So if we just reply "Done" in the web UI, we
>> will only see "Done" in the notification email, which is not very
>> enlightening.  So please try to use "Quote" instead of "Reply" and quote
>> the relevant portion of what you are replying to (just like you would by
>> email), so that it appears in the notification email.

Great work Simon!

Clearly if you're just responding with "Done" without any quote it's like
hitting reply to an email without quoting it and saying "Done."

There are habits we will all have to learn about Gerrit, like not just
hitting "+2" in the top right. Just hit reply and give your +2 so you don't
immediately send out email. This kind of quirk is interesting. I thought it
was bad and then I realized that the +2 not being staged is a "quick" way
to just review dozens of typo patches and get them approved without hitting
reply, and +2, and send.

So I think there *is* method in what appears to be arbitrary UI elements,
but are actually, like vim or emaacs, well tuned interfaces to their particular
uses.

Thank you again for working through the email formatting issues!

I hope we get these accepted upstream :-)


>> Simon
>>
>> [1] https://gnutoolchain-gerrit.osci.io/r/gitweb?p=gerrit.git;a=blob;f=resources/com/google/gerrit/pgm/init/generate-comment-diff.py;hb=refs/heads/stable-3.0-gnu
> 
> I was told that this link was not accessible.  It was a permission problem
> with Gitweb, it should be fixed now.
> 
> Sorry for the trouble.
> 
> Simon
> 


-- 
Cheers,
Carlos.