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

[Eric Anholt]

Eric Engestrom <eric.engestrom at intel.com> writes:

> 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?

In the xserver we decided that it was more important to have MRs be
available (so a reviewer can click "merge when the tests are green")
than to have in-git tracking of whether commits were reviewed.  I think
it's been a great tradeoff, but I understand if Mesa doesn't want to
make the same one.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181129/5fccf59f/attachment.sig>