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

[Daniel Stone]

Hi all,
Thanks for the CC. I'm on a sabbatical until mid-January; I'll be
around but not following the lists/etc as actively as before. Please
feel free to liberally CC me (on this address, not work) or poke me on
IRC if there's something I should see or could contribute to. I'll
have limited time for the next week or so, but should be able to do
more on it next month, albeit in Australian timezone.

On Wed, 28 Nov 2018 at 17:23, Dylan Baker <dylan at pnwbakers.com> wrote:
> Quoting Matt Turner (2018-11-27 19:20:09)
> > Discussion point: I think attempting to have simultaneous review in
> > two places is a recipe for wasted time. Strawman: maybe we can only
> > email the cover letter to the mailing list and include in it a link to
> > the MR?
>
> I think it's a *really* bad idea to allow both. Some people will immediately
> begin using MR/PR's only and never read the list, others will never check
> MR/PRs and only use the list. It'll leave the rest of us forced to use both. We
> should either go all in and archive the mailing list and use only gitlab, or not
> use PR/MR's IMHO.

I fully agree, for all the reasons listed here. It imposes a much
higher overhead on contributors, and means that reviewers effectively
need to work their way through both interfaces anyway. Since you don't
get stable links until you push, in order to link between the two, you
would need to submit a MR to GitLab, capture the URL, include that in
your cover letter / patches, git-send-email, then capture the message
ID / archive link and go back to GitLab to edit your cover letter and
insert the link. Not to mention that it would basically destroy the
usefulness of Patchwork overnight.

Weston has a pretty conservative development workflow, but even then
we've had near-zero pushback, and saw a pretty big leap in
contributions from people who have been sitting on patchsets for a
long time but not bothered to submit them when we had review on the
list. I was genuinely surprised at how big the uptick was.

> > > +  <li>Never use the merge button on the GitLab page to push patches
> >
> > Can we disable this in Gitlab? If the button is there, people *will*
> > accidentally press it.
>
> I think Daniel was working on getting a "rebase and push" button like github has
> so that we could use the button, CC'd.

This is already there, and can be configured per-repo. Under settings
-> merge requests, choose this as your merge strategy:
'Fast-forward merge
No merge commits are created and all merges are fast-forwarded, which
means that merging is only allowed if the branch could be
fast-forwarded.
When fast-forward merge is not possible, the user is given the option
to rebase.'

If the user selects the 'allow commits from members who can merge to
the target branch' checkbox when submitting the MR, reviewers can
themselves request a rebase. We have FF-only MRs in Weston, and when I
want to land a contribution after review, I hit rebase followed by
'merge after pipeline succeeds', which does what it says on the box,
i.e. waits until CI has finished and then automatically lands it.

There are a couple of rough edges here. First, users have to
explicitly select the checkbox for this to happen; an upstream feature
request to make this the default or even mandatory is at
https://gitlab.com/gitlab-org/gitlab-ce/issues/49323. Secondly, if
something else lands in the target branch between the rebase starting
and CI finishing (i.e. it's no longer an FF landing by the time CI
reports success), the reviewer has to request the rebase again. Jason
was discussing having GitLab busy-loop, running rebase+CI in a loop
until it could actually land. I don't have a link to hand for this
though.

> > I'm not sure if it's a custom thing or what (I can find out) but I'd
> > much prefer to automate things if we can. Just like patchwork, people
> > *will* forget to close merge requests if it's possible. (And people
> > currently *do* forget to close bugzilla bugs)
>
> Or we could just use gitlab's issue tracker, which will automatically close
> issues when a commit pushed to master has a tag like `Fixes: #1234` or
> `Closes: #1234`.

Right, and obviously if you land a MR through the web UI then it's
automatically closed.

> Personally speaking, I think that better next steps for gitlab integration are:
> - migrate from bugzilla to gitlab issues

This is currently held up by a mutual death grip: both AMD and Intel
want to be able to move or reassign issues between kernel / Mesa / X11
within Bugzilla, so have requested that nothing moves until everything
moves. I don't know whether that has to be one big flag day or whether
the driver teams would find it acceptable for all three components to
have a documented plan with a timeline on it. Intel also have some
pretty heavy scripting and tooling around Bugzilla which would need to
be modified to track GitLab instead.

>From an infrastructure point of view though, Bugzilla is getting less
and less acceptable to run as a service. It's no longer just a
horribly insecure pile of Perl, but now one which has been abandoned
upstream.

Upstream has had no commits in 3 months:
https://github.com/bugzilla/bugzilla/branches
No releases in over 9 months: https://www.bugzilla.org/news/
Mozilla is trying to spin Bugzilla out to a separate non-Mozilla
project; the last update from 5 months ago is that they were trying to
figure out where to host it: https://planet.bugzilla.org
All the development seems to be happening in a fork of Bugzilla which
is fairly ominously forked from the bugzilla.mozilla.org branch, and
even that has had no activity in 6 weeks:
https://github.com/bugzilla/harmony/commits/master

Luckily Bugzilla runs on its own VM, but I have zero confidence that
Bugzilla is at all secure anymore. After looking into it over the past
few weeks, I really want to have Bugzilla shut down sooner rather than
later.

> - convert piglit to a PR/MR workflow and see how it goes. The list for piglit
>   is barely looked at anyway.

This is a good idea, since it allows people to get more familiar with
it, developing (and documenting!) a good workflow.

Cheers,
Daniel