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

Wed Nov 28 19:41:45 UTC 2018

On Wed, Nov 28, 2018 at 2: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:
> > > > > >
> > > > > > 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 kinda liked the idea of send-cover-letter-to-list-with-link-to-MR
approach, just to avoid encouraging review happening in two places..
but that is just my $0.02

If there is not consensus, perhaps just picking one option for a
time-limited trial period (one or two months) and then revisit
afterwards would be a way to move forward?

BR,
-R

>
> 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
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev