On Mon, May 07, 2018 at 04:59:49PM +0100, Daniel Stone wrote: > Hi all, ... snip ...
> > Code review > ----------------- > > [admin hat on] > > GitLab also provides code reviews through merge requests[3], which are > like GitHub's pull requests (push to a branch or separate repo, create > a request for someone to review and merge that code), but much more > attuned to our workflows. Specifically, GitHub is geared towards > chronological commit streams (initial code, fix, fix, fix, fix, fix, > more stuff, more fixes) which ultimately get squashed into one - which > was replicated by Phabricator. GitLab does allow for the merge-ordered > microcommits we have on the list today, including allowing you to > rebase them before submitting new versions, and also not squashing the > results. > > More usefully, it also allows you to create these with 'git push' then > following the URL it offers you to create a MR. Being a real git > branch allows reviewers to pull the pristine tree as the submitter had > (without lossily running through patch files), and also for CI > pipelines to be run on those branches so they can see problems without > needing the reviewer as a proxy to 'make distcheck'. > > Comments on GitLab MRs can either be placed as a general comment on > the MR, or directly associated with lines of code. These comments can > either be comments (general chatter), or marked as an 'issue', which > must be explicitly marked as resolved in order to move on. This makes > it more difficult to forget review comments in subsequent revisions. > > > [admin hat off, contributor hat on] > > Every time someone comes on to #wayland saying that they want to > submit a patch series but can't figure out how to use git-send-email, > it kills me. Every time I look at the state of Patchwork and realise > that it's a wasteland because the commit -> Patchwork hooks are > necessarily lossy, I despair a bit. That's if it even correctly > identifies and groups the patches in the first place, which it doesn't > despite two years of development specifically aimed at having it work > well for dri-devel@ and intel-gfx@. > > Not having code review hooked into issue tracking means that even > after we've demanded our contributors jump through the git-send-email > hoops, their contributions can disappear into the void. I think having > it hooked up to issue tracking, with assignable labels and target > milestones, with discussions people can actually follow (rather than > Mailman archives, where any discussion spanning a month boundary is > split in two), would really lower the barrier to contributing. > > We can also put a CONTRIBUTING.md file which is shown to people when > they create merge requests, outlining how we review and what we're > looking for. CI means that we can pick up the mechanical failures > early, rather than waiting for a human to come by, run the tests > themselves, and inform someone who may no longer have time or interest > at that point. > > Something I've been told anecdotally by people either contributing to > or using wlc/wlroots/etc, is that one of the reasons they used that > over libweston is because our archaic setup means it's pointlessly > difficult to communicate with us. They've got a point. > > 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? 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. Jonas > > Cheers, > Daniel > > [0]: https://gitlab.freedesktop.org/freedesktop/freedesktop/wikis/home > [1]: https://wiki.gnome.org/Initiatives/DevelopmentInfrastructure > [2]: https://docs.gitlab.com/ce/workflow/todos.html > [3]: https://docs.gitlab.com/ce/user/project/merge_requests/index.html > _______________________________________________ > wayland-devel mailing list > wayland-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/wayland-devel _______________________________________________ wayland-devel mailing list wayland-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/wayland-devel
