From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.107.236.98]) by mx.groups.io with SMTP id smtpd.web11.5377.1589919055128050445 for ; Tue, 19 May 2020 13:10:55 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@microsoft.com header.s=selector2 header.b=ftUDW3i2; spf=pass (domain: microsoft.com, ip: 40.107.236.98, mailfrom: bret.barkelew@microsoft.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Mr3xCvA+UhXS7Incy+8IVSNqNmsCP/TfFzBxEiMHGeBE/eQFvmBWUXsZjPFO+ofrD6rfNNQ6gFf6ncpydtA6Q41uhPWT34SdvcUV+GtpA0Cfz/uirIj0oWoyaK4KpqyzbRAJG9L8cLZAfzLSRSMylc9RoMhLuAshFE1KvpMORHN3XPrAnn+ChzhVA5JOdNqeKu2gsmZc841l9bhhmfxj7u6qb5Ra+hrPHIIWcQpNh8m2x9OtDQptOR7hs+oNcLk7bqiNUTifMQ5WaNUny9ecVivnAVKnFTUp1oFtE9syS8oG9CSvxzSrJYOW4Kl5osmMBxIgPpITC3oqWzaOCCdwYQ== 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=Sf7d4UI+/UbSG7K4s1rCzp4TQDkbgTtCFmqSMitRha0=; b=jQRbDZwa+7lMDb1YMelOo84KwjX0FlHvwYNYUbiGNdNpL+Brh1nLlJUKbKBA8NxwqFiPM4Wql5DEJso+WMuXOfRL5sk7MZkvky8QNKlEFATremb+Hl2Mw9bZs524lJChnuySkI1qge9MY7qpC2nD1ekDS+4Rl86ccV9OumIPr2wWJ+ZFSwFa0M08x1LVgRByxe0krp050UGgxPIqypmMHV9IhYgbxv+c2a8zR2fG6shtDdmZcn1pIUGRED1+WCizWVVvjRuRXRQddBSyd+oA5j6wJjt8RKzVUuZ7fUwk6crMQypi4x8uPGzk1GXAYyo1lT2Q8wJeOvfquXMilhrhnQ== 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=Sf7d4UI+/UbSG7K4s1rCzp4TQDkbgTtCFmqSMitRha0=; b=ftUDW3i2Vf8gAvbbgSN3eRqCx8t0BpBgyaYDdW1yS/7Wi7bTHKFHDbJXR9a4MBtp7V07Do7flFr9zOqQHRpE1msuTViId64rxuetxH/rhbUpf7i2NLR0Zfz3SbMaRq6gNRT9VWq0ag1r0IklaFKE48qqwDGZMKXYwnJMxjX4nQQ= Received: from CY4PR21MB0743.namprd21.prod.outlook.com (2603:10b6:903:b2::9) by CY4PR21MB0183.namprd21.prod.outlook.com (2603:10b6:903:ba::9) 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 20:10:52 +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 20:10:52 +0000 From: "Bret Barkelew" To: "Desimone, Nathaniel L" , "devel@edk2.groups.io" , "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: AQHWLgex0PJvxvOSUUG9f19bEFeuN6ivzG8tgAAHdACAAAA/bQ== Date: Tue, 19 May 2020 20:10:52 +0000 Message-ID: References: , In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: yes 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: intel.com; dkim=none (message not signed) header.d=none;intel.com; dmarc=none action=none header.from=microsoft.com; x-originating-ip: [71.212.144.72] x-ms-publictraffictype: Email x-ms-office365-filtering-ht: Tenant x-ms-office365-filtering-correlation-id: b613ecfc-4b69-4dbb-6cea-08d7fc30bb20 x-ms-traffictypediagnostic: CY4PR21MB0183: x-ld-processed: 72f988bf-86f1-41af-91ab-2d7cd011db47,ExtAddr x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:27; x-forefront-prvs: 040866B734 x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: LkNkr8Eu/YdCk0WNPa4OyfcVof08b9F3EEpgsN75dVY62Gm4UpXEwboViMqwZurmWOGfYSyNNtAgwFT5/7mFIjXQ09d0jWjTIpcaM0ypoW45FzTX5wY202zue/yX+FOop0UxJUL+BKsfbQZsET2/S0KAqgpjkj6yQPUKIzn4RMOZeJHtiYR2kboM3jzzR0Uh8XzcESL6vtqVn1Yw838KemAeoUO/UBZlhqm8RG6M502c5kTVqlBI7lZohWuBlcUp8NHKoO7XB7sj/yUqhrVCp97SD7g9Q49V8Xg//lmKeVBPZkvV6DyO6vah0NSOn5ptMcB8c6yu3QOD64wk4UioVAQ/0oSyUI5eiFl7fJKPB0SjLHVHRAlFMg0zD+847gIifOofU1Ar2Itq3tiJ0MUsssz4nsEtzhlZ5O5b5kNwQ3jg5xnwiysbZsAs9yxd/B6rRI8l+uXmWlSerZ8rYGHjKg== 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)(66446008)(66616009)(66556008)(8936002)(64756008)(91956017)(66946007)(66476007)(8676002)(76116006)(52536014)(82960400001)(82950400001)(99936003)(26005)(8990500004)(55016002)(33656002)(6506007)(186003)(53546011)(30864003)(7696005)(2906002)(86362001)(498600001)(71200400001)(110136005)(166002)(9686003)(10290500003)(76236002)(966005)(5660300002)(31884003);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata: WzrU1OJOlZbPtI0MxrVSgWdYhFzNKkqNaK9N8ZISFPTjqQgRoceYHxnzwaabd6zouyRS3IBI3lvl6h48hiuQZQY7TYwM4QUGNftWhq8ynRlAC1V6BM+lBy4ncCiyY0uJ3ONzeTFzmrBA4rxjOgJc1sUIJCySit4gxgc6LXKB0Tc0quPJANLTdwmkJpZHNDwYzYrTUqiRYXz7BZJslizRabeTHNVagB/Xtn8Zx0oZTIbCz7aTf6OoHbcuPIIic5XojoXjZeECovv3ZkBYELMpY3mqCGEVwTEA+/js7RObVA5pkzYNSuvH2Xc5ylhNUO1nD/9+WhwP0IlHxt3XnzsX4kkFEcx4wQDIZbBKshEgG1Q9t5YBN9eZZhUXH4v8f0yLuHavSkYeTrG/tTCpbK0GGGLiSRH1c1a5GiXF9ldTU/HiTmmut3XPjlNr9Xy/K4zoGLVV3ZGlG7cGjRVAKJhyrl5yJnLQ+55ajjGdLnZ4Lls= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: microsoft.com X-MS-Exchange-CrossTenant-Network-Message-Id: b613ecfc-4b69-4dbb-6cea-08d7fc30bb20 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 May 2020 20:10:52.2569 (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: zfEOcPLyT56fcNKVm363ZE22U9EW16mwJJIIGeCM3/vVhVuSUBwFlILoxmKoItUaX1Upq7QeH7L1Jf//XOyyUA== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR21MB0183 X-Groupsio-MsgNum: 59846 Content-Language: en-US Content-Type: multipart/related; boundary="_004_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_"; type="multipart/alternative" --_004_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_ Content-Type: multipart/alternative; boundary="_000_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_" --_000_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_ Content-Type: text/plain; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable I will honor Mike Kinney=92s efforts with my vote of confidence. I think we=92re headed in the right direction, even with some of the thing= s that I disagree with. In my history with TianoCore, I have learned to not be so quick to say =93= this is fucking stupid=94. Every time I=92ve done that, I=92ve later discov= ered the reasons behind it, and even come to the conclusion that the design= ers were quite clever. That said, I want to contribute. And I won=92t with the current system. I = hope to be able to with the future system. - Bret From: Desimone, Nathaniel L Sent: Tuesday, May 19, 2020 12:59 PM To: devel@edk2.groups.io; Bret Barkelew; spbrogan@outlook.com; rfc@edk2.groups.io; lersek@redhat.com; Kinney, Michael D Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based = Code Review Process Hi Bret, I believe you missed my point. I don=92t want my patch series to be merged= piece by piece; I want it merged all at once, in the order that I specifie= d. I tend to agree with Laszlo that you are choosing not to learn how to use = Git properly. Commit early, commit often, perfect later, publish once is th= e Git best practice. You should not hide the sausage making, which is exact= ly what you are proposing. I find it unfortunate that you consider refusing= to learn GIt best practices a mark of prestige. Thanks, Nate From: on behalf of "Bret Barkelew via groups.io" Reply-To: "devel@edk2.groups.io" , "bret.barkelew@mi= crosoft.com" Date: Tuesday, May 19, 2020 at 12:35 PM 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 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_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_ Content-Type: text/html; charset="Windows-1252" Content-Transfer-Encoding: quoted-printable

I will honor Mike Kinney=92s efforts with my vote o= f confidence.

I think we=92re headed in the right direction, even= with some of the things that I disagree with.

 

In my history with TianoCore, I have learned to not= be so quick to say =93this is fucking stupid=94. Every time I=92ve done th= at, I=92ve later discovered the reasons behind it, and even come to the con= clusion that the designers were quite clever.

 

That said, I want to contribute. And I won=92t with= the current system. I hope to be able to with the future system.

 

- Bret

 

From: Desimone, Nathaniel L
Sent: Tuesday, May 19, 2020 12:59 PM
To: devel@edk2.groups.io; Bret Barkelew; spbrogan@outloo= k.com; rfc@edk2.groups.io; lersek@redhat.com; Kinne= y, Michael D
Subject: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request= based Code Review Process

 

Hi Bret,

 

I believe you missed my point. I don=92t want my pa= tch series to be merged piece by piece; I want it merged all at once, in th= e order that I specified.

 

I tend to agree with Laszlo that you are choosing n= ot to learn how to use Git properly. Commit early, commit often, perfect la= ter, publish once is the Git best practice. You should not hide the sausage= making, which is exactly what you are proposing. I find it unfortunate that you consider refusing to learn = GIt best practices a mark of prestige.

 

Thanks,

Nate

 

Fro= m: <devel@edk2.g= roups.io> on behalf of "Bret Barkelew via groups.io" <bret.= barkelew=3Dmicrosoft.com@groups.io>
Reply-To: "devel@edk2.groups.io" <devel@edk2.groups.io= >, "bret.barkelew@microsoft.com" <bret.barkelew@microsoft.c= om>
Date: Tuesday, May 19, 2020 at 12:35 PM
To: "devel@edk2.groups.io" <devel@edk2.groups.io>, = "Desimone, Nathaniel L" <nathaniel.l.desimone@intel.com>, &= quot;spbrogan@outlook.com" <spbrogan@outlook.com>, "rfc@edk= 2.groups.io" <rfc@edk2.groups.io>, "lersek@redhat.com"= <lersek@redhat.com>, "Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code= Review Process

 

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://nam06.safelin= ks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2= Fmessage%2F59779&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C6= e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%= 7C637255081625996312&amp;sdata=3DfVz16E37%2BwW5pSgRxI45K7nWPDlIoS0HuI8U= CGmEwjY%3D&amp;reserved=3D0
https://nam06.safelin= ks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2= Fmessage%2F59780&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C6= e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%= 7C637255081625996312&amp;sdata=3D4q0lC1BSlSoQ3p0HGWwlph09HTjgJRo4nTO2Qx= 59%2Fjc%3D&amp;reserved=3D0
https://nam06.safel= inks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel= %2Fmessage%2F59781&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7= C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C= 0%7C637255081625996312&amp;sdata=3DXQVSwPMXdpDJXj9nkuvq2fenwhNt6HGGZXsJ= wH5Bu8E%3D&amp;reserved=3D0
https://nam06.safelinks= .protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fm= essage%2F59782&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C6e4= 7f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C= 637255081625996312&amp;sdata=3DkCULGBc6%2Bifcn3cnPTV1odHI1ZUxuWQePN3POK= KS3SM%3D&amp;reserved=3D0
https://nam06.safel= inks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel= %2Fmessage%2F59783&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7= C6e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C= 0%7C637255081625996312&amp;sdata=3DSCOhUMdNXHIymGLaw9z3JTh%2Fe2BfaJaAyE= C99EkG%2Fvg%3D&amp;reserved=3D0
https://nam06.safelin= ks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2= Fmessage%2F59784&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C6= e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%= 7C637255081625996312&amp;sdata=3DepET6Wk30bIHQCvEDFLkeHEfmm9tzlxRrJ%2FQ= AuEfQFs%3D&amp;reserved=3D0
https://nam06.safelin= ks.protection.outlook.com/?url=3Dhttps%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2= Fmessage%2F59785&amp;data=3D02%7C01%7Cbret.barkelew%40microsoft.com%7C6= e47f5e045f740536a0708d7fc1ed290%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%= 7C637255081625996312&amp;sdata=3DN8T7HjerJVvyGg94yMWjLm%2Fw7WDdXOdby1Jp= OYlPeVc%3D&amp;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_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_-- --_004_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_ Content-Type: image/png; name="801BEA439D70489191D6469EEA476862.png" Content-Description: 801BEA439D70489191D6469EEA476862.png Content-Disposition: inline; filename="801BEA439D70489191D6469EEA476862.png"; size=140; creation-date="Tue, 19 May 2020 20:10:51 GMT"; modification-date="Tue, 19 May 2020 20:10:51 GMT" Content-ID: Content-Transfer-Encoding: base64 iVBORw0KGgoAAAANSUhEUgAAAsUAAAABCAYAAAA2LdOTAAAAAXNSR0IArs4c6QAAAARnQU1BAACx jwv8YQUAAAAJcEhZcwAADsMAAA7DAcdvqGQAAAAhSURBVEhL7cMBDQAACAMg+5cygQkeRoMIG9WT VVXVv7MHBORqBklJA7cAAAAASUVORK5CYII= --_004_CY4PR21MB074380CFA82DC27BD087A392EFB90CY4PR21MB0743namp_--