[Mesa-dev] [PATCH v2] docs: Document GitLab merge request process (email alternative)

Thu Dec 13 17:07:30 UTC 2018

On Thu, Dec 13, 2018 at 4:52 PM Alex Deucher <alexdeucher at gmail.com> wrote:
>
> On Wed, Dec 12, 2018 at 3:42 AM Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
> >
> > Personally, I will continue to use the list, at least for a simplicity
> > point of view. I'm not sure if using a new tool will improve quality and
> > code review process.
> >
> > Though, if the majority reports that is really nice to use, I will
> > probably change my mind. Not a strong reject.
>
> I agree.  We've been using the MR interface for xf86-video-amdgpu and
> I find it awkward compared to the mailing list.  Maybe it just takes
> getting used to.  I also feel less inclined to do drive by patch
> review if I have to explicitly delve into the browser to look at the
> outstanding MRs.  Over email, sometimes I see a patch set in my in box
> that piques my interest and I find some time to review it when I might
> not have otherwise if the bar were higher.
>
> Out of curiosity what do others like about the MR interface?  How are
> you using it?  What advantages does it give you over the mailing list?
>  I feel like maybe I'm not using it right or missing features that
> make it more useful and less awkward.  I feel like the interface makes
> it harder than it needs to be to see the actual changes in MR to be
> reviewed.  All of the links click through to the source view rather
> than the patch view.

FWIW, the killer feature for me (if it actually works out in practice)
is that I can keep track of patches that still need attention.

With me doing this in my free time mostly still and the mesa list
being pretty high volume, as well the tendency of giving regular
reviews by r-b, it is very hard for me to track what MRs I still need
to review, what has been pushed and what has been abandoned. If a
series is not nearly trivial and I review it in one evening I lose
track easily. Furthermore, I think patchwork has pretty much shown
that deriving this from email is not going to work. Doing this in a
more integrated environment where we can actually get people to
indicate the status is IMO the only way to work towards getting there.

As far as the actual reviewing itself, sure I agree that it needs improvement.

>
> Alex
>
> >
> > On 12/6/18 12:32 AM, Jordan Justen wrote:
> > > This documents a process for using GitLab Merge Requests as an second
> > > way to submit code changes for Mesa. Only one of the two methods is
> > > allowed for each patch series.
> > >
> > > We will *not* require all patches to be emailed. Some code changes may
> > > be reviewed and merged without any discussion on the mesa-dev email
> > > list.
> > >
> > > v2:
> > >   * No longer require email. Allow submitter to choose email or a
> > >     GitLab merge request.
> > >   * Various feedback from Brian, Daniel, Dylan, Eric, Erik, Jason,
> > >     Matt, Michel and Rob.
> > >
> > > Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> > > ---
> > >   docs/submittingpatches.html | 76 ++++++++++++++++++++++++++++++++++---
> > >   1 file changed, 71 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
> > > index 92d954a2d09..21175988d0b 100644
> > > --- a/docs/submittingpatches.html
> > > +++ b/docs/submittingpatches.html
> > > @@ -21,7 +21,7 @@
> > >   <li><a href="#guidelines">Basic guidelines</a>
> > >   <li><a href="#formatting">Patch formatting</a>
> > >   <li><a href="#testing">Testing Patches</a>
> > > -<li><a href="#mailing">Mailing Patches</a>
> > > +<li><a href="#submit">Submitting Patches</a>
> > >   <li><a href="#reviewing">Reviewing Patches</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>
> > > @@ -42,8 +42,10 @@ components.
> > >   <code>git bisect</code>.)
> > >   <li>Patches should be properly <a href="#formatting">formatted</a>.
> > >   <li>Patches should be sufficiently <a href="#testing">tested</a> before submitting.
> > > -<li>Patches should be submitted to <a href="#mailing">mesa-dev</a>
> > > -for <a href="#reviewing">review</a> using <code>git send-email</code>.
> > > +<li>Patches should be <a href="#submit">submitted</a>
> > > +to <a href="#mailing">mesa-dev</a> or with
> > > +a <a href="#merge-request">merge request</a>
> > > +for <a href="#reviewing">review</a>.
> > >
> > >   </ul>
> > >
> > > @@ -180,10 +182,19 @@ run.
> > >   </p>
> > >
> > >
> > > -<h2 id="mailing">Mailing Patches</h2>
> > > +<h2 id="submit">Submitting Patches</h2>
> > >
> > >   <p>
> > > -Patches should be sent to the mesa-dev mailing list for review:
> > > +Patches may be submitted to the Mesa project by
> > > +<a href="#mailing">email</a> or with a
> > > +GitLab <a href="#merge-request">merge request</a>. To prevent
> > > +duplicate code review, only use one method to submit your changes.
> > > +</p>
> > > +
> > > +<h3 id="mailing">Mailing Patches</h3>
> > > +
> > > +<p>
> > > +Patches may be sent to the mesa-dev mailing list for review:
> > >   <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
> > >   mesa-dev at lists.freedesktop.org</a>.
> > >   When submitting a patch make sure to use
> > > @@ -217,8 +228,63 @@ disabled before sending your patches. (Note that you may need to contact
> > >   your email administrator for this.)
> > >   </p>
> > >
> > > +<h3 id="merge-request">GitLab Merge Requests</h3>
> > > +
> > > +<p>
> > > +  <a href="https://gitlab.freedesktop.org/mesa/mesa">GitLab</a> Merge
> > > +  Requests (MR) can also be used to submit patches for Mesa.
> > > +</p>
> > > +
> > > +<p>
> > > +  If the MR may have interest for most of the Mesa community, you can
> > > +  send an email to the mesa-dev email list including a link to the MR.
> > > +  Don't send the patch to mesa-dev, just the MR link.
> > > +</p>
> > > +<p>
> > > +  Add labels to your MR to help reviewers find it. For example:
> > > +  <ul>
> > > +    <li>Mesa changes affecting all drivers: mesa
> > > +    <li>Hardware vendor specific code: amd, intel, nvidia, ...
> > > +    <li>Driver specific code: anvil, freedreno, i965, iris, radeonsi,
> > > +      radv, vc4, ...
> > > +    <li>Other tag examples: gallium, util
> > > +  </ul>
> > > +</p>
> > > +<p>
> > > +  If you revise your patches based on code review and push an update
> > > +  to your branch, you should maintain a <strong>clean</strong> history
> > > +  in your patches. There should not be "fixup" patches in the history.
> > > +  The series should be buildable and functional after every commit
> > > +  whenever you push the branch.
> > > +</p>
> > > +<p>
> > > +  It is your responsibility to keep the MR alive and making progress,
> > > +  as there are no guarantees that a Mesa dev will independently take
> > > +  interest in it.
> > > +</p>
> > > +<p>
> > > +  Some other notes:
> > > +  <ul>
> > > +    <li>Make changes and update your branch based on feedback
> > > +    <li>Old, stale MR may be closed, but you can reopen it if you
> > > +      still want to pursue the changes
> > > +    <li>You should periodically check to see if your MR needs to be
> > > +      rebased
> > > +    <li>Make sure your MR is closed if your patches get pushed outside
> > > +      of GitLab
> > > +  </ul>
> > > +</p>
> > > +
> > >   <h2 id="reviewing">Reviewing Patches</h2>
> > >
> > > +<p>
> > > +  To participate in code review, you should monitor the
> > > +  <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">
> > > +  mesa-dev</a> email list and the GitLab
> > > +  Mesa <a href="https://gitlab.freedesktop.org/mesa/mesa/merge_requests">Merge
> > > +  Requests</a> page.
> > > +</p>
> > > +
> > >   <p>
> > >   When you've reviewed a patch on the mailing list, please be unambiguous
> > >   about your review.  That is, state either
> > >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev