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

[Eric Engestrom]

On Wednesday, 2018-11-28 13:36:29 -0800, Eric Anholt wrote:
> Jordan Justen <jordan.l.justen at intel.com> writes:
> 
> > 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
> 
> Like others, I disagree and think this will lead to confusion.  Let
> people send to one or the other.
> 
> > +  <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
> 
> Why "never use the merge button"?  We've been using rebase+merge in
> xserver and it's *awesome* and has greatly increased the review rate.

Because GitLab doesn't (yet) support tracking 'reviewed' status in
the commit message, which is something I think is important to track for
various reasons. If those are manually added to the commit messages
before the MR is merged, then there's no issue there.

We could also have a check that an MR can't be merged until all its
commits contain the string "Reviewed-by:", which might be a better
solution?

("yet" because there's some upstream discussion on this, but it
hasn't materialized into an MR yet.)

> 
> > +  <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>
> 
> Overall:
> 
> I can't wait to have a full MR process.  What if we just *never* merged
> code that broke the build or introduced warnings, because the MR process
> ensured it?  How much time would we all save?  How much less training
> would we need to do on new contributors?

Agreed on all counts, and I believe we'll get there soon ;)