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

[Eric Engestrom]

On Thursday, 2018-11-29 10:11:22 +0100, Erik Faye-Lund wrote:
> 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.

GitLab recently changed the way commit messages are shown, which should
help with this. The way it used to show them (and GitHub still does)
basically means that the individual commits have no meaning and this was
really annoying, but I think GitLab fixed this now.

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

(unfinished sentence?)

I don't really have experience with that on GitLab, so I can't comment.

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

Agreed, I forgot to mention earlier but you right, we need a bot that
closes MRs after 3 months with a message along the lines of:
> This has been auto-closed due to inactivity; if you think it should
> still be opened, please re-open with a comment.

This should get rid of most of the forgotten MRs.

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

Agreed on all of the above :)