Migrating Wayland & Weston to GitLab

[Daniel Stone]

Hi Jonas,

On 8 May 2018 at 11:00, Jonas Ådahl <jadahl at gmail.com> wrote:
> On Mon, May 07, 2018 at 04:59:49PM +0100, Daniel Stone wrote:
>> My proposal is that as of the repo migration, we trial GitLab merge
>> requests on a couple of patchsets where we'd expect to see the need
>> for complex structured comments and multiple revisions. These
>> revisions should probably be shadow-posted to the list, to make sure
>> we didn't miss anything.
>>
>> Once we'd been through a couple of rounds and we had a general
>> consensus amongst reviewers that we had a good workflow we were happy
>> to use, we would add that to CONTRIBUTING.md as well as README,
>> informing would-be contributors that we now preferred to receive
>> patches through GitLab, though would continue to accept patches
>> through the list for a short time (informing those who did submit via
>> the list that they should move to GitLab). Once the next release had
>> passed, we would stop doing that, and have GitLab MRs as our sole
>> point of code review.
>>
>>
>>
>> So - thoughts?
>
> First: Yay, Gitlab!
>
> Now some questions/concerns.
>
> It is common that a patch series gets per-patch reviews, each patch
> may receive multiple reviews, and we keep track of that by adding the
> Reviewed-by/Acked-by tags. How are we suppose to keep doing that with
> Gitlab?

It's a good question.

> There are two explicit issues related to reviewing:
>
> How do we keep track of who reviewed, acked or tested what patch? As far
> as I know, there is no such UI in Gitlab (yet?) for that, and also AFAIK
> no way to comment on individual commits in a merge request.
>
> The other issue is how do we "save" the reviewed-ness. The only way I
> know will work is to require the contributor to click the
> allow-maintainer-edits, then having the patch merger fetch the branch,
> rewrite the branch by adding Reviewed-by:s, pushing back the branch
> overwriting the one in the merge request, then clicking the merge
> button. All that is a bit cumbersome, especially when there is a green
> merge button waiting to be clicked.
>
> The alternative is to just stop using commit message tags, relying on
> digging out the merge request and see who commented what, which would be
> a bit unfortunate I think. It'd also loose the acknowledgements in the
> git log however, which is indeed a loss.
>
> Then there is the linear history vs merge commits. Gitlab supports both,
> with requirement for linear history being that the contributor clicks the
> allow-maintainer-edits, which makes it possible to rebase the merge
> request branch before merging. I'm not sure it's possible to make it
> checked by default however, which might be a bit unfortunate.

All good questions, and I don't really have an opinion either way.

We could do what we do now, which is rely on people to type out
explicit tags into a comment box, and copy & paste those across. Or we
could infer those from more free-form review comments.

Rather than leaning heavily on the big green button, we could pull the
commits, rebase and add the tags ourselves, then just directly push.
But I don't think this closes the merge request?

I think what I would prefer most is sensible free-form comments on
GitLab, which maintainers could then use at their discretion to decide
whether or not to push from. But in the short term, maybe a decent
compromise solution is:
  * people push to GitLab and open a MR
  * the MR is reviewed, revised, etc etc
  * as developers are comfortable with it, they add R-b/A-b tags
explicitly, as we do now
  * rather than land with the big green button, we write some
exceedingly lame script (maybe based off
https://github.com/zaquestion/lab) which scrapes the MR for tags,
rewrites commit messages to apply them, adds a link to the MR to every
commit, pushes and then closes the MR

I don't have a great answer for per-commit tags to be honest, but
again, since this is something we now do manually, maybe we could just
keep on doing that.

Cheers,
Daniel