unofficial mirror of libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Simon Marchi <simon.marchi@polymtl.ca>
To: libc-alpha <libc-alpha@sourceware.org>,
	"gdb-patches@sourceware.org" <gdb-patches@sourceware.org>
Subject: Gerrit update - diff in comment notification emails
Date: Fri, 8 Nov 2019 12:16:26 -0500	[thread overview]
Message-ID: <326a619f-852b-5356-1e32-4dc4c6f833c1@polymtl.ca> (raw)

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

             reply	other threads:[~2019-11-08 17:16 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-08 17:16 Simon Marchi [this message]
2019-11-09  0:03 ` Gerrit update - diff in comment notification emails Simon Marchi
2019-11-09 13:22   ` Carlos O'Donell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/libc/involved.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=326a619f-852b-5356-1e32-4dc4c6f833c1@polymtl.ca \
    --to=simon.marchi@polymtl.ca \
    --cc=gdb-patches@sourceware.org \
    --cc=libc-alpha@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).