Re: Setup non-pushing gerrit instance for glibc.

[Sergio Durigan Junior <sergiodj at redhat dot com>]

On Thursday, October 24 2019, Carlos O'Donell wrote:

> On 10/24/19 5:41 PM, Joseph Myers wrote:
>> On Thu, 24 Oct 2019, Carlos O'Donell wrote:
>> 
>>> * Try to setup email based code review in gerrit.
>>>   - Currently email is outbound only.
>> 
>> Observations on teething troubles with the initial setup:
>> 
>> 1. What's the status of fixing the problem with insufficient diff context 
>> in emails when comments relate to particular parts of the diff?  They need 
>> to quote the relevant amount of context (typically at least the whole diff 
>> hunk, including the hunk header showing the changed function name) rather 
>> than just one line and a reference to an external URL.  It's important for 
>> messages with reviews to be meaningful in themselves without depending on 
>> external links.  This is a longstanding problem (it was obvious in some 
>> experiments some people did with proposing GCC patches with Rietveld, 
>> gerrit's predecessor, several years ago).  Someone in the GDB discussions 
>> mentioned a prototype patch to add some context to the emails, so maybe we 
>> could use that patch.
>
> I don't have any status on this change.
>
> Sergio, Were you working on a patch like this to expand the diff context?
>
> If I understand the problem correctly it's that:
> ~~~
> https://gnutoolchain-gerrit.osci.io/r/c/glibc/+/303/1//COMMIT_MSG@19 
> PS1, Line 19: Signed-off-by: Carlos O'Donell <carlos@redhat.com>
> We do not use DCO and Signed-off-by.
> ~~~
> Is insufficient context. It shouldn't just be line 19, it should be the
> whole hunk with the comment on the line it belongs to.

Simon was working on this, and he told me he has a prototype fix that he
was planning to deploy on our instance.  I don't know the current
status, though.  I added him to the Cc list.

BTW, I agree this is a problem that should be fixed; quoting just one
line from the change is really not sufficient.

>> 2. Could text comments in emails from gerrit be properly wrapped?  
>> Messages such as 
>> <https://sourceware.org/ml/libc-alpha/2019-10/msg00715.html> are hard to 
>> read in the list archives because of very long lines.  (Of course, diff 
>> context / quoted source lines should not be wrapped.)
>
> I agree that should be fixed.
>
> I also find it hard to read.

The users can configure line wrapping under Settings -> Edit
Preferences.  This is a per-user configuration; gerrit doesn't have a
global setting that forces line wrapping.  However, I think it's
possible to edit the All-Users repository and make this setting
automatically enabled for everybody.

/me does some tests...

Hm, it seems the line wrapping setting doesn't really apply to messages?
I tried writing a comment without line wrapping here:

  https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/161/3#message-9f0b67bbd0ef0765d07f417a15f50aa79a5414d5

And it generated this message:

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

I mean, gnus wraps the line for me, but I understand this is my MUA's
setting, and not something we can really expect.

>> 5. It would also be useful to have documentation for how someone should 
>> make a patch series appear appropriately in gerrit if they want to propose 
>> a series that way.  That means the emails for a patch series should 
>> include 1/N, 2/N etc. in their subjects (with a 0/N cover letter as 
>> appropriate).
>
> Agreed, that will be a next step. I'm trying to get through just one patch
> review right now to see how that works.

Bear in mind that gerrit still doesn't support cover letters.  We've had
a discussion about this on gdb-patches, and the outcome is:

1) The gerrit bug requesting support for cover letters has been reopened
(it had been closed due to inactivity).  You can see the bug here:

  https://bugs.chromium.org/p/gerrit/issues/detail?id=924

There is a discussion happening there, but unfortunately I am unable to
participate because their bug tracker demands a Google account, which I
don't have.

2) It is possible, although very hacky, to create a cover letter using a
git empty commit as the first commit.

3) It is possible to continue using the mailing list for that: you can
send the change via gerrit, which will send the email notifications to
the ml.  Then, you can send an email in reply to one of gerrit's
messages explaning what the series is about.  This is also not very
nice.

You can see the gdb-patches discussion here:

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

Even though gerrit has the concept of a patch series, when it sends the
email notifications it doesn't thread them, and it also doesn't include
the "X/N" numbering scheme, so it's not trivial to identify series in
the mailing list (which can also be seen as a drawback).

>> 6. Lower priority, but note there are certain kinds of changes involving 
>> huge diffs (e.g. to generated files) that thus *would* need a message size 
>> limit and pointing to a URL for the diffs in that case, for it to be 
>> possible to handle such changes through gerrit.  (When sending email 
>> manually for such a change one can always omit the 50 MB of diffs to 
>> generated files, but as I understand it part of the point of such a patch 
>> review system is that the system seems the exact change proposed to be 
>> committed, complete with generated files, so enabling automated testing of 
>> patch proposals if desired.)
>
> Correct, you'd need the full commit of 50MiB, but I don't know what happens
> in that case with gerrit. I could try something that is MiB's in size, like
> removing all the locale data.

That'd be interesting to see.

> Thanks for your feedback and list of things you'd like to see, it will take
> time to sort through them.

Indeed: thank you for the feedback, and it will take some time to sort
through them, for sure :-).

Thanks,

-- 
Sergio
GPG key ID: 237A 54B1 0287 28BF 00EF  31F4 D0EB 7628 65FC 5E36
Please send encrypted e-mail if possible
http://sergiodj.net/