From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM10-BN7-obe.outbound.protection.outlook.com (NAM10-BN7-obe.outbound.protection.outlook.com [40.107.92.121]) by mx.groups.io with SMTP id smtpd.web12.15927.1589557421031927102 for ; Fri, 15 May 2020 08:43:41 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=JXr+FE8d; spf=pass (domain: microsoft.com, ip: 40.107.92.121, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=S36lsrK+9uj06YKum02F92UWA4aMyOsZGwk/cqtU1aYxiGguuPZUqMEz+RCTikgnXQ3PB5rdvy8f2o3yRXV4BPQMzGpHbLAa9r3ITsgfDsgrpPjn/qbekXvHXGNY+CTSfa867upTRVBy7vnmixZukN7ZEHridW+1teu4EaJh9rsL41uuaH9fU5VTmPV797/7JgZU7KViOg+7mVXJt11BcmIU/ETm4J+HXYj48M8W+f9XRnKeGW7dZooW3RmNjavV912l0JrE264ATFkOyBMHbSfYIXSqJjGiWS3j8M99KWEPzbQDZEIU++n3vbWoKY814AYoUw3L2BvYtRl3NbSDiQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=rrfxDWtqwmscp2CJ7BEgzYUpSNhNuFKTpgRM7zDzEHg=; b=MvihNvHbi2rxFvzgOAar3FFo6C7aPlgkXGvXQjaZudX3xWK1EA0iV9ajqm1j46yslD9vQBf/BRMMVLxLblVmgEGmBxGu4a0nboSjkAM4zA6IwNd2YzH9I4fTQvW86EvZe/GbNCIiNFMJc5ve5uBt2k5sBJni/84qNi1i7syn4pcOAkMKBPjmaat0uNhx5AxJVEveigNtK1nwlL32vJcMvumDdO3Oqy8dCfPYy2W/vgTt26t8DWGk7l82hxmfcxPXDDUa8+CvDLaRo2B3Xmfcsu10VUqzjRGj/CHiwpJaWxOQU963muJ80W7hRlA6m0Jbzr/6HpyiL9b8qRDFrKzKtQ== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=microsoft.com; dmarc=pass action=none header.from=microsoft.com; dkim=pass header.d=microsoft.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=rrfxDWtqwmscp2CJ7BEgzYUpSNhNuFKTpgRM7zDzEHg=; b=JXr+FE8dZ5OM8RaTKGbHnpTg5Q9F2Cv4DrdPqgpYRcpmhqTbuxWgQQO2rS1mYMlQbWeXd8vZxkN977EzwMiQzH3fCWiNMlPEOxDOY8C4LN0GNVyXpSWPeHSNjMbVdjBoJPqk5/qGHalsI1kNUew+sudJsIPqfbYkfxcLyg43m24= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR21MB0136.namprd21.prod.outlook.com (2603:10b6:903:b2::18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.2; Fri, 15 May 2020 15:43:39 +0000 Received: from CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::9918:8742:bbe7:84e8]) by CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::9918:8742:bbe7:84e8%14]) with mapi id 15.20.3021.002; Fri, 15 May 2020 15:43:39 +0000 From: "Bret Barkelew" To: Laszlo Ersek , "rfc@edk2.groups.io" , "devel@edk2.groups.io" , "Kinney, Michael D" Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process Thread-Topic: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process Thread-Index: AdYlrbT8bFM60bvPQMeeqo8QbNJ7HwCHhx4AAAELN4UAAPT4UACYjp5FAAgLXZAAB0/4ZgAJNVyAAA2YA6c= Date: Fri, 15 May 2020 15:43:39 +0000 Message-ID: References: <8389d6a6-aaf5-3c0e-904f-84f814c9d385@redhat.com> , In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: msip_labels: MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Enabled=True;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SiteId=72f988bf-86f1-41af-91ab-2d7cd011db47;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_SetDate=2020-05-15T15:37:04.7120752Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: redhat.com; dkim=none (message not signed) header.d=none;redhat.com; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [174.21.74.235] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: 3072ac64-466c-480b-bc3b-08d7f8e6bd1b x-ms-traffictypediagnostic: CY4PR21MB0136: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 04041A2886 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: yUUTLqxkAxiKCCXE534jTgbUg60m7AJQIsw6AgTt5hzynipvBF1XUxP9q9m9l01Tlfm3Jjdr5HoWua35DEWyeRrZwAK9p9cuzccptG0p/t37nMc5vPkcSeUCIC11m939RTUE+B3bJNTipFh66KHL3OoNsLDJxXOpbQ2jas2lhNVA24ug55JBgtodxYKJVSCwJ06rBsHiiyFS9BTTRwOsBioX1HvLUNLktjHRSWvqZZMKZrVz5S/Em7iCgAKVVzYpsjoEEZY1D8s1CMDEYaDHa8AmKq7bXDACJ/9PbQVlEf9WTratk+bc74mic6lpZNSKniNbFRsZIJLUP/Qa/DmBbA== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CY4PR21MB0743.namprd21.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:(4636009)(366004)(346002)(136003)(396003)(376002)(39860400002)(82950400001)(71200400001)(86362001)(10290500003)(5660300002)(7696005)(2906002)(33656002)(6506007)(53546011)(9686003)(52536014)(55016002)(186003)(26005)(66476007)(8676002)(316002)(8990500004)(82960400001)(478600001)(76116006)(8936002)(66446008)(64756008)(66556008)(91956017)(66946007)(110136005);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: sb39cWNgdMw2HOgGKG5QiowRYhee8Bd8grFkCOLClFbfjLr9DDxRlBQsu2sxVKd9Bdw8EodfeBbGSSuYEa8GCIQq+3LhGz+oH+iSx4bTc4/jNgM4gWTOdadx4oZR3xwpFaYsOXNxh8lGo3JnBfdThvnf5vNnpEtr/FQicWtHZOPDkvwiVewuMSAYO6mbSenciQrYXRwtSzgsdXfAWVQ8mSBaGJB+gLrsp9H8owivwvO5vwfj6RA6pLRSifrJb0vz00gzGTR+n5zgzcYAaRBHONUFao/LLwF/TLFLCc8DLg7u+ZEGh2pWBQfbjGsI5jTeCB8bPsOUxlMDo8tpMmv52kCkznz/af4vJ2GxZW30JQ9VkS086lHk+RNRu8cCVUcYXS1snxNBm3TnOIU3FjquGrzntjoYCzkRvZbOLfM43mi6MR1M3hzs1xvPu/zHG/GfF03zztpP/aOTcEdx105FDDIeGcJBYuawr+nmmlF/zfCg71fuQesRACWg8VHL0mdy x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: 3072ac64-466c-480b-bc3b-08d7f8e6bd1b X-MS-Exchange-CrossTenant-originalarrivaltime: 15 May 2020 15:43:39.4182 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 72f988bf-86f1-41af-91ab-2d7cd011db47 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: Ota4CTofS6dPoQ48UeLoc+WEa3mibsPgx06oudbCfzxrWG6zDgonNl0UcFYAoXRTRiw5Pv5hVu6nU16IHuyaUA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR21MB0136 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CY4PR21MB07433D82F434DDC03E58BBACEFBD0CY4PR21MB0743namp_" --_000_CY4PR21MB07433D82F434DDC03E58BBACEFBD0CY4PR21MB0743namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable I agree with some of your points, but I don=92t believe that this calls for= dependencies at all. If a PR can pass CI with the changes, it=92s functionally unordered. And if a PR can=92t, 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=92s very good at precise histor= y management. If a given PR requires that level of control, those tools wil= l 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=92s worth, this is all posturing on my part. I intend =96 and, = indeed, am eager to =96 follow the process that we=92ve been helping Mike t= o set up. - Bret From: Laszlo Ersek Sent: Friday, May 15, 2020 2:08 AM To: rfc@edk2.groups.io; Bret Barkelew; devel@edk2.groups.io; Kinney, Michael D Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request bas= ed 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=E2=80=99re now verg= ing > 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 --_000_CY4PR21MB07433D82F434DDC03E58BBACEFBD0CY4PR21MB0743namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

I agree with some of your points, but I don=92t beli= eve that this calls for dependencies at all.

If a PR can pass CI with the changes, it=92s functio= nally unordered.

And if  a PR can=92t, it has to wait until the = PRs that can are in.

 

This also allows the group to focus on getting one t= hing done at a time.

 

I use rebase all the time and agree that it=92s very= good at precise history management. If a given PR requires that level of c= ontrol, those tools will always be there.

 

But just as you say that the simple should not precl= ude the difficult, the difficult 5% should not needlessly complicated the s= imple 95%.

 

For what it=92s worth, this is all posturing on my p= art. I intend =96 and, indeed, am eager to =96 follow the process that we= =92ve been helping Mike to set up.

 

- Bret

 

From: Laszlo Ersek
Sent: Friday, May 15, 2020 2:08 AM
To: rfc@edk2.groups.io; Bret Barkelew; devel@edk2.group= s.io; Kinney, Michael D
Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Requ= est based Code Review Process

 

On 05/15/20 06:49, Br= et 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<= br> > 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=E2=80=99re now v= erging
> 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&= quot; 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
            designat= ed MdeModulePkg review), on the day of posting; no
            other fe= edback 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 t= ruly), 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 buil= d
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

 

--_000_CY4PR21MB07433D82F434DDC03E58BBACEFBD0CY4PR21MB0743namp_--