Re: Gerrit

[Simon Marchi <simon.marchi@polymtl.ca>]

On 2019-10-14 1:55 p.m., Eli Zaretskii wrote:
>> The content of the emails is fully configurable. We can work on 
>> improving them if you have specific pain points or suggestions.
> 
> It is too wordy in some parts, and in others don't say enough.  For
> example, a part like this:
> 
>> Tom de Vries has posted comments on this change. ( https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/28 )
>>
>> Change subject: [gdb] Only force INTERP_CONSOLE ui_out for breakpoint commands in MI mode
> 
> is completely redundant (and long lines make it look ugly).

Ok, I have removed the "Change subject" line in many of the email templates.  From what I
understand, it is indeed redundant with the email subject.  I also removed some not so
important text to be more straight to the point.

I have put the URL on a separate line to avoid having this too long line.

Here's the result:

> The epilogue, viz.:
> 
>> Gerrit-Project: binutils-gdb
>> Gerrit-Branch: master
>> Gerrit-Change-Id: Id1771e7fcc9496a7d97ec2b2ea6b1487596f1ef7
>> Gerrit-Change-Number: 28
>> Gerrit-PatchSet: 1
>> Gerrit-Owner: Tom de Vries <tdevries@suse.de>
>> Gerrit-Reviewer: Andrew Burgess <andrew.burgess@embecosm.com>
>> Gerrit-Reviewer: Tom de Vries <tdevries@suse.de>
>> Gerrit-Comment-Date: Mon, 14 Oct 2019 15:45:58 +0000
>> Gerrit-HasComments: No
>> Gerrit-Has-Labels: No
>> Gerrit-MessageType: comment
> 
> seems also mostly useless.

I guess these can be used by people to write email filters or scripts.  But we also find almost
the same information as headers in the email:

...
X-Gerrit-Change-Id: I806ef0851c43ead90b545a11794e41f5e5178436
X-Gerrit-Change-Number: 25
X-Gerrit-ChangeURL: <https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/25>
...

So I removed them.

So here's the result for a notification of type "comment" now:

https://sourceware.org/ml/gdb-patches/2019-10/msg00388.html

> OTOH, a message such as this:
> 
>> Patch Set 2:
>>
>> This change is ready for review.
> 
> doesn't show the patch set at all, so it's also unhelpful.

Indeed, this looks like (I can't find it) a notification of type "comment",
sent when somebody writes a comment about a patch set.  This type doesn't
include the full diff of the patch, and I don't think it would help to do so.

In that example, Tom has replied to the patch set 2 (i.e. version 2) of
that change.  Normally, you'd have received the diff in a previous email
in the same thread, the one where he uploaded this patch set 2.  Here, it's
possible you don't have it because we didn't have the notifications yet when
he did.

When somebody leaves a comment attached to a given line of the patch
(which you do through the web UI), the notification will include that
line above the comment.  I'd like if it was possible to include a slightly
larger context, say the 5 lines above that line too, like one would typically
do if replying by email.  But this is a bit more advanced, I don't know if
it's possible yet.

>>> Anyway, seeing the beginning of a patch was the only way for me to
>>> know that a patch needs me to review the documentation parts.  Now I
>>> wonder how I can do that when the patch is posted on Gerrit.
>>
>> What do you mean by "beginning of a patch", do you mean the diff stat 
>> that shows the changed files? If so, I believe this information is 
>> available in the notification sent for a new change.
> 
> Not only the difstat part, but also the ChangeLog part.
> 
>>    M gdb/block.c
>>    M gdb/testsuite/gdb.dwarf2/varval.exp
>>    2 files changed, 55 insertions(+), 3 deletions(-)
>>
>> Does that help?
> 
> I'd prefer to see the whole commit log message with the rationale
> etc., it sometimes touches subjects where I'd like to voice an
> opinion, even if that's not only about documentation.

The commit log is shown in the notifications sent when a user uploads a new patch
or a new version of a patch.  If they have put the ChangeLog entry in the commit log,
you should therefore see it there.  From this point of view, this is exactly like
regular email patches.

Simon