From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM12-BN8-obe.outbound.protection.outlook.com (NAM12-BN8-obe.outbound.protection.outlook.com [40.107.237.107]) by mx.groups.io with SMTP id smtpd.web12.4639.1589916870270881023 for ; Tue, 19 May 2020 12:34:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=eg2n330s; spf=pass (domain: microsoft.com, ip: 40.107.237.107, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=POohDpbxQ1l0ISgcrg8Imo+e/gqcpSaB6lKOQ/r0eMeBt/aVcqipSNPLesGY+OD29ECCV8oB+RnOUHIHbhxHh850sxGnQ9aGEk7orootqrzorxzuqzFsm7yiBC5ypJdFlFmriA9z/S4fD2HQK1r9PRLxQdGchBL+vXpn3p3Ih8H5b4+L4LbSrcSCw7wHN8G+ckD6a0qaRIGfCWghL8yA5OpQB2u/5OGAujPQRGjkRguZTyPPJut3/uNsuB0Y2fHXOxQ8Uo9wshamYtqTRdlLCryzbRzRVMvT33CsHfyRxoq7CawsmFcLAaM13pyXwS/+dSU7JQ5E2FGSbZSLeTDDMA== 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=aJc14JSNaRCMmyed/jYrlUW11epWjjZ8348aKmCEvyM=; b=coOaOjHyzjNZsEuU22Yi0uILGuCGrHPQIXYRPhgtFbyrx5/8m8lwNR4fpj/blbZ+l5FExtoaPYLlj62NqvdCGZeY5+4cKxfRoqGVILE+ycjY+rlyejDQETtL98VYDYqSHlMt6kLNpApmUFBGx3tNEI8lDRIBthyAWcJrS4DeTXhzp7UJ0ed54LR2wfY8WAJILjicMO+IhjbRDmmi4xVgM4ar9pYjI+WdAjRQ7O0HIO8mgolOoeWQeWe6L8a69AfXCDtNbXpOMSJODf3i9eoW++ojR2EHWRgCZZkzUVp3eM/v0I8FS8UyT/7HznT/Ak6MR39nFvfTPGS1UNj933PNgw== 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=aJc14JSNaRCMmyed/jYrlUW11epWjjZ8348aKmCEvyM=; b=eg2n330s65gZNj7fhc2OiKAuAsaPs71Bnw6X1i/59YVl8ugklRcHAY6bDOBH4Yv8T4eH/yV6cEnf6EmDTNmSoHC/IrhrGjx/PLnGe5WvycFWlRiixA2aoBzkWoN825vwl3enXegJWCBHF4ojQnYf9k/FdLSWOL2Yw/Pa0IKyXEg= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR21MB1554.namprd21.prod.outlook.com (2603:10b6:910:92::26) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3045.5; Tue, 19 May 2020 19:34:28 +0000 Received: from CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::f112:82fb:d4fd:f7dd]) by CY4PR21MB0743.namprd21.prod.outlook.com ([fe80::f112:82fb:d4fd:f7dd%12]) with mapi id 15.20.3045.005; Tue, 19 May 2020 19:34:28 +0000 From: "Bret Barkelew" To: "devel@edk2.groups.io" , "Desimone, Nathaniel L" , "spbrogan@outlook.com" , "rfc@edk2.groups.io" , "lersek@redhat.com" , "Kinney, Michael D" Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process Thread-Topic: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process Thread-Index: AQHWLgex0PJvxvOSUUG9f19bEFeuN6ivzG8t Date: Tue, 19 May 2020 19:34:27 +0000 Message-ID: References: , 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-19T19:32:34.9250949Z;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_ContentBits=0;MSIP_Label_f42aa342-8706-4288-bd11-ebb85995028c_Method=Privileged authentication-results: edk2.groups.io; dkim=none (message not signed) header.d=none;edk2.groups.io; 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: ffc292f8-edc3-43eb-4af8-08d7fc2ba53b x-ms-traffictypediagnostic: CY4PR21MB1554: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:4502; x-forefront-prvs: 040866B734 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 98/u/iUaTHM6R7r9QihCHJd+CzPQckzDc0ubTO/i2RHomjYRvaiWabve+Qxwlns4molYY2XeZnBnadqcUJ8p4o365W5vXotzUtCnibUafAXH+Iu7RtSck/fKgcnItewly/GNLNUKGadgsbjAlXYaQe4GsuHKxiA2QsV/SywKSQmy0BJV2Ov60v8+b7NsG465u1AJztsZSazBzOpvR7nJBF1tuOkLblae0UhVav+NNLOac8XZTNhCHWNXHqjPZgdJKhKTPWSjN6e/tVwaugCkYULZGpmfu4/sBsayO5cVmimGE5FEjNFT1G5+O0xljwnib4LGgpQ/55lLCsk7Ujhw0qnc7jHRteMKgfjTonn64zSQCucnPpWuW/1FYINDjNf9UAwLUNPv/8zc/BthCvE4wA== 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)(376002)(39860400002)(346002)(396003)(136003)(366004)(8990500004)(166002)(9686003)(8676002)(8936002)(66946007)(10290500003)(33656002)(66476007)(66556008)(64756008)(66446008)(76116006)(7696005)(966005)(478600001)(316002)(110136005)(86362001)(52536014)(82960400001)(82950400001)(30864003)(55016002)(5660300002)(6506007)(53546011)(186003)(26005)(2906002)(71200400001);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: p3wwiyRC47sDSm7sdjcRmtQ+7fMXPyWzcMBiOhiEaNWNstnY7k+YBHIseXjq1Zrzf/ZhoyxrOIuVr+3bDp6tqXgYSYvyH+O+w5xbyH6IzzhwYMg2YyTeotlLeB2JgTyvCrzSfO5V+paqs0OBsDdMPd8OVRpvhWmJTws7Lp7of3p8xkgkBjaqDyLlHcCHHmd9q9gwZXIakBt6aNpspi5np3h4VCkdDIIU3wti/1o14AE7QbeTRuGI7Ixh7RaX6nPm6g3WqXxxp6EIJsPejTKBRYiLHXcSuRMOYOSh7gmzea+9zvOXrukQdSpBN/EvF1ffsY3mR5g8DuGL7fbsM0lVzlK4fylMrXUEChqrRYYkrQ3wV1tE0H+f5ss38ybSgvg9Nbm9IerWsVDjJ/Fneaqu822Ef0T3h8oV5mjBcYFsxQPCujbX8oxDPBXpJDCFxnEoBG09u7LuW8Lt3G6GzKh/joODlruqi868d4Rmm1I00yY= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: ffc292f8-edc3-43eb-4af8-08d7fc2ba53b X-MS-Exchange-CrossTenant-originalarrivaltime: 19 May 2020 19:34:28.0065 (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: 9gdumsSeTUnCqSmLdAgoTz8bS33ukhChtp4TQ4P1pe/wxdlEzY8CDnNh6lOWEDGaBL4LmArVQYQiW/pEnn3q0Q== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR21MB1554 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CY4PR21MB0743BB6834B9415FBD61E6CEEFB90CY4PR21MB0743namp_" --_000_CY4PR21MB0743BB6834B9415FBD61E6CEEFB90CY4PR21MB0743namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable Nate, I believe you missed Sean=92s point. Each one of those packages should have been a separate PR. Ergo, no information would have been lost in the squash. Also, it=92s not so much that we *can=92t* learn. It=92s that we choose no= t to. Around here, it=92s a mark of prestige to not open doors with your fa= ce if it seems like there=92s a better way. Makes it easier to focus on the= work. - Bret From: Nate DeSimone via groups.io Sent: Tuesday, May 19, 2020 11:02 AM To: devel@edk2.groups.io; spbrogan@outlook.co= m; rfc@edk2.groups.io; lersek@redhat.com; Bret Barkelew; Kinney, Michael D Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based = Code Review Process Hi Sean, My recent spelling fix patch series is a good example of why this is a bad= idea actually: https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59779&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DfVz16E37%2BwW5pSgRxI45K7= nWPDlIoS0HuI8UCGmEwjY%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59780&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3D4q0lC1BSlSoQ3p0HGWwlph09= HTjgJRo4nTO2Qx59%2Fjc%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59781&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DXQVSwPMXdpDJXj9nkuvq2fen= whNt6HGGZXsJwH5Bu8E%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59782&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DkCULGBc6%2Bifcn3cnPTV1od= HI1ZUxuWQePN3POKKS3SM%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59783&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DSCOhUMdNXHIymGLaw9z3JTh%= 2Fe2BfaJaAyEC99EkG%2Fvg%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59784&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DepET6Wk30bIHQCvEDFLkeHEf= mm9tzlxRrJ%2FQAuEfQFs%3D&reserved=3D0 https://nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.g= roups.io%2Fg%2Fdevel%2Fmessage%2F59785&data=3D02%7C01%7Cbret.barkelew%4= 0microsoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7= cd011db47%7C1%7C0%7C637255081625996312&sdata=3DN8T7HjerJVvyGg94yMWjLm%2= Fw7WDdXOdby1JpOYlPeVc%3D&reserved=3D0 Notice that I split along package boundaries, because the maintainers for = each package is a different set of people. If my patch series was squashed = at merge time... how do I know who reviewed what? If the commit set is not = correct.. I tend to say so in my feedback :). The only sane way to squash t= his series would be to have a human re-write all the commit messages, which= I am against. Generally those that prefer an easily bisectable history have such prefere= nce mostly due to the usage of validators that immediately resort bisecting= as a method to root cause an issue since they tend to not understand the c= ode very well. Edk2 already has 12 years of non-bisectable history, so this= method is going to be ineffective anyway. With regard to sending squashed commits, I understand that those who are n= ew may have difficulty sending a properly formatted patch series, but frank= ly attempting to shield them from having to learn I am strongly against. I = suggest that Microsoft invest in its human capital similar to how Intel doe= s. If you cannot figure out how to send a properly formatted patch series..= . then do your work on the internal codebase (or perhaps MU.) Within the In= tel, having the skillset to contribute to TianoCore is considered a mark of= prestige, and thus needs to be earned. TLDR, I will reject squashed commits on any packages that I maintain. Thanks, Nate > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Sean > Sent: Tuesday, May 19, 2020 9:54 AM > To: rfc@edk2.groups.io; Desimone, Nathaniel L > ; lersek@redhat.com; > bret.barkelew@microsoft.com; devel@edk2.groups.io; Kinney, Michael D > > Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Revi= ew > Process > > Nate/Laszlo, > > Regarding a squash merge workflow. I agree it can be abused and we all > have seen terrible examples. But a patch series that contains 500+ file > changes isn't really much better. Just because it is broken into multip= le > commits doesn't mean it is the right set of commits. > > Anyway a squash merge workflow works amazingly well with and is > optimized for a web based review and PR processes. It allows a user to > respond to changes, fix issues, learn thru the PR process, all while kee= ping > complete track of the progression. Then once all "status" > checks and reviews are complete, it is squashed into a neat commit for > mainline, containing only the relevant data in the message. > > So, the ask is that we don't exclude squash merge workflows. Those > reviewing the PR can decide what is appropriate for the PR content > submitted. Just as you would request changes to the contents (or > ordering) of a commit in a series, if the reviewers don't agree that the= PR > contents should be in a single commit then obviously it shouldn't be > squashed to one. > > Contributions like spelling fixes, typos, minor bug fixes, documentation > additions/fixes, etc all are great examples of PRs that can easily lever= age > squash merges and this workflow significantly lowers the burden of the > contribution and review process. This workflow is also are much easier = for > casual or first time contributors. > > I don't exactly know how we would enable this but I assume we could > leverage tags or make it clear in the PR description. First step is to = get > alignment that a squash merge workflow, while not appropriate for all > contributions, is not something to be excluded. > > Thanks > Sean > > > > On 5/19/2020 12:21 AM, Nate DeSimone wrote: > > Hi All, > > > > > > > > I tend to agree with most of Laszlo's points. Specifically, that movin= g to pull > requests will not fix the fact that maintainers are usually busy people = and > don't always give feedback in a punctual manner. Like Laszlo, I would al= so > prefer that we do not squash patch series. My biggest reason for not > squashing patch series is because when you put everything into a single > commit, I have had to review commits with 500+ files changed. Opening gi= t > difftool on a commit like that is awful. > > > > > > > > However, I would like to register my general endorsement for pull requ= ests > or some other web based system of code review=85 and I don=92t have an > Instagram account by the way :) Personally, I prefer Gerrit as I use it = a lot with > coreboot and other projects. But since we are using Github for hosting, = pull > requests are an easy switch and a logical choice. My main reason for bei= ng > excited about pull requests mostly has to do with the amount of manual > effort required to be a TianoCore maintainer right now. I have set up my > email filter so that the mailing list is categorized like so: > > > > > > > > [cid:image001.png@01D62D71.502B55E0] > > > > > > > > Implementing the logic to parse the contents of emails to categorize t= hem > like this required me to define no less than 12 email filter rules in Mi= crosoft > Outlook, and I have to change my filtering logic every time I am > added/removed from a Maintainers.txt file. I=92m sure every other mainta= iner > has spent a time separately implementing filtering logic like I have. Th= is helps, > but still for every thread, I have to go and check if one of the other > maintainers has already reviewed/pushed that patch series yet, and if no= t > review/push it. If I have ] feedback on a patch series, I have to catego= rize it > as awaiting response from author and check up on it from time to time, > sometimes I ping the author directly and remind them to send a new patch > series. Implementing this state machine is a lot of manual work and it k= ind of > feels like I=92m a telephone operator in the 1950s. I greatly welcome > automation here as I am sure it will increase the number of patch series= I am > able to review per hour. > > > > > > > > Thanks, > > > > Nate > > > > > > > > -----Original Message----- > > From: rfc@edk2.groups.io On Behalf Of Laszlo > > Ersek > > Sent: Friday, May 15, 2020 2:08 AM > > To: rfc@edk2.groups.io; bret.barkelew@microsoft.com; > > devel@edk2.groups.io; Kinney, Michael D > > 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=E2=80=99re now ve= rging > > > >> 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 whil= e > reworking a later patch. For instance, the git-rebase(1) manual dedicate= s 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 patc= hes. > 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 i= f that > complicates simple things somewhat. A process that makes simple things > simple and difficult things impossible is useless. This is what the Inst= agram > 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 h= ave 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 pos= ted. > But the rest of the v1 series (the actual patches) received feedback lik= e 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 v= 1 > 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, th= ey > 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 de= lay. 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 se= ries > 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_CY4PR21MB0743BB6834B9415FBD61E6CEEFB90CY4PR21MB0743namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

Nate, I believe you missed Sean=92s point.

 

Each one of those packages should have been a separ= ate PR.

Ergo, no information would have been lost in the sq= uash.

 

Also, it=92s not so much that we *can=92t* l= earn. It=92s that we choose not to. Around here, it=92s a mark of prestige = to not open doors with your face if it seems like there=92s a better way. M= akes it easier to focus on the work.

 

- Bret

 

From: Nate DeSimone via= groups.io
Sent: Tuesday, May 19, 2020 11:02 AM
To: devel@edk2.groups.io; spbrogan@outlook.com; rfc@edk2.g= roups.io; lersek@redhat.com; Bret Barkelew; Kinney, M= ichael D
Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request= based Code Review Process

 

Hi Sean,

My recent spelling fix patch series is a good example of why this is a bad= idea actually:

https://n= am06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2= Fg%2Fdevel%2Fmessage%2F59779&data=3D02%7C01%7Cbret.barkelew%40micro= soft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011d= b47%7C1%7C0%7C637255081625996312&sdata=3DfVz16E37%2BwW5pSgRxI45K7nW= PDlIoS0HuI8UCGmEwjY%3D&reserved=3D0
https://n= am06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2= Fg%2Fdevel%2Fmessage%2F59780&data=3D02%7C01%7Cbret.barkelew%40micro= soft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011d= b47%7C1%7C0%7C637255081625996312&sdata=3D4q0lC1BSlSoQ3p0HGWwlph09HT= jgJRo4nTO2Qx59%2Fjc%3D&reserved=3D0
https://nam= 06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg= %2Fdevel%2Fmessage%2F59781&data=3D02%7C01%7Cbret.barkelew%40microso= ft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db4= 7%7C1%7C0%7C637255081625996312&sdata=3DXQVSwPMXdpDJXj9nkuvq2fenwhNt= 6HGGZXsJwH5Bu8E%3D&reserved=3D0
https://n= am06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2= Fg%2Fdevel%2Fmessage%2F59782&data=3D02%7C01%7Cbret.barkelew%40micro= soft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011d= b47%7C1%7C0%7C637255081625996312&sdata=3DkCULGBc6%2Bifcn3cnPTV1odHI= 1ZUxuWQePN3POKKS3SM%3D&reserved=3D0
https:/= /nam06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io= %2Fg%2Fdevel%2Fmessage%2F59783&data=3D02%7C01%7Cbret.barkelew%40mic= rosoft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd01= 1db47%7C1%7C0%7C637255081625996312&sdata=3DSCOhUMdNXHIymGLaw9z3JTh%= 2Fe2BfaJaAyEC99EkG%2Fvg%3D&reserved=3D0
https://n= am06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2= Fg%2Fdevel%2Fmessage%2F59784&data=3D02%7C01%7Cbret.barkelew%40micro= soft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011d= b47%7C1%7C0%7C637255081625996312&sdata=3DepET6Wk30bIHQCvEDFLkeHEfmm= 9tzlxRrJ%2FQAuEfQFs%3D&reserved=3D0
https://n= am06.safelinks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2= Fg%2Fdevel%2Fmessage%2F59785&data=3D02%7C01%7Cbret.barkelew%40micro= soft.com%7C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011d= b47%7C1%7C0%7C637255081625996312&sdata=3DN8T7HjerJVvyGg94yMWjLm%2Fw= 7WDdXOdby1JpOYlPeVc%3D&reserved=3D0

Notice that I split along package boundaries, because the maintainers for = each package is a different set of people. If my patch series was squashed = at merge time... how do I know who reviewed what? If the commit set is not = correct.. I tend to say so in my feedback :). The only sane way to squash this series would be to have a h= uman re-write all the commit messages, which I am against.

Generally those that prefer an easily bisectable history have such prefere= nce mostly due to the usage of validators that immediately resort bisecting= as a method to root cause an issue since they tend to not understand the c= ode very well. Edk2 already has 12 years of non-bisectable history, so this method is going to be ineffectiv= e anyway.

With regard to sending squashed commits, I understand that those who are n= ew may have difficulty sending a properly formatted patch series, but frank= ly attempting to shield them from having to learn I am strongly against. I = suggest that Microsoft invest in its human capital similar to how Intel does. If you cannot figure out how= to send a properly formatted patch series... then do your work on the inte= rnal codebase (or perhaps MU.) Within the Intel, having the skillset to con= tribute to TianoCore is considered a mark of prestige, and thus needs to be earned.

TLDR, I will reject squashed commits on any packages that I maintain.

Thanks,
Nate

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of = Sean
> Sent: Tuesday, May 19, 2020 9:54 AM
> To: rfc@edk2.groups.io; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; lersek@redhat.com;
> bret.barkelew@microsoft.com; devel@edk2.groups.io; Kinney, Michael D<= br> > <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code R= eview
> Process
>
> Nate/Laszlo,
>
> Regarding a squash merge workflow.  I agree it can be abused and= we all
> have seen terrible examples.  But a patch series that contains 5= 00+ file
> changes isn't really much better.  Just because it is broken int= o multiple
> commits doesn't mean it is the right set of commits.
>
> Anyway a squash merge workflow works amazingly well with and is
> optimized for a web based review and PR processes.  It allows a = user to
> respond to changes, fix issues, learn thru the PR process, all while = keeping
> complete track of the progression.  Then once all "status&q= uot;
> checks and reviews are complete, it is squashed into a neat commit fo= r
> mainline, containing only the relevant data in the message.
>
> So, the ask is that we don't exclude squash merge workflows.  Th= ose
> reviewing the PR can decide what is appropriate for the PR content > submitted.  Just as you would request changes to the contents (o= r
> ordering) of a commit in a series, if the reviewers don't agree that = the PR
> contents should be in a single commit then obviously it shouldn't be<= br> > squashed to one.
>
> Contributions like spelling fixes, typos, minor bug fixes, documentat= ion
> additions/fixes, etc all are great examples of PRs that can easily le= verage
> squash merges and this workflow significantly lowers the burden of th= e
> contribution and review process.  This workflow is also are much= easier for
> casual or first time contributors.
>
> I don't exactly know how we would enable this but I assume we could > leverage tags or make it clear in the PR description.  First ste= p is to get
> alignment that a squash merge workflow, while not appropriate for all=
> contributions, is not something to be excluded.
>
> Thanks
> Sean
>
>
>
> On 5/19/2020 12:21 AM, Nate DeSimone wrote:
> > Hi All,
> >
> >
> >
> > I tend to agree with most of Laszlo's points. Specifically, that= moving to pull
> requests will not fix the fact that maintainers are usually busy peop= le and
> don't always give feedback in a punctual manner. Like Laszlo, I would= also
> prefer that we do not squash patch series. My biggest reason for not<= br> > squashing patch series is because when you put everything into a sing= le
> commit, I have had to review commits with 500+ files changed. Ope= ning git
> difftool on a commit like that is awful.
> >
> >
> >
> > However, I would like to register my general endorsement for pul= l requests
> or some other web based system of code review=85 and I don=92t have a= n
> Instagram account by the way :) Personally, I prefer Gerrit as I use = it a lot with
> coreboot and other projects. But since we are using Github for hostin= g, pull
> requests are an easy switch and a logical choice. My main reason for = being
> excited about pull requests mostly has to do with the amount of manua= l
> effort required to be a TianoCore maintainer right now. I have set up= my
> email filter so that the mailing list is categorized like so:
> >
> >
> >
> > [cid:image001.png@01D62D71.502B55E0]
> >
> >
> >
> > Implementing the logic to parse the contents of emails to catego= rize them
> like this required me to define no less than 12 email filter rules in= Microsoft
> Outlook, and I have to change my filtering logic every time I am
> added/removed from a Maintainers.txt file. I=92m sure every other mai= ntainer
> has spent a time separately implementing filtering logic like I have.= This helps,
> but still for every thread, I have to go and check if one of the othe= r
> maintainers has already reviewed/pushed that patch series yet, and if= not
> review/push it. If I have ] feedback on a patch series, I have to cat= egorize it
> as awaiting response from author and check up on it from time to time= ,
> sometimes I ping the author directly and remind them to send a new pa= tch
> series. Implementing this state machine is a lot of manual work and i= t kind of
> feels like I=92m a telephone operator in the 1950s. I greatly welcome=
> automation here as I am sure it will increase the number of patch ser= ies I am
> able to review per hour.
> >
> >
> >
> > Thanks,
> >
> > Nate
> >
> >
> >
> > -----Original Message-----
> > From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of= Laszlo
> > Ersek
> > Sent: Friday, May 15, 2020 2:08 AM
> > To: rfc@edk2.groups.io; bret.barkelew@microsoft.com;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@int= el.com>
> > Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull<= br> > > 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 commit= s to
> >> allow
> >
> >> for the squash flexibility (and is the strategy I think I wo= uld
> >> 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 i= n. 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=99= 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 wit= h).
> >
> > And I think the above workflow is out of scope, for migrating th= e edk2
> process to github.
> >
> >
> >
> > Again, the structuring of a patch series is a primary trait. Ite= rating 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 emerge= s while
> reworking a later patch. For instance, the git-rebase(1) manual dedic= ates 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 s= eries" concept at all, it
> only supported review requests for individual patches, and it support= ed
> setting up dependencies between them. So, for example, a 27-patch ser= ies
> would require 27 submissions and 26 dependencies.
> >
> >
> >
> > Lacking support for the patch series concept was an immediate de= al
> breaker with Phabricator.
> >
> >
> >
> > The longest patch series I've ever submitted to edk2-devel had 5= 8 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-origi= nated
> > 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 thing= s
> simple and difficult things impossible is useless. This is what the I= nstagram
> 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 see= m 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 w= as 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 ser= ies (May
> 11th).
> >
> >
> >
> > (1c) The v2 blurb got almost immediate, and numerous feedback (o= n 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 (n= ot 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 po= sting
> >
> > - 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-maint= ainer
> >
> >           = ;   (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 cha= nge
> with the transition to github.com.
> >
> >
> >
> >
> >
> > (2) The VariablePolicy series is actually a good example that pa= tch 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 t= he library
> instance into each platform DSC, last update Variable driver to consu= me 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 pat= ch order
> changes, and an increased patch count.
> >
> >
> >
> > Thanks
> >
> > Laszlo
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
>
>


 

--_000_CY4PR21MB0743BB6834B9415FBD61E6CEEFB90CY4PR21MB0743namp_--