Re: On the need to move to a merge request workflow

On Fri, Mar 27, 2020 at 02:15:01PM +0100, Peter Krempa wrote: > On Thu, Mar 26, 2020 at 15:50:20 +0000, Daniel Berrange wrote: > > On Thu, Mar 26, 2020 at 02:10:59PM +0100, Peter Krempa wrote: > > > On Fri, Mar 06, 2020 at 11:44:07 +0000, Daniel Berrange wrote: > > > > We've discussed the idea of replacing our mailing list review workflow with > > > > a merge request workflow in various places, over the last 6 months or so, > > > > > > One thing I feel the need to voice until this is taken in place is a > > > matter of personal preference: > > > > > > I severely dislike the merge request workflow and I'll be severely > > > disappointed once we switch over to it. > > > > Can you elaborate on specific things you don't like, so we can see if > > there are any options to mitigate them ? > > - it's a web page > - it's very slow on big projects and slow in general > - it's ugly, "theme" support is a joke, white background burns my > eyes > - it works badly on a portrait display > - mangles commit messages in an attempt to render them (link and > signed-off line concatenated: > https://gitlab.com/libvirt/libvirt/-/commit/e05dd1abdc3b3eeac6e12ab105e56... This looks like a bug in their code which renders the messages. It is failing to preserve a newline character when it follows a hyperlink. I'll see about reporting this as an issue. > > > - usability > - default view after opening a branch is "Files" not "commits" > - web interface doesn't really carry information about which merge > requests are new to me > > - review is terrible > - threads are only single level > - response can't be quoted (okay, they can but you have to copy and > pase what you want to quote and then select that it's a quote) > - you need to click to see the code > - comments to code are in proportional font > - a lot of useless clutter in the UI > - need to click open comments in code > - extra steps necessary to apply code locally (granted you get a > branch by default, but fetching it is not as easy as I have with > mail workflow) Overall the issues are primarily focused on the web UI, rather than the conceptual idea of the merge request workflow.

I share the same view of many of these issues, but I do think we can mitigate most of it with a terminal based tool as an option for those who don't like the web UI. > - review process favours review without actually fetching the code > locally (if you can click a button, who will actually test that > the code works?) If anything I feel the current email review makes me even less likely to actually apply & test the code locally, as it requires several tedious error prone steps, compared to being able to direct "git fetch" a ref for the merge request. The more automated testing we do the better, but either way we rely on reviewers to be diligent enough to decide when something needs some more testing. > > > Now I know you will suggest I try your 'bichon' tool for reviews as it > fixes a handful of the problems outlined above, but unfortunately that > comes with it's own set of problems since it's in infancy stage. Yes, it certainly isn't developed to where it needs to be yet, but I have started using it for real code review on libosinfo repos and found it is useful already.