Re: Continuing the Gitlab experiment: Single-patch PRs for gitlab

[George Dunlap <George.Dunlap@xxxxxxxxxx>]

> On Sep 4, 2020, at 11:20 AM, Jan Beulich <jbeulich@xxxxxxxx> wrote:
> 
> On 04.09.2020 11:54, George Dunlap wrote:
>> At the community call last month as well as this, we discussed whether to 
>> continue the “Gitlab experiment”.  It was generally agreed that reviewing 
>> Juergen’s long series was fairly sub-optimal, and that email was more suited 
>> to that sort of series.
>> 
>> That said, there was general agreement that requiring all patches to go 
>> through email was going to limit contribution, particularly of one-off 
>> “drive-by” contributions.  As such, it was proposed that we consider 
>> allowing both Gitlab PRs, and email: that for one-off or short series, 
>> Gitlab PRs would be accepted, but that for longer series and/or longer term 
>> contributors, we would encourage people to switch to patchbombing the 
>> mailing list.
>> 
>> We decided to continue the “Gitlab Experiment”, but with short PRs.  As 
>> such, Andy Cooper has posted two PRs:
>> 
>> https://gitlab.com/xen-project/xen/-/merge_requests/2
> 
> This looks to be confusing, to me at least. Following this link I
> can't see the actual change directly. Following either of the links
> after "Request to merge" gives a 404 error (after gitlab not being
> able to sign me in via Google, but then being able to sign me in
> via github) on both
> 
> https://gitlab.com/xen-project/people/andyhhp/xen
> https://gitlab.com/xen-project/people/andyhhp/xen/-/tree/xen-pv-segbase
> 
> There's also an endlessly circling kind-of-icon next to "Checking
> pipeline status", indicating to me the page tries to load some
> information, but can't quite complete doing so.

It sounds like we could use a “Gitlab for git-send-email users” guide. :-)

I haven’t used a PR system extensively, but it seems like for most PR-based 
systems there’s less an emphasis on reviewing each individual commit, and more 
of an emphasis on seeing the effect of all the commits together.  That culture 
affects how the tools are designed.

So if you go to the merge request, just under the title there are some tabs.  
“Commits” will give you a list of all the individual commits.  “Changes” will 
show you a graphical diff of all the changes in the series together.

The UI for the pipeline failure is definitely sub-optimal at the moment.  The 
best way to see the pipeline failure is to either click the red circled X, or 
the pipeline number link.  For me that pretty quickly shows up a list of the CI 
builds and tests, with the failed ones highlighted in red.

One other sub-optimal thing at the moment is that the testing seems to be done 
off of Andy’s tree; so if you click the pipeline, you end up on a different 
tree, and suddenly the “merge requests” tab on the left doesn’t show any merge 
requests any more; you have to click around to get back to the main tree.  
Presumably that’s something we could change.

> I also wonder how one is to become aware of pending merge requests.
> For the ones here, your mail was the only indication so far that
> they existed. I hope the answer to this is not going to be to poll
> gitlab.com. I'm sorry if I'm making newbie mistakes or assumptions
> here, but as far as gitlab goes I'm afraid I am.
> 
> As it stands I'm afraid I'll be able to see what is proposed to be
> committed (and afaics also approved already) only when it hits the
> staging tree.

Well one simple thing to do is at the top of the XenProject, next to the “Star” 
and “Fork” buttons, there’s a bell icon with a dropdown menu.  You can click 
the dropdown and select “custom”.  I’ve just set it to email me when there’s a 
new merge request.

If the volume of patches increases (or if filtering becomes more of an issue), 
then we might add a bot to look at the contents of the patch and tag the 
appropriate people (perhaps extending MAINTAINERS to include gitlab usernames 
or something).

It’s certainly different, but I think it’s worth giving a good try before we 
decide to reject it.

 -George