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

Wed Nov 28 21:54:13 UTC 2018

Quoting Jordan Justen (2018-11-28 10:21:13)
> On 2018-11-28 09:22:35, Dylan Baker wrote:
> > Quoting Matt Turner (2018-11-27 19:20:09)
> > > On Tue, Nov 27, 2018 at 5:13 PM Jordan Justen <jordan.l.justen at intel.com> 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
> > > 
> > > Discussion point: I think attempting to have simultaneous review in
> > > two places is a recipe for wasted time. Strawman: maybe we can only
> > > email the cover letter to the mailing list and include in it a link to
> > > the MR?
> > 
> > I think it's a *really* bad idea to allow both. Some people will immediately
> > begin using MR/PR's only and never read the list, others will never check
> > MR/PRs and only use the list. It'll leave the rest of us forced to use both.
> 
> Requiring emails seems to avoid these issues for now. We'd just be
> trying out using merge requests, but if you want to see all patches,
> use email.
> 
> > We should either go all in and archive the mailing list and use only
> > gitlab, or not use PR/MR's IMHO.
> 
> If that is the only choice, then I choose that we not use MR. Or, wait
> until everyone is forced to try MR via other freedesktop projects. :)
> 
> One motivation I had for this patch was to see if MR could be a
> 'patchwork with labels'. Although, unlike patchwork, the contributor
> would have to take an extra step to open the MR and apply the label.
> One positive vs patchwork would be having the same login system as
> Mesa's repo.
> 
> -Jordan

This really feels like we're asking people to do more work. They now need to
send patches to the list, open an MR, add lables, then take into account review
feedback from both sources (and deal with conflicting review, which resolves
itself naturally with only *one* place to do review), send out a v2 and force
push so gitlab notices. Then use git to push, and close their MR.

I know I sound really critical here and I'm sorry if I'm coming off gruff.
Patchwork has really burned me on "optional" features like this. I tried to use
patchwork, but just ended up spending a ton of time doing janitorial work, which
very few people really seemed interested in. I'm afraid this is going to be a
community split with neither option being definitively "better" than the other.
Some members of the community really like email, plain text, and list based
review; others hate it, and really like the web driven approach; but those are
just "likes" there isn't something we can point at and say "yeah, X is annoying,
but Y makes it worth while".

I'm being honest, I don't like mailing lists as much as I like gitlab, but this
hybrid approach feels like it's just doubling the amount of work I need to do,
and under this proposal I don't plan to use gitlab, I'm just going to keep
sending regular series the mailinglists. Again, if the community is interested
in trying an MR workflow I think piglit is a great place to try that out.

Dylan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: signature
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181128/97866eab/attachment-0001.sig>