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

[Eric Engestrom]

On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:
> On 2018-11-27 19:20:09, Matt Turner wrote:
> > 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.
> 
> That's possible. It also happens on email sometimes. But, I want to
> say that maybe the usual problem is too little code review, and not
> too much? :)

I think duplicating the review possibilities might very well lead to
less review as people might (unconsciously) think "it has been/will be
reviewed on the other one".

> 
> > Strawman: maybe we can only email the cover letter to the mailing
> > list and include in it a link to the MR?
> 
> I was hoping to make a smaller step and see what happens. Maybe this
> will give people the chance to try out MR based reviews, but not take
> away email based reviews just yet.
> 
> I don't think we should move away from email based reviews until we
> are sure MR's actually work better. (I'm far from convinced on this
> point. :)

I think one way to solve this is to allow both, but only one at a time.
Devs can choose to send their patches/series as either an MR _or_ as
emails, but not both. One can still send a cover-letter style email to
the ML to present the MR with a link to it.

I also agree with Matt that we need to disable everything that shouldn't
be used, but unfortunately we can't disable the merge button without
disabling merge requests.

As for auto-closing issues and MRs by adding a special string to the
commit message, GitLab doesn't support that for commits that are pushed
behind its back, but it does support it it GitLab is used.
Adding a post-receive hook that does that should be possible, I'm not
sure how much work that would be and if it would be worth it.
DanielS?

Other things that need to be present in this 'GitLab MR' document are:

- Review process: when you change something in your MR, force-push the
  new version, don't add fixup commits. The history of the MR at any one
  time must be "clean" in the sense that it would be acceptable to push
  it as is.

- WIP MRs: prefix your MR title with "WIP" to indicate that it's a work
  in progress and is not ready to be merged as is (Side note: GitLab
  will not offer to merge an MR that is prefixed with "WIP")

- I think we should split the list in two between the dev side and the
  reviewer side.

- When an issue is raised, who closes it? The dev to indicate that the
  new revision should fix it, or the reviewer to indicate that they have
  verified the fix?