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

[Dylan Baker]

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. We
should either go all in and archive the mailing list and use only gitlab, or not
use PR/MR's IMHO.

> > +  <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
> 
> Can we disable this in Gitlab? If the button is there, people *will*
> accidentally press it.

I think Daniel was working on getting a "rebase and push" button like github has
so that we could use the button, CC'd.

> 
> > +  <li>Close your MR when your patches get pushed!
> 
> Gentoo has some automation that scans the git log for "Closes: ..."
> and automatically closes the merge requests or bugzilla bugs.
> 
>     Closes: https://github.com/gentoo/gentoo/pull/7423
>     Closes: https://bugs.gentoo.org/603294
> 
> I'm not sure if it's a custom thing or what (I can find out) but I'd
> much prefer to automate things if we can. Just like patchwork, people
> *will* forget to close merge requests if it's possible. (And people
> currently *do* forget to close bugzilla bugs)

Or we could just use gitlab's issue tracker, which will automatically close
issues when a commit pushed to master has a tag like `Fixes: #1234` or
`Closes: #1234`.

Personally speaking, I think that better next steps for gitlab integration are:
- migrate from bugzilla to gitlab issues
- convert piglit to a PR/MR workflow and see how it goes. The list for piglit
  is barely looked at anyway.

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/43cf04de/attachment-0001.sig>