[Mesa-dev] [PATCH] docs: Document optional GitLab code review process

[Jason Ekstrand]

On Wed, Nov 28, 2018 at 1:16 PM Jordan Justen <jordan.l.justen at intel.com>
wrote:

> On 2018-11-28 10:14:49, Jason Ekstrand wrote:
> > On Wed, Nov 28, 2018 at 11:35 AM Jordan Justen <
> jordan.l.justen at intel.com>
> > wrote:
> > > On 2018-11-28 06:59:42, Eric Engestrom wrote:
> > > > On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:
> > > > > On 2018-11-27 19:20:09, Matt Turner wrote:
> > > > > > Strawman: maybe we can only email the cover letter to the mailing
> > > > > > list and include in it a link to the MR?
> > > > >
> > > > > I was hoping to make a smaller step and see what happens. Maybe
> this
> > > > > will give people the chance to try out MR based reviews, but not
> take
> > > > > away email based reviews just yet.
> > > > >
> > > > > I don't think we should move away from email based reviews until we
> > > > > are sure MR's actually work better. (I'm far from convinced on this
> > > > > point. :)
> > > >
> > > > I think one way to solve this is to allow both, but only one at a
> time.
> > > > Devs can choose to send their patches/series as either an MR _or_ as
> > > > emails, but not both. One can still send a cover-letter style email
> to
> > > > the ML to present the MR with a link to it.
> > >
> > > I would prefer to keep emails as required for now. Let people
> > > optionally try it out for some time.
> > >
> > > Perhaps as a next step we can let the poster consider using a MR and
> > > only a cover letter. (Assuming the MR method proves useful. If it
> > > doesn't we should revert back to email only.)
> > >
> >
> > I agree with Eric on this one.  If a single submission has both types of
> > review then you will effectively end up with two different groups of
> > reviewers providing potentially conflicting feedback without seeing each
> > other's feedback and the submitter has to mediate.
>
> They always have to. Case in point, this patch. I want to keep all
> patches on the list but let people optionally post an MR so people can
> try it out as a first step. Dylan says it has to be all or nothing.
> You and Eric say the submitter should be forced to choose one or the
> other. (I'm not sure where to go from here. :)
>

Yes, but the point is that we (the reviewers) know that we're conflicting.
That's very different from what I could easily see happening *a lot* were
ML reviewer A is perfectly happy with some bit of code but MR reviewer B
asks for it to be completely reworked.  In v2 of the series, the submitter
reworks it but now reviewer A is unhappy.  "Why did you change it?" he
says, "It was just fine before!".  "Reviewer B requested the rework," says
the submitter.  "When did he say that?  I didn't see that comment." says
B.  "On the GitLab MR," says the submitter.  "Well, I don't read MRs; this
kind of feedback should happen on the list where we can all read it," says
A.

If you can't immagine that exchange happening, then you haven't been on
this list long enough. :-)  (Says a guy who's been on the list for about
half as long as Jordan.)

We have enough stubborn people on the list that MRs are going to constantly
get pulled back to the list just because someone doesn't want to use the
web interface.  That's really mean to submitters who actually want to use
the MR process and is strictly worse than what we have today.  If we're
going to actually try out MRs, we need those people trying it too at least
from the reviewer side.


> I think this is a worst case scenario situation. Normally people
> comment about separate parts of code, and not often in conflicting
> ways.
>
> If a major conflict comes up, then the submitter could ask the MR
> based reviewer to add their point on the email list discussion. (Since
> we'd be keeping email as the primary.)
>
> > It's better just to
> > have any given series have a single point for review.  Yes, this means
> that
> > everyone will be forced to start doing GitHub review as soon as someone
> > does an MR that's relevant to them.  I don't see a good way around that
> > which doesn't make a mess.
> >
> > Certain projects could, I suppose, have a requirement that all
> significant
> > work on that project be done on the list or on GitLab.  However, people
> who
> > feel like custodians of Mesa as a whole will have to do both as long as
> > both are supported.
>
> I guess by 'projects', you mean sub-projects within Mesa? I'm not
> against Eric's idea of allowing the submitter to choose email list vs
> MR, but I do think we should hold off and consider that in 3~6 months.
>

Right.  Eric Anholt has already moved most of vc4 development to GitHub
just so he can use PRs for review.  If he wants vc4 submissions to go
through GitLab MRs, that should be his choice.  I don't see us making that
switch for Intel code any time particularly soon but I think it's worth
allowing.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181128/993f5a8b/attachment.html>