[Mesa-dev] GitLab merge requests, discussion summary - Re: [PATCH] docs: Document optional GitLab code review process

Tue Dec 4 02:23:26 UTC 2018

Seems like a pretty good sum-up of the discussion to me.

On Mon, Dec 3, 2018 at 7:50 PM Jordan Justen <jordan.l.justen at intel.com>
wrote:

> Lots of discussion here. :) I hoped to just dip our toes into the
> merge request stream, but the consensus is clearly against my idea. Oh
> well. :)
>
> Here's what I gathered from the discussion:
>
> == Keep email, allow duplicate MR, my idea ==
>
> I don't think anyone else was in favor of this. Several were against
> it.
>
> == NACK merge requests altogether ==
>
> I did not see this, so that seems promising for merge requests. :) I
> guess no one strongly disagrees with trying merge requests.
>
> == Allow submitter (or driver) to choose to email or MR, but not both ==
>
> Several seemed to argue that this could be a good next step. But, in
> all those cases, I think they wanted to use merge requests going
> forward.
>
> It was mentioned that vc4 has moved to github in order to allow github
> pull requests to be used.
>
> == Require all email list, or all merge requests ==
>
> A few argued in favor of this, and seemed to feel pretty strongly
> about it. In both cases I believe they wanted to more to merge
> requests. :)
>
> == Next step? ==
>
> So, what do you all think is the best next step after the discussion?
>
> We could take the smaller step to allow the submitter, or project to
> decide which they prefer. It would allow those who prefer email to
> continue with email, while trying out gitlab merge requests. But, it
> could mean that devs need to check both gitlab and email.
>
> Or, we could just try to make the jump to MR only. Maybe if I write
> that patch someone will finally stand up to try to NACK moving away
> from email. :)
>
> Or, are we a little too early for this discussion, and we should just
> drop it for now?
>
> My opinion would be to try to move a bit slower and allow the
> submitter/project to choose for a trial period. (Dylan and Daniel seem
> to think this could be really frustrating for devs though.) But, if
> everyone seems to think it's reasonable to try to jump straight to
> using merge requests exclusively, I can type that version up...
>

I agree, for two reasons:

 1) Hard switches are hard on everyone; some people have outstanding work
that is mid-review and telling them to move it to a MR immediately may be
rough.  It can also be a big workflow break and I think it's good to let
people change their workflow at their own pace for now.

 2) It gives us a way out if things go sideways.  I don't expect it'll be
too bad but there will be issues.  Some people may prefer to keep sending
e-mails until certain issues are addressed in some way.

That said, given the over-all positive response towards MRs, I expect the
list will become a ghost town in no time and the full transition will
happen all by itself.

--Jason

> -Jordan
>
> On 2018-11-27 17:13:38, Jordan Justen wrote:
> > This documents a mechanism for using GitLab Merge Requests as an
> > optional, secondary way to get code reviews for patchsets.
> >
> > We still require all patches to be emailed.
> >
> > Aside from the potential usage for code review comments, it might also
> > help reviewers to find unmerged patchsets which need review. (Assuming
> > it doesn't fall into the same fate of patchwork with hundreds of open
> > MRs.)
> >
> > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > Cc: Jason Ekstrand <jason at jlekstrand.net>
> > ---
> >  docs/submittingpatches.html | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
> > index 5d8ba443191..852f28c198a 100644
> > --- a/docs/submittingpatches.html
> > +++ b/docs/submittingpatches.html
> > @@ -24,6 +24,7 @@
> >  <li><a href="#testing">Testing Patches</a>
> >  <li><a href="#mailing">Mailing Patches</a>
> >  <li><a href="#reviewing">Reviewing Patches</a>
> > +<li><a href="#gitlab">GitLab Merge Requests</a>
> >  <li><a href="#nominations">Nominating a commit for a stable branch</a>
> >  <li><a href="#criteria">Criteria for accepting patches to the stable
> branch</a>
> >  <li><a href="#backports">Sending backports for the stable branch</a>
> > @@ -282,6 +283,30 @@ which tells the patch author that the patch can be
> committed, as long
> >  as the issues are resolved first.
> >  </p>
> >
> > +<h2 id="gitlab">GitLab Merge Requests</h2>
> > +
> > +<p>
> > +  <a href="https://gitlab.freedesktop.org/mesa/mesa">GitLab</a> Merge
> > +  Requests (MR) can be used as an optional, secondary method of
> > +  obtaining code review for patches.
> > +</p>
> > +
> > +<ul>
> > +  <li>All patches should be submitted using email as well
> > +  <li>Consider including a link to the MR in your email based
> > +    cover-letter
> > +  <li>Address code review from both email and the MR
> > +  <li>Add appropriate labels to your MR. For example:
> > +    <ul>
> > +      <li>Mesa changes affect all drivers: mesa
> > +      <li>Hardware vendor specific code: amd, intel, nvidia, etc
> > +      <li>Driver specific code: anvil, freedreno, i965, iris, radeonsi,
> radv, vc4, etc
> > +      <li>Other tag examples: gallium, util
> > +    </ul>
> > +  <li>Never use the merge button on the GitLab page to push patches
> > +  <li>Add Reviewed-by tags to your commits and push using git
> > +  <li>Close your MR when your patches get pushed!
> > +</ul>
> >
> >  <h2 id="nominations">Nominating a commit for a stable branch</h2>
> >
> > --
> > 2.20.0.rc1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181203/7319bf80/attachment.html>