some thoughts on gerrit

some thoughts on gerrit

[Tom Tromey]

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.


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?


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.

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'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.

Tom

Re: some thoughts on gerrit

[Sergio Durigan Junior]

On Sunday, October 20 2019, Tom Tromey wrote:

> After using gerrit for gdb for just this past week, I have mixed
> feelings, which I thought I'd share.

Thanks for your email, Tom.

> 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.

I haven't submitted any real patches yet, but for me this problem
doesn't apply, so I can't comment on it.  My workflow is likely to stay
the same: git-new-workdir, hack, submit (either via gerrit or git
send-email), push.

> 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.

I saw your patch series yesterday night, and I had the exact same
thought as you.  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.

I absolutely agree.

> 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 see your point, but I still think a cover letter serves a different
purpose than the commit messages.  A cover letter introduces the big
picture, and it is often a good place for reviewers to make generic
comments about the overall implementation.

> I wonder if there'd be a way to make "git review" prompt for a cover
> letter and attach it somewhere as a comment.

That could work, I guess, but it would still be a bit cumbersome.

I found this on gerrit's bugzilla:

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

It seems like there was a request, but it was closed very recently due
to inactivity.  Perhaps we could try pushing a bit more...

> Lack of threading means it is hard to see how patches relate when
> reading in email.  Maybe this can be fixed in gerrit?

I don't know, but I'm hoping that when I configure gerrit to accept
emails, this will be "magically" fixed.  The reason is because if gerrit
accepts emails, it is also the only "entity" to reply to its own
messages, which may be easier for gerrit to keep track of Message-Id's
and In-Reply-To.

I should have an update about this tomorrow, and we can see what
happens.

> 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.
>
> 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.

The way I see it, I think the top patch reviewers should come together
and decide what works best for them.  Maybe it is patchwork, maybe it's
something else.  I know that I would not use patchwork that much, even
if it is configure to automatic remove patches when they're pushed, but
in this case I think patchwork's purpose would be only to serve as a
dashboard for whoever is reviewing the patches.

> 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.

Sure thing.  I'd like to see if the threading problem gets fixed when
gerrit starts accepting emails, at least.  Other than that, I personally
don't have a preference, and I would not feel bad if the community
decides to drop gerrit.

Cheers,

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

Re: some thoughts on gerrit

[Sergio Durigan Junior]

On Sunday, October 20 2019, I wrote:

> On Sunday, October 20 2019, Tom Tromey wrote:
>> 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.
>
> I absolutely agree.
>
>> 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 see your point, but I still think a cover letter serves a different
> purpose than the commit messages.  A cover letter introduces the big
> picture, and it is often a good place for reviewers to make generic
> comments about the overall implementation.
>
>> I wonder if there'd be a way to make "git review" prompt for a cover
>> letter and attach it somewhere as a comment.
>
> That could work, I guess, but it would still be a bit cumbersome.
>
> I found this on gerrit's bugzilla:
>
>   https://bugs.chromium.org/p/gerrit/issues/detail?id=924
>
> It seems like there was a request, but it was closed very recently due
> to inactivity.  Perhaps we could try pushing a bit more...

Ah, something I forgot to say: a possible (but hacky) workaround that I
thought for the cover letter problem is to create an empty commit before
everything else, with just the commit message.  We'd have to
"garbage-collect" it manually when the series is pushed (or maybe we can
figure out a way to do that automatically; not sure), but it could
work...

The other problem with this workaround is that it will not be possible
to create threading using the cover letter as the root message.

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/

Re: some thoughts on gerrit

[Andreas Schwab]

On Okt 20 2019, Sergio Durigan Junior wrote:

> doesn't apply, so I can't comment on it.  My workflow is likely to stay
> the same: git-new-workdir, hack, submit (either via gerrit or git

Consider using git worktree instead, git-new-workdir is obsolete.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: some thoughts on gerrit

[Christian Biesinger via gdb-patches]

FWIW, I filed https://bugs.chromium.org/p/gerrit/issues/detail?id=11791
and reopened https://bugs.chromium.org/p/gerrit/issues/detail?id=924

Christian

On Mon, Oct 21, 2019 at 4:03 AM Andreas Schwab <schwab@suse.de> wrote:
>
> On Okt 20 2019, Sergio Durigan Junior wrote:
>
> > doesn't apply, so I can't comment on it.  My workflow is likely to stay
> > the same: git-new-workdir, hack, submit (either via gerrit or git
>
> Consider using git worktree instead, git-new-workdir is obsolete.
>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

Re: some thoughts on gerrit

[Sergio Durigan Junior]

On Monday, October 21 2019, Christian Biesinger wrote:

> FWIW, I filed https://bugs.chromium.org/p/gerrit/issues/detail?id=11791
> and reopened https://bugs.chromium.org/p/gerrit/issues/detail?id=924

Thanks, Christian.

I think the two bugs are closely related.  If gerrit doesn't send a
cover letter, then it's hard to thread the emails.  One can argue that
the first patch should become the root message, but it's not so
organized.  A cover letter really simplifies things in this case.

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

Re: some thoughts on gerrit

[Sergio Durigan Junior]

On Monday, October 21 2019, Andreas Schwab wrote:

> On Okt 20 2019, Sergio Durigan Junior wrote:
>
>> doesn't apply, so I can't comment on it.  My workflow is likely to stay
>> the same: git-new-workdir, hack, submit (either via gerrit or git
>
> Consider using git worktree instead, git-new-workdir is obsolete.

Ah, thanks, I don't use git-new-workdir directly (instead, I have a
script that invokes it), so I didn't know about this deprecation.

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

Re: some thoughts on gerrit

[Andrew Pinski]

On Sun, Oct 20, 2019 at 7:37 AM Tom Tromey <tom@tromey.com> 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.
>
>
> 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
> ]]]

Hi,
 First, we use gerrit internally at Marvell.  I have another benefit
for gerrit, which is not listed here.  You can configure gerrit to
hook into an automation service which will give then automated
feedback.  E.g. we use it to run check patch (on Linux kernel patches
and others) and then if the patch builds and tests it on a few
platforms.
I don't know if you could use patchworks to do that though.

Thanks,
Andrew Pinski

>
>
> 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?
>
>
> 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.
>
> 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'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.
>
> Tom

Re: some thoughts on gerrit

[Simon Marchi]

On 2019-10-20 7:16 p.m., Andrew Pinski wrote:
> Hi,
>  First, we use gerrit internally at Marvell.  I have another benefit
> for gerrit, which is not listed here.  You can configure gerrit to
> hook into an automation service which will give then automated
> feedback.  E.g. we use it to run check patch (on Linux kernel patches
> and others) and then if the patch builds and tests it on a few
> platforms.
> I don't know if you could use patchworks to do that though.
> 
> Thanks,
> Andrew Pinski

Hi Andrew,

Indeed, this is part of what we want to do, make it really easy to test
a given patch on the buildbot.

Git-based patch systems like Gerrit make it really easy to do that.  Because
people push git commits, it's always works to just check out those commits to
test them.

With email patches, we can try to apply patches, but they may not apply cleanly,
either because the person didn't use git-send-email, or the patches were based
on an older commit.

It seems like patchwork, has an event API that can tell you when a new patch
is detected, and that can be used to trigger CI builds:

https://patchwork.readthedocs.io/en/latest/usage/overview/#events

The CI system can then attach some metadata as "checks":

https://patchwork.readthedocs.io/en/latest/usage/overview/#checks

Here's a FOSDEM video I found about these features:

https://www.youtube.com/watch?v=zSexR5c4Vxk

Simon

Re: some thoughts on gerrit

[Simon Marchi]

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