Re: [Ksummit-discuss] [TOPIC] Encouraging more reviewers

[Daniel Vetter <daniel.vetter@ffwll.ch>]

On Thu, May 29, 2014 at 1:31 AM,  <josh@joshtriplett.org> wrote:
>> I really started to put breaks into this cycle of hell, where I get
>> spammed with a 30+ patch series in the morning and after I spent some
>> quality time looking at it and replying to a particular patch, I get
>> another spam bomb within a few hours, which is not much better than
>> the previous one.
>
> That's definitely a good workflow question.  We tell people to break
> huge patches down into pieces, and that can turn substantial changes
> into long patch series.  Which of the following is your primary concern:
>
> - That people update the patch series before you're done submitting
>   feedback on it?  That does seem like an annoying problem, and I
>   haven't seen anyone explicitly talk about it; we should document it,
>   and develop some standard boilerplate saying "If you get feedback on
>   patch 04/25, don't send out v2 of the whole series until you get the
>   remaining feedback on the patches".  And until users are reasonably
>   educated on that problem, frequent reviewers may want to include that
>   boilerplate in their patch reviews.

What I enforce on our mailing list is that for updates for issues in
specific patches submitters should resend only that patch in-reply-to
the old one. It means a bit more work of fiddling out the msg-id and
submitters complain (but meh), but it does nicely keep the discussion
together. And it allows submitters to chase reviewer's feedback while
the review is still ongoing without causing hell on the m-l. This
makes for a bit of fun when applying patches, but if it gets messy I
just ask for a full resend with r-b tags and all included once it's
all done.

Of course patchbombs which are fundamentally wrong need to be resent
hole. And for those I tell submitters I tell them pretty sternly that
they need to wait with resending until the review round concluded if
they fail to do so.

If course

> - That people send patch series with too many problems, and you're
>   frustrated having to educate them on a dozen fundamental problems
>   (formatting, standard API usage, etc) rather than just what's specific
>   to your subsystem?  It seems completely reasonable to remind people of
>   the standard tools, improve those standard tools as needed, and
>   ideally set up more automation to run those tools before patches hit
>   reviewers.  You shouldn't need to waste your time reviewing patches
>   that don't build, for instance.

Imo checkpatch makes this worse since with that you get patches that
look pretty but are still shit. Imo we need to push back on companies
who consistenly submit crap patches and tell them to get some good
internal mentoring going. Or throw money at lf/linaro/whatever to get
such expertise if they completely lack it.

And for me hobbyist/gsoc/... are completely different and I'll give
them much more leeway simply because I've started like that myself.

Of course for both these points Intel mostly gets this and I only have
issues with new people joining upstream from e.g. Android teams If
they don't get it I escalate through their managers _real_ quickly.
Maybe we should require from companies some "you have a loose cannon,
take care of it" contact, at least those in lf/linaro/... to make sure
this gets addressed quickly? Since ime sternly written mails at
engineers is usually not sufficient, escalation is usually required.

> - That a 30-patch series is painful to review via email?  Agreed
>   completely; I think we need better git-based workflows that don't
>   always have to fall back to the least-common-denominator of patchbomb
>   threads.

Nope if you mean tools like gerrit. The android gfx guys use it and I
think it's horrible. Mail-based patchbomb review works, and tool like
gerrit has all the sigils of a working review process but imo
completely misses the point. From a UI perspective the web thing is a
disaster, and everything else has too many indirections until you can
finally look at the patch. The only upside I see is that managers like
it since it helps to them to keep track of things. Not something
upstream cares about.

For our review we're looking into augmenting mail-based review with
stuff like patchwork, but atm patchwork is still lacking a lot. But at
least it's opt-in for everyone (even if a given maintainer uses it)
and doesn't replace the main mail discussions at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch