Proposed changes to merge request workflow

Wed Oct 9 11:24:36 UTC 2019

Becoming a recognized reviewer before starting writing code feels 
perverse for me.

Linux kernel writes in 
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#select-the-recipients-for-your-patch 
as follows. (Note that patches are submitted by sending emails, read 
accordingly)

 > You should always copy the appropriate subsystem maintainer(s) on any 
patch to code that they maintain; look through the MAINTAINERS file and 
the source code revision history to see who those maintainers are. The 
script scripts/get_maintainer.pl can be very useful at this step. If you 
cannot find a maintainer for the subsystem you are working on, Andrew 
Morton (akpm at linux-foundation.org) serves as a maintainer of last resort.

GHC already has CODEOWNERS file, and there could be a person of last 
resort. There should be more names there though, e.g. /libraries/base/ 
should have the whole CLC, not only HVR; template-haskell could have 
Ryan Scott, etc. As the first commit message of CODEOWNERS says:

 > GitLab uses this file to suggest reviewers based upon the files that 
a Merge Request touches.

Kernel guidelines also have a section about trivial patches. Thing to 
learn, there is a light way to get trivial patches in, but what's a 
trivial patch is objectively defined. An actual trivial patch monkey is 
a real person, but to my understanding it's a circulating role.

 > For small patches you may want to CC the Trivial Patch Monkey 
trivial at kernel.org which collects “trivial” patches. Have a look into 
the MAINTAINERS file for its current manager.
 >
 > Trivial patches must qualify for one of the following rules:
 >
 > - Spelling fixes in documentation
 > - Spelling fixes for errors which could break grep(1)
 > - Warning fixes (cluttering with useless warnings is bad)
 > - Compilation fixes (only if they are actually correct)
 > - Runtime fixes (only if they actually fix things)
 > - Removing use of deprecated functions/macros
 > - Contact detail and documentation fixes
 > - Non-portable code replaced by portable code (even in arch-specific, 
since people copy, as long as it’s trivial)
 > - Any fix by the author/maintainer of the file (ie. patch monkey in 
re-transmission mode)

- Oleg

On 9.10.2019 13.31, Simon Peyton Jones via ghc-devs wrote:
> |  >  Make it clear that it is the contributor's responsibility to identify
> |  reviewers for their merge requests.
> |
> |  Asking for reviews is one of the most frustrating parts of
> |  contributing patches, even if you know who to ask! So I think the
> |  maintainer's should be responsible for finding suitable and willing
> |  reviewers.
>
> It is true that it's hard to find reviewers. But if it's hard for the author it is also hard for the maintainers.  A patch is a service that an author is offering, which is great.   But every patch is owed, as a matter of right, suitable and willing reviewers, the patch is /also/ a blank cheque that any author can write, but it's up to someone else to pay.   That's not good either.  No author has an unlimited call on the time of other volunteers, and I don't think any author truly expects that.
>
> It's an informal gift economy.  I review your patches (a) because I have learned that you have good judgement and write good code (b) because I want the bug that you are fixing to be fixed and (c) because you give me all sorts of helpful feedback about my patches, or otherwise contribute to the community in constructive ways.
>
> That may make it hard for /new/ authors to get started.  Being an assiduous reviewer is an excellent plan, because it gets you into GHC's code base, guided by someone else's work; and it earns you all those good-contributor points.  But even then it may be hard.  So I think it's absolutely reasonable for authors to ask for help in finding reviewers.
>
> But simply saying that it's "the maintainers" responsibility to find reviewers goes much too far in the other direction, IMHO.
>
> Perhaps we should articulate some of this thinking.
>
> Simon
>
> |  -----Original Message-----
> |  From: ghc-devs <ghc-devs-bounces at haskell.org> On Behalf Of Matthew
> |  Pickering
> |  Sent: 09 October 2019 11:18
> |  To: Ben Gamari <ben at well-typed.com>
> |  Cc: ghc-devs at haskell.org
> |  Subject: Re: Proposed changes to merge request workflow
> |
> |  Sounds good in principal but I object to
> |
> |  >  Make it clear that it is the contributor's responsibility to identify
> |  reviewers for their merge requests.
> |
> |  Asking for reviews is one of the most frustrating parts of
> |  contributing patches, even if you know who to ask! So I think the
> |  maintainer's should be responsible for finding suitable and willing
> |  reviewers.
> |
> |  Cheers,
> |
> |  Matt
> |
> |  On Tue, Oct 8, 2019 at 7:17 PM Ben Gamari <ben at well-typed.com> wrote:
> |  >
> |  > tl;dr. I would like feedback on a few proposed changes [1] to our merge
> |  >        request workflow.
> |  >
> |  >
> |  > Hello everyone,
> |  >
> |  > Over the past six months I have been monitoring the operation of our
> |  > merge request workflow, which arose rather organically in the wake of
> |  > the initial move to GitLab. While it works reasonably well, there is
> |  > clearly room for improvement:
> |  >
> |  >   * we have no formal way to track the status of in-flight merge
> |  >     requests (e.g. for authors to mark an MR as ready for review or
> |  >     reviewers to mark work as ready for merge)
> |  >
> |  >   * merge requests still at times languish without review
> |  >
> |  >   * the backport protocol is somewhat error prone and requires a great
> |  >     deal of attention to ensure that patches don't slip through the
> |  >     cracks
> |  >
> |  >   * there is no technical mechanism to prevent that under-reviewed
> |  >     patches from being merged (either intentionally or otherwise) to
> |  >     `master`
> |  >
> |  > To address this I propose [1] a few changes to our workflow:
> |  >
> |  >   1. Define explicit phases of the merge request lifecycle,
> |  >      systematically identified with labels. This will help to make it
> |  >      clear who is responsible for a merge request at every stage of its
> |  >      lifecycle.
> |  >
> |  >   2. Make it clear that it is the contributor's responsibility to
> |  >      identify reviewers for their merge requests.
> |  >
> |  >   3. Institute a final pre-merge sanity check to ensure that
> |  >      patches are adequately reviewed, documented, tested, and have had
> |  >      their ticket and MR metadata updated.
> |  >
> |  > Note that this is merely a proposal; I am actively seeking input from
> |  > the developer community. Do let me know what you think.
> |  >
> |  > Cheers,
> |  >
> |  > - Ben
> |  >
> |  >
> |  > [1]
> |  https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.h
> |  askell.org%2Fghc%2Fghc%2Fwikis%2Fproposals%2Fmerge-request-
> |  workflow&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f
> |  08d74ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6370621311033130
> |  52&sdata=SxBADAuF%2FvGzduaytetUzIxGr8lC%2BjTX2eCLNEoOCkQ%3D&reserv
> |  ed=0
> |  > _______________________________________________
> |  > ghc-devs mailing list
> |  > ghc-devs at haskell.org
> |  >
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103313052&a
> |  mp;sdata=T%2FyLoRH9BTIVPxMzF0%2BAa3c20qCBkhvQrp53FtROz40%3D&reserved=0
> |  _______________________________________________
> |  ghc-devs mailing list
> |  ghc-devs at haskell.org
> |  https://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fmail.hask
> |  ell.org%2Fcgi-bin%2Fmailman%2Flistinfo%2Fghc-
> |  devs&data=02%7C01%7Csimonpj%40microsoft.com%7Cd1199fd308b442cf744f08d7
> |  4ca2074b%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637062131103323047&a
> |  mp;sdata=IwsIP3P6W5qtsLxfePbYOWTXdPLttNMLHWXkuTtVWgI%3D&reserved=0
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs