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

Fri May 15 15:43:39 UTC 2020

I agree with some of your points, but I don’t believe that this calls for dependencies at all.
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.

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 (#59691): https://edk2.groups.io/g/devel/message/59691
Mute This Topic: https://groups.io/mt/74144675/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20200515/ba2aaf5c/attachment.htm>