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

[Jordan Justen]

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

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