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

[Jordan Justen]

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:
> > > > >
> > > > > Discussion point: I think attempting to have simultaneous review in
> > > > > two places is a recipe for wasted time.
> > > >
> > > > That's possible. It also happens on email sometimes. But, I want to
> > > > say that maybe the usual problem is too little code review, and not
> > > > too much? :)
> > >
> > > I think duplicating the review possibilities might very well lead to
> > > less review as people might (unconsciously) think "it has been/will be
> > > reviewed on the other one".
> >
> > Maybe they were looking for an excuse to not do code review? :)
> >
> > I agree with your point to some extent, but I think it's better to try
> > to work out a transition. To not jump immediately to forcing people to
> > go to the web page.
> >
> > >
> > > > > 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. :)

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.

> > > Other things that need to be present in this 'GitLab MR' document are:
> > >
> > > - Review process: when you change something in your MR, force-push the
> > >   new version, don't add fixup commits. The history of the MR at any one
> > >   time must be "clean" in the sense that it would be acceptable to push
> > >   it as is.
> >
> > We don't document this for email based reviews, but people seem to
> > figure it out. :)
> >
> > Is this more of a concern because they might not know that pushing the
> > same branch updates the MR?
> >
> 
> I think it's worth explicitly documenting.  Many people who are used to the
> MR workflow are also used to having very sloppy branches because they
> assume that only the final merge matters.  This is why GitLab and GitHub
> both have an option for "squash before merge". :-(

Ok. It seems reasonable to add.

-Jordan