Re: some thoughts on gerrit

[Simon Marchi <simark at simark dot ca>]

On 2019-10-20 10:37 a.m., Tom Tromey wrote:
> After using gerrit for gdb for just this past week, I have mixed
> feelings, which I thought I'd share.
> 
> The upside is pretty good -- basically what I was hoping for when we
> discussed this topic at Cauldron.
> 
> The major benefits is that it's easy to see the status of patches.
> 
> A benefit I didn't predict is that it's a bit simpler to submit patches.
> In particular, my personal email host doesn't like it if I send log
> series, so I have to remember to throttle when using send-email -- but
> with gerrit that's a non-issue, because it is just a push.

As a tangeant to this, what I like about Gerrit is that compared to emails,
it's not really possible to create a Gerrit change in a wrong way.  Whereas
I often encounter patches that don't apply and have to fight a bit with them,
sometimes applying them by hand, or give up and tell the author to send it
again with git-send-email.  Note that this mostly happens with one-off
contributors who are not really used to the email workflow.  After somebody
has sent their first few patches, they generally get it right.  But it's
time consuming to do this education, and sometimes demotivates me from doing
reviews.

Maybe I could just be more strict and say that I won't look at the patch if
it doesn't apply right away.

> So far the major downsides are related to patch series.
> 
> [[[
> First, as an aside, my most recent patch series does show up here:
> 
> https://sourceware.org/ml/gdb-patches/2019-10/authors.html
> 
> (Search for "RAII class" under the name "Tom Tromey (Code Review)")
> 
> ...but it somehow doesn't show up here:
> 
> https://sourceware.org/ml/gdb-patches/2019-10/threads.html
> ]]]
> 
> 
> Anyway, with series we are missing two things that email had: namely,
> the cover letter, and threading.
> 
> The cover letter is often a good guide to what is going on in the
> series.  See for example:
> 
> https://sourceware.org/ml/gdb-patches/2019-10/msg00590.html
> 
> It seems a shame to lose this.
> 
> One counter-argument about the cover letter I thought of is that,
> because it doesn't end up in the history, maybe the lack of it will
> force us to make each commit message even clearer.  And, we should do
> that ... it's just that the roadmap is handy when reviewing.
> 
> I wonder if there'd be a way to make "git review" prompt for a cover
> letter and attach it somewhere as a comment.
> 
> 
> Lack of threading means it is hard to see how patches relate when
> reading in email.  Maybe this can be fixed in gerrit?

Hmm, I knew that patch series weren't really well handled in Gerrit, but
now that you mention it, I think it would be quite a regression in terms of
workflow, given how much we rely on (sometimes quite large) patch series.

The cover letter is indeed very important for reviewers, to understand how
to approach the series as a whole.  Putting the equivalent of the cover
letter as a comment on one of the patches would make it hard to find.
Sergio's suggestion of having an empty commit works, but is a bit cumbersome.

We can still use the mailing list for that though.  In lieu of a cover letter,
you can send an email to gdb-patches saying "I've posted a series that does X",
post a link to Gerrit (perhaps with the link to the topic, like [1] (I've added
the minsyms-threads topic to your patches)) and say what you have to say about
the series there.

In the web UI, I think Gerrit shows patches in the same chain pretty well:

    https://nova.polymtl.ca/~simark/ss/ad123r4v.png

[1] https://gnutoolchain-gerrit.osci.io/r/q/topic:%22minsyms-threads%22+(status:open%20OR%20status:merged)

I also don't see how to make emails for the multiple patches of a series
threaded together, given that Gerrit doesn't have the concept of series (it
just knows that they are chained).  We've been trying to tweak the emails
Gerrit sends to make them closer to what we are used to now, but there is a
limit to what we can do.  If we choose to continue with Gerrit, we'll have
to accept that while it's possible to have some email notifications, it's
still primarily designed to be used from the web-UI.  The email experience
will never be as good as what we have today.

> I wonder a little if a sufficiently configured patchworks would be a
> better fit for gdb.
>
> The major problem with the current patchworks is that it doesn't
> automatically remove patches when they are checked in.  However, a newer
> version of patchworks can do this, especially if we augment it with a
> commit hook to add a UUID to the commit message (which we've already
> accepted for gerrit...).  It seems easy to set this up.

Sounds like you talk about this:

  https://patchwork.readthedocs.io/en/latest/deployment/installation/#optional-configure-your-vcs-to-automatically-update-patches

If we don't want to change our ways of working, then yes Patchwork would
be a better option, as it builds on top of the email workflow.  I only
have experience with the old patchwork, and thought it wasn't very good:
a inhuman amount of work was needed to shepherd it and manually change
patches state.  And it didn't add much value, it didn't tell me much that
I wouldn't know by looking at my email client.

If the script that automatically marks patches as merged can work for > 95%
of the patches, we can probably accept to do the rest of the cleanup by hand.
Patchwork also apparently can detect follow up versions of patches and patch
series.  I'm curious to see how it handles patches changing name/subject
between versions,  If it doesn't work reliably enough, we'll still end up doing
some of that sorting manually.

> Another drawback of patchworks is that reviews aren't done online -- you
> still use email.  This doesn't bother me, but maybe it's an important
> consideration for others.

I don't mind writing review comments by email, it's quite efficient.  But the killer
Gerrit feature for me as a reviewer is that I can so easily check the diff between
two versions.  For example, take this review I did:

https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/21/2..4

On the left is the version I reviewed, with my review comments, and on the right
the new version.  This lets me quickly just look at the new changes.  With email,
I have to dig up my old review email, and for each comment go look at that place
in the file to see if the comment was addressed, and if it was done correctly.
But since it's possible that other changes were added to the version of the patch,
I should ideally check the whole patch again.  This is very time consuming.  With
Gerrit's diff view, those changes would appear immediately.

> I'm not saying we should definitely switch -- just that I've noticed
> some drawbacks to gerrit as compared to email, and I am wondering if we
> can somehow preserve more of the good things.

Thanks for sharing your thoughts.

Simon