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

[Erik Faye-Lund]

On Tue, 2018-11-27 at 17:13 -0800, 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>

In lack of a better thread to discuss this, I'm going to hijack this
one slightly. I hope I'm not too disturbing.

For the better part of the last 4 years, I've been working on a large-
ish (commercial, but open source) project that used GitHub PRs for
contributions. It's a great way of working, for all the reasons we've
already been through:
- Structured reviews
- Easy to get an overview what's happening
- Prevent most bad merges due to CI

But there's a few down-sides as well, compared to mailing list
discussions, and these are the reasons I wanted to speak up. I'm a
strong supporter for MRs, by the way. I just want these issues talked
about. Also note that my experience here comes from GitHub, and not
specifically from GitLab, but I suspect at least some of them apply
here as well:

1. There's usually no reasonable way of reviewing commit messages. You
can add general comments on the development history, but it gets
awkward to navigate between tabs to copy something to quote and stuff.
In e-mail based reviews, you just click reply.

2. It's often difficult to see comments on intermediate commits. At
least on GitHub, intermediate changes gets collapsed as "out-of-date",
because GitHub doesn't seem to care about clean development history.
This tends to 

3. The MR list becomes something that needs to be maintained. Sometimes
there's long-lived MRs that takes months to get merged, or just simply
stall... and they all start piling up. That's fine in a mailing list,
but it quickly gets cumbersome in a MR-based work-flow, as old MRs take
up space in UIs, and makes it harder to see what's going on. So at some
point, stale MRs should be closed IMO. This is going to need someone to
either do it, or to maintain some bot to do it. And some developers
tends to get angry when you close a MR after three months of inactivity
just because nobody wanted to review it.

Out of these, #1 and #2 are my biggest nusances with it, really. The
PR/MR workflow tends to be all about the end-result, and not so much
about the intermediate steps or the "story" they tell. And the UIs
seems to reward such development rather than "the story matters"-
development.

I'm a bit worried that we'd start gradually changing how we developed;
I saw this happen to some degree in my past experience. But at the same
time, I do think that the benefits greatly outweigh the disadvantages.