[edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

[Philippe Mathieu-Daudé]

Hi Bret,

On 5/15/20 5:43 PM, Bret Barkelew via groups.io wrote:
> I agree with some of your points, but I don’t believe that this calls for dependencies at all.

Which points are you disagreeing?

> If a PR can pass CI with the changes, it’s functionally unordered.
> And if  a PR can’t, it has to wait until the PRs that can are in.
> 
> This also allows the group to focus on getting one thing done at a time.
> 
> I use rebase all the time and agree that it’s very good at precise history management. If a given PR requires that level of control, those tools will always be there.
> 
> But just as you say that the simple should not preclude the difficult, the difficult 5% should not needlessly complicated the simple 95%.
> 
> For what it’s worth, this is all posturing on my part. I intend – and, indeed, am eager to – follow the process that we’ve been helping Mike to set up.
> 
> - Bret
> 
> From: Laszlo Ersek<mailto:lersek at redhat.com>
> Sent: Friday, May 15, 2020 2:08 AM
> To: rfc at edk2.groups.io<mailto:rfc at edk2.groups.io>; Bret Barkelew<mailto:Bret.Barkelew at microsoft.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney at intel.com>
> Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process
> 
> On 05/15/20 06:49, Bret Barkelew via groups.io wrote:
> 
>> I would far prefer the approach of individual PRs for commits to
>> allow for the squash flexibility (and is the strategy I think I would
>> pursue with my PRs). For example, the VarPol PR would be broken up
>> into 9 PRs for each final commit, and we can get them in one by one.
>> Ideally, each one would be a small back and forth and then in. If it
>> had been done that way to begin with, it would be over in a week and
>> a half or so, rather than the multiple months that we’re now verging
>> on.
> 
> This differs extremely from how we've been working on edk2-devel (or
> from how any git-based project works that I've ever been involved with).
> And I think the above workflow is out of scope, for migrating the edk2
> process to github.
> 
> Again, the structuring of a patch series is a primary trait. Iterating
> only on individual patches does not allow for the reordering /
> restructuring of the patch series (dropping patches, reordering patches,
> inserting patches, moving hunks between patches).
> 
> It's common that the necessity to revise an earlier patch emerges while
> reworking a later patch. For instance, the git-rebase(1) manual
> dedicates a separate section to "splitting commits".
> 
> In the initial evaluation of "web forges", Phabricator was one of the
> "contestants". Phabricator didn't support the "patch series" concept at
> all, it only supported review requests for individual patches, and it
> supported setting up dependencies between them. So, for example, a
> 27-patch series would require 27 submissions and 26 dependencies.
> 
> Lacking support for the patch series concept was an immediate deal
> breaker with Phabricator.
> 
> The longest patch series I've ever submitted to edk2-devel had 58
> patches. It was SMM enablement for OVMF. It went from v1 to v5 (v5 was
> merged), and the patch count varied significantly:
> 
> v1: 58 patches (25 Jul 2015)
> v2: 41 patches ( 9 Oct 2015)
> v3: 52 patches (15 Oct 2015)
> v4: 41 patches ( 3 Nov 2015)
> v5: 33 patches (27 Nov 2015)
> 
> (The significant drop in the patch count was due to Mike Kinney open
> sourcing and upstreaming the *real* PiSmmCpuDxeSmm driver (which was
> huge work in its own right), allowing me to drop the Quark-originated
> 32-bit-only PiSmmCpuDxeSmm variant, from my series.)
> 
> The contribution process should make difficult things possible, even if
> that complicates simple things somewhat. A process that makes simple
> things simple and difficult things impossible is useless. This is what
> the Instagram generation seems to be missing.
> 
> 
> I don't know why the VariablePolicy work took months. I can see the
> following threads on the list:
> 
> * [edk2-devel] [PATCH v1 0/9] Add the VariablePolicy feature
>    Fri, 10 Apr 2020 11:36:01 -0700
> 
> * [edk2-devel] [PATCH v2 00/12] Add the VariablePolicy feature
>    Mon, 11 May 2020 23:46:23 -0700
> 
> I have two sets of comments:
> 
> (1) It's difficult to tell in retrospect (because the series seem to
> have been posted with somewhat problematic threading), but the delay
> apparently came from multiple sources.
> 
> (1a) Review was slow and spotty.

IIUC it is easier for the "Instagram generation" to write a GitHub
plugin which ping an unmerged pullrequest for them, rather than tracking
their WiP and send a "ping" via an email client.

That reminds me of the Prophet tool:

Prophet: The first generate-and-validate tool that uses machine learning
techniques to learn useful knowledge from past human patches to
recognize correct patches. It is evaluated on the same benchmark as
GenProg and generate correct patches (i.e., equivalent to human patches)
for 18 out of 69 cases.

https://groups.csail.mit.edu/pac/patchgen/
https://en.wikipedia.org/wiki/Automatic_bug_fixing#C [8]

Use it as source, combined with a fuzzer that open GH pull-requests, and 
see if a patch get merged... /s

> 
> The v1 blurb received some comments in the first week after it was
> posted. But the rest of the v1 series (the actual patches) received
> feedback like this:
> 
> - v1 1/9: no feedback
> - v1 2/9: 12 days after posting
> - v1 3/9: 16 days after posting
> - v1 4/9: no feedback
> - v1 5/9: no feedback
> - v1 6/9: no feedback
> - v1 7/9: no feedback
> - v1 8/9: no feedback
> - v1 9/9: no feedback
> 
> (1b) There was also quite some time between the last response in the v1
> thread (Apr 26th, as far as I can see), and the posting of the v2 series
> (May 11th).
> 
> (1c) The v2 blurb got almost immediate, and numerous feedback (on the
> day of posting, and the day after). Regarding the individual patches,
> they didn't fare too well:
> 
> - v2 01/12: superficial comment on the day of posting from me (not a
>              designated MdeModulePkg review), on the day of posting; no
>              other feedback thus far
> - v2 02/12: ditto
> - v2 03/12: no feedback
> - v2 04/12: superficial (coding style) comments on the day of posting
> - v2 05/12: no feedback
> - v2 06/12: no feedback
> - v2 07/12: no feedback
> - v2 08/12: no feedback
> - v2 09/12: no feedback
> - v2 10/12: no feedback
> - v2 11/12: reasonably in-depth review from responsible co-maintainer
>              (yours truly), on the day of posting
> - v2 12/12: no feedback
> 
> In total, I don't think the current process takes the blame for the
> delay. If reviewers don't care (or have no time) now, that problem will
> not change with the transition to github.com.
> 
> 
> (2) The VariablePolicy series is actually a good example that patch
> series restructuring is important.
> 
> (2a) The patch count went from 9 (in v1) to 12 (in v2).
> 
> (2b) And under v2, Liming still pointed out: "To keep each commit build
> pass, the patch set should first add new library instance, then add the
> library instance into each platform DSC, last update Variable driver to
> consume new library instance."
> 
> Furthermore, I requested enabling the feature in ArmVirtPkg too, and
> maybe (based on owner feedback) UefiPayloadPkg.
> 
> Thus, the v2->v3 update will most likely bring about both patch order
> changes, and an increased patch count.
> 
> Thanks
> Laszlo
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#59736): https://edk2.groups.io/g/devel/message/59736
Mute This Topic: https://groups.io/mt/74289183/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-