<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Wed, Nov 28, 2018 at 1:16 PM Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2018-11-28 10:14:49, Jason Ekstrand wrote:<br>
> On Wed, Nov 28, 2018 at 11:35 AM Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>><br>
> wrote:<br>
> > On 2018-11-28 06:59:42, Eric Engestrom wrote:<br>
> > > On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:<br>
> > > > On 2018-11-27 19:20:09, Matt Turner wrote:<br>
> > > > > Strawman: maybe we can only email the cover letter to the mailing<br>
> > > > > list and include in it a link to the MR?<br>
> > > ><br>
> > > > I was hoping to make a smaller step and see what happens. Maybe this<br>
> > > > will give people the chance to try out MR based reviews, but not take<br>
> > > > away email based reviews just yet.<br>
> > > ><br>
> > > > I don't think we should move away from email based reviews until we<br>
> > > > are sure MR's actually work better. (I'm far from convinced on this<br>
> > > > point. :)<br>
> > ><br>
> > > I think one way to solve this is to allow both, but only one at a time.<br>
> > > Devs can choose to send their patches/series as either an MR _or_ as<br>
> > > emails, but not both. One can still send a cover-letter style email to<br>
> > > the ML to present the MR with a link to it.<br>
> ><br>
> > I would prefer to keep emails as required for now. Let people<br>
> > optionally try it out for some time.<br>
> ><br>
> > Perhaps as a next step we can let the poster consider using a MR and<br>
> > only a cover letter. (Assuming the MR method proves useful. If it<br>
> > doesn't we should revert back to email only.)<br>
> ><br>
> <br>
> I agree with Eric on this one.  If a single submission has both types of<br>
> review then you will effectively end up with two different groups of<br>
> reviewers providing potentially conflicting feedback without seeing each<br>
> other's feedback and the submitter has to mediate.<br>
<br>
They always have to. Case in point, this patch. I want to keep all<br>
patches on the list but let people optionally post an MR so people can<br>
try it out as a first step. Dylan says it has to be all or nothing.<br>
You and Eric say the submitter should be forced to choose one or the<br>
other. (I'm not sure where to go from here. :)<br></blockquote><div><br></div><div>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.</div><div><br></div><div>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.)</div><div><br></div><div>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.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
I think this is a worst case scenario situation. Normally people<br>
comment about separate parts of code, and not often in conflicting<br>
ways.<br>
<br>
If a major conflict comes up, then the submitter could ask the MR<br>
based reviewer to add their point on the email list discussion. (Since<br>
we'd be keeping email as the primary.)<br>
<br>
> It's better just to<br>
> have any given series have a single point for review.  Yes, this means that<br>
> everyone will be forced to start doing GitHub review as soon as someone<br>
> does an MR that's relevant to them.  I don't see a good way around that<br>
> which doesn't make a mess.<br>
> <br>
> Certain projects could, I suppose, have a requirement that all significant<br>
> work on that project be done on the list or on GitLab.  However, people who<br>
> feel like custodians of Mesa as a whole will have to do both as long as<br>
> both are supported.<br>
<br>
I guess by 'projects', you mean sub-projects within Mesa? I'm not<br>
against Eric's idea of allowing the submitter to choose email list vs<br>
MR, but I do think we should hold off and consider that in 3~6 months.<br></blockquote><div><br></div><div>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.<br></div></div></div>