On Wed, Nov 28, 2018 at 2:16 PM Jordan Justen <jordan.l.jus...@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.jus...@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
