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

Wed Nov 28 18:14:49 UTC 2018

First off, +1 to experimenting with MRs.  I've been working with GitLab MRs
in another context for some time and I think the process actually works out
really pretty well.  There are issues, of course, but I don't think there's
any real show-stoppers as long as we have a bit of process around it such
as requiring the submitter to apply Reviewed-by tags manually before
merging.

On Wed, Nov 28, 2018 at 11:35 AM Jordan Justen <jordan.l.justen at intel.com>
wrote:

> On 2018-11-28 06:59:42, Eric Engestrom wrote:
> > On Tuesday, 2018-11-27 19:45:50 -0800, Jordan Justen wrote:
> > > On 2018-11-27 19:20:09, Matt Turner wrote:
> > > >
> > > > 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".
>
> Maybe they were looking for an excuse to not do code review? :)
>
> I agree with your point to some extent, but I think it's better to try
> to work out a transition. To not jump immediately to forcing people to
> go to the web page.
>
> >
> > > > 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 would prefer to keep emails as required for now. Let people
> optionally try it out for some time.
>
> Perhaps as a next step we can let the poster consider using a MR and
> only a cover letter. (Assuming the MR method proves useful. If it
> doesn't we should revert back to email only.)
>

I agree with Eric on this one.  If a single submission has both types of
review then you will effectively end up with two different groups of
reviewers providing potentially conflicting feedback without seeing each
other's feedback and the submitter has to mediate.  It's better just to
have any given series have a single point for review.  Yes, this means that
everyone will be forced to start doing GitHub review as soon as someone
does an MR that's relevant to them.  I don't see a good way around that
which doesn't make a mess.

Certain projects could, I suppose, have a requirement that all significant
work on that project be done on the list or on GitLab.  However, people who
feel like custodians of Mesa as a whole will have to do both as long as
both are supported.

> > 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.
>
> I guess we can't do this. Can we just tell people with push access to
> not do that again if they make a mistake?
>
> Can merge request approvals be used to only prevent the web page from
> merging things? (But, still allow pushes.)
>
> https://docs.gitlab.com/ee/user/project/merge_requests/merge_request_approvals.html
>
> Maybe require approval from "dont-merge-use-git-push"? :)
>

I don't think we can disable pushing the "merge" button.  However, we can
require fast-forward merges and we can require either through process or
through a Git hook of some form that they have the right tags.  At that
point whether the person rebases, applies tags, force pushes the MR branch,
and then clicks merge or pushes directly to master is immaterial.

> > 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.
>
> Are you sure? Pushes seem to work this way on github, and my reading of
> https://docs.gitlab.com/ee/user/project/issues/automatic_issue_closing.html
> is that on gitlab pushes do the same.
>

I think pushes do close issues.

It's also worth throwing out there that GitLab has some sort of Bugzilla
interaction interface.  I'm not sure what all it lets you do but it's there
if we want to experiment with it.  That said, migrating to GitLab issues
may be a better plan.

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

GitLab does support hooks but it does so through this web interface which
is a bit awkward.  Basically, we'd just need a bit of Python CGI code
running in a little web server somewhere.  Shouldn't be all that hard to
set up but Daniel would likely need to be involved.

> > 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.
>
> We don't document this for email based reviews, but people seem to
> figure it out. :)
>
> Is this more of a concern because they might not know that pushing the
> same branch updates the MR?
>

I think it's worth explicitly documenting.  Many people who are used to the
MR workflow are also used to having very sloppy branches because they
assume that only the final merge matters.  This is why GitLab and GitHub
both have an option for "squash before merge". :-(

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20181128/09775aa9/attachment.html>