Re: [Ksummit-discuss] [MAINTAINERS SUMMIT] Pull network and Patch Acceptance Consistency

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

On Fri, Jun 14, 2019 at 6:16 PM James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Fri, 2019-06-14 at 13:04 -0300, Mauro Carvalho Chehab wrote:
> > Em Fri, 14 Jun 2019 08:49:46 -0700
> > James Bottomley <James.Bottomley@HansenPartnership.com> escreveu:
> >
> > > On Fri, 2019-06-14 at 12:43 -0300, Mauro Carvalho Chehab wrote:
> > > > Em Fri, 14 Jun 2019 08:23:05 -0700
> > > > James Bottomley <James.Bottomley@HansenPartnership.com> escreveu:
> > > >
> > > > > On Fri, 2019-06-14 at 12:11 -0300, Mauro Carvalho Chehab wrote:
> > > > > [...]
> > > > > > If you think this is relevant to a broader audience, let me
> > > > > > reply with a long answer about that. I prepared it and
> > > > > > intended to reply to our internal media maintainer's ML
> > > > > > (added as c/c).
> > > > > >
> > > > > > Yet, I still think that this is media maintainer's dirty
> > > > > > laundry and should be discussed elsewhere ;-)
> > > > > >
> > > > > > ---
> > > > >
> > > > > So trying not to get into huge email thread, I think this is
> > > > > the key point:
> > > > >
> > > > > [...]
> > > > > > Currently, I review all accepted patches.
> > > > >
> > > > > This means you effectively have a fully flat tree.
> > > >
> > > > Yes.
> > > >
> > > > > Even if you use git, you're using it like an email transmission
> > > > > path.  One of the points I was making about deepening the tree
> > > > > is that the maintainer in the middle should trust the
> > > > > submaintainer they pull from, so there should be no need to
> > > > > review all the patches because of that trust.
> > > >
> > > > It is not a matter of trust. It is just that the media subsystem
> > > > is a complex puzzle. Just the V4L2 API has more than 80 ioctls.
> > > >
> > > > So, the goal here is to do my best to ensure that patches will
> > > > get at least two reviews.
> > > >
> > > > > This is how deepening the tree helps to offload maintainers
> > > > > because review is one of the biggest burdens we have and
> > > > > deepening the tree is a way to share it.  Without trust, we
> > > > > achieve no offloading and therefore no utility from deepening
> > > > > the tree.
> > > >
> > > > Yeah, I know one day this won't scale. The day it happens, I'll
> > > > just start picking pull requests. As we already use git, a
> > > > change like that would be trivial.
> > > >
> > > > > So, to get back to the original question, which was *should* we
> > > > > deepen the tree: why don't you feel you can let branches with
> > > > > patches you haven't reviewed into your
> > > > > tree?  I've characterised it as a
> > > > > trust issue above, but perhaps it isn't. I think this is a key
> > > > > question which would help us understand whether a deeper tree
> > > > > model is at all possible.
> > > >
> > > > One of the aspects is that developers nowadays are specialists on
> > > > a subset of the media devices. Most of them are working on
> > > > complex camera support, with envolves a subset of the APIs we
> > > > have. They never worked on a driver that would use other parts of
> > > > the API, like DVB, Remote Controllers, TV, V4L2 streaming
> > > > devices, etc.
> > > >
> > > > So, having someone with a more generalist view at the end of the
> > > > review process helps to identify potential problems that might
> > > > affect other devices, specially when there are API changes
> > > > involved[1].
> > > >
> > > > [1] Since when I started maintaining the subsystem, back on 2005,
> > > > on almost every single Kernel review there are API changes in
> > > > order to support new types of hardware.
> > >
> > > Actually, this leads me to the patch acceptance criteria: Is there
> > > value in requiring reviews?  We try to do this in SCSI (usually
> > > only one review), but if all reviewers add a
> > >
> > > Reviewed-by:
> > >
> > > tag, which is accumulated in the tree, your pull machinery can
> > > detect it on all commits in the pull and give you an automated
> > > decision about whether to accept the pull or not.  If you require
> > > two with one from a list of designated reviewers, it can do that as
> > > well (with a bit more complexity in the pull hook script).
> > >
> > > So here's the question: If I help you script this, would you be
> > > willing to accept pull requests in the media tree with this check
> > > in place?  I'm happy to do this because it's an interesting
> > > experiment to see if we can have automation offload work currently
> > > done by humans.
> >
> > We could experiment something like that, provided that people will be
> > aware that it can be undone if something gets wrong.
> >
> > Yet, as we discussed at the Media Summit, we currently have an
> > issue: our infrastructure lack resources for such kind of
> > automation.
>
> This one doesn't require an automation infrastructure: the script runs
> as a local pull hook on the machine you accept the pull request from
> (presumably your laptop?) so  the workflow is you receive a pull
> request, pull it into your tree and if the pull hook finds a bogus
> commit it will reject the pull and tell you why; if the script accepts
> the pull then you do whatever additional checks you like, then push it
> to kernel.org when you're satisfied it didn't break anything.

Jumping in here with a +1 on sharing scripts. We have the drm
inglorious maintainer scripts which originated from drm-intel.git, but
is now used for drm-misc.git and drm.git overall two. There's
essentially now three kinds of trees we pull into drm.git:

- the well-maintained ones that use those scripts: Experienced
maintainers and reviewers make sure the big picture is solid, and the
scripting makes sure all the details are done correctly too (e.g.
we've recently added the Fixes: validation that linux-next started
reporting). Processing those pulls is a button-push fire&forget
affair.

- well-maintained trees without good tooling. High level will be all
solid, but our scripting will catch the oddball screwed up Fixes: tag,
misformatted sob or slightly botched last-minute rebase. So
occasionally it's not just single button push but takes another
iteration to report the issues and create a revised pull.

- the not-so-well maintained trees. Usually just a handful of patches
(or less) per release cycle. Those you actually have to look at
patches and check a pile of things. I'm trying to get them merged into
bigger existing teams (like drm-misc.git) so they could benefit&learn
from all the tooling and experience, but for some folks it's a hard
sell. And I don't want to be too obnoxious about enforcing certain
process and maybe annoying the maintainer of that driver.

As a rule everytime someone screws up and the script doesn't catch it,
we volunteer them to improve the scripting.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch