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.92.21.95]) by mx.groups.io with SMTP id smtpd.web11.1545.1589927123228890857 for ; Tue, 19 May 2020 15:25:23 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@outlook.com header.s=selector1 header.b=QGgCPdeT; spf=pass (domain: outlook.com, ip: 40.92.21.95, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=Iwujseioa/gJb1cdAFphw48ecKmp+g2+rC/RO1icel1LBodBb7dJaFZ9rbUG6r9V67eyV+XGvHuIKg1dOVrHQTcN5iarzSZ8d6oiYg0SlTjDfhXELvYA+YuCzD5tvBpq1JLeWZHhuPxyyUIvI2vNrOftGcAT8HaXD2egvCnnHDnz8mYXp55k1XVs/4+/QlWWb6mZ+E/8vDOSJigxZGfnQFxJoX9JpDrpHjcbjFx2AHBoQBZ19WwNY4ljw0ruMubLjlsO7N1wrL72LPOr9PhJTef1aDIH66jiRd3Tib41X2OpR5h8jYfRhx6PzQp7hBCgB9hx4IfZby8Uy+wOsnnWag== 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=kXBlae93lJBFVRRyvHv6wWBC/S7yl61jKGdC5cfMRRE=; b=I5n/sgvZafvUplb03CIIRqidtgJew1bmeEQhwASOA141gsxIiy2v4kJExPY2HSuPvki3cHds+RsURZYM3cKa4xQr5yv5lG6MPx4vqTuNqpdW3XFdxujaTVkc13mEHpKaxeDN2bEQigzGqd0AiPICRPX80D+I7vRaGwW0UehjZhWWgpaDfSdRL3OeklcBV1KxH/radquOZb2SU6WSg3xKKPcMB//4+TFoeaxlyZ56UrpIMWKG3mqmAtQJ6xrz+CdEHvfae+80HUF7pn7FcIGiwBwxDcpnOPtJFBfw9LXUxcf9BSIGNWx2nba87tUz4gignIvIZsu/Bu/bDDfyDyt40w== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=outlook.com; dmarc=pass action=none header.from=outlook.com; dkim=pass header.d=outlook.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=outlook.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=kXBlae93lJBFVRRyvHv6wWBC/S7yl61jKGdC5cfMRRE=; b=QGgCPdeTrdDgXL8XiNvDo1rZcpSnDfCvipQMkhEFL1qzzQaRv6QhwUm2d1xTrYUmuveuc6mYYJtekD2v2VRgkFc347htQ4+78nFO+U0MzHPtfbigcKEuuso3twxKqeUTVKzGyH3tkVB/kLRtsToPOX54hO0sr0Vs/88UwBh08iQ4QKaL1xnrzHlsmEbCB1gDfoL1HA8qYGO5epRgV1cgYP8PkZ0ED99e6zd0XHhtJr/z+eGrbyGcOXBDYdg6h5SHitFJXzEc71xd5F3YglIFnAunJu23bNGgpygTYzULqVNrhAU5nyGJbBasNqanQ1NzVZuabawF4nKpWKoTsVO5jw== Received: from BN8NAM12FT026.eop-nam12.prod.protection.outlook.com (2a01:111:e400:fc66::41) by BN8NAM12HT103.eop-nam12.prod.protection.outlook.com (2a01:111:e400:fc66::131) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.11; Tue, 19 May 2020 22:25:21 +0000 Received: from BN8PR07MB6962.namprd07.prod.outlook.com (2a01:111:e400:fc66::46) by BN8NAM12FT026.mail.protection.outlook.com (2a01:111:e400:fc66::202) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.11 via Frontend Transport; Tue, 19 May 2020 22:25:21 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:25ECA550C2DA7CDAC1529DA62484B7F132135D7F0DD85EF26ADFDFE054175499;UpperCasedChecksum:C8CB6AB573945BBAD49B415BC8BC3DB27B521B42622339175A803EADFA492791;SizeAsReceived:9247;Count:49 Received: from BN8PR07MB6962.namprd07.prod.outlook.com ([fe80::edbc:7122:7dd9:1403]) by BN8PR07MB6962.namprd07.prod.outlook.com ([fe80::edbc:7122:7dd9:1403%9]) with mapi id 15.20.3000.034; Tue, 19 May 2020 22:25:21 +0000 Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: Laszlo Ersek , rfc@edk2.groups.io, nathaniel.l.desimone@intel.com, "bret.barkelew@microsoft.com" , "devel@edk2.groups.io" , "Kinney, Michael D" , "Leif Lindholm (Nuvia address)" , Andrew Fish References: From: "Sean" Message-ID: Date: Tue, 19 May 2020 15:25:18 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 In-Reply-To: X-ClientProxiedBy: MWHPR19CA0060.namprd19.prod.outlook.com (2603:10b6:300:94::22) To BN8PR07MB6962.namprd07.prod.outlook.com (2603:10b6:408:d6::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: <9422d6a0-d5fc-2bb5-911e-01541fbf80c6@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.2.78] (50.47.113.221) by MWHPR19CA0060.namprd19.prod.outlook.com (2603:10b6:300:94::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3000.25 via Frontend Transport; Tue, 19 May 2020 22:25:20 +0000 X-Microsoft-Original-Message-ID: <9422d6a0-d5fc-2bb5-911e-01541fbf80c6@outlook.com> X-TMN: [u7IKIsRf1+CGSFGA93X6+lu2BzpjtJe3] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 49 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: fec2b45d-4ac0-4b7b-2827-08d7fc43845d X-MS-TrafficTypeDiagnostic: BN8NAM12HT103: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: 9Zor3tUp3e4l0Ixs9IRUl9To731P1HMlqoyYKpOAYE0M67Oik2Y4sxmhtFqL9qW9P/WRQFUXTYqXei2QzTyskHdG64sZWw9NsybC1HIuENXNivXowq4bqkkgq7PjFJH+TaHS050VfFCduUGbk6A1usvDtSil9Tz15FNjiJ3YAV9gWXFc+9JFgLysAdSKO/b8mfThaDRTZdjtj+uUrlHDiQ== X-Forefront-Antispam-Report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:0;SRV:;IPV:NLI;SFV:NSPM;H:BN8PR07MB6962.namprd07.prod.outlook.com;PTR:;CAT:NONE;SFTY:;SFS:;DIR:OUT;SFP:1901; X-MS-Exchange-AntiSpam-MessageData: LVQ5Aj+NkSdCd1MNl/I9CpmuIEQzhnUpCgVCXpQfz96dB7A9I+iU7gufvZt4Ia8wkJqMHoon2l8fPCJgvOVSSrvOc8mQP3dKDY9MwUAsHJ9Nu8/kvPoApRKDMnh73jP4Ws5GdCETEYlKnU3ZDUIwUg== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: fec2b45d-4ac0-4b7b-2827-08d7fc43845d X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 May 2020 22:25:21.8074 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-FromEntityHeader: Internet X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8NAM12HT103 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable Laszlo, First let me be clear, there is no desire or intent in any of these=20 conversations/discussions for anyone to feel so distraught to give up=20 this project, let alone someone so active and involved as yourself. The basis for my perspective goes back to the conversations we had=20 numerous years ago about being more inclusive and enabling more of the=20 firmware development community to contribute and be involved in this=20 project. In my opinion this project needs help. It needs more=20 maintainers, contributors, reviewers, testers, and active users. It=20 needs people to write documentation, answer questions, triage bugs, and=20 plan release cycles. Without removing some of today's barriers, support=20 will continue to decline and relevancy of this project will decline with it= . One of the first suggestions was to evaluate the contribution and review=20 process, looking for places in the current process that are confusing,=20 error prone, inefficient, or hard to drive consistently. It was also=20 important to evaluate trends in other successful open source projects.=20 From this the process moved towards evaluating GitHub based pull=20 requests for the contribution and review process. That gets us to this=20 discussion and in my opinion a slightly larger scope in that we are not=20 trying to reproduce the current process using new tools but rather=20 adjust the process to address the discussed issues leveraging these tools. Another request from the community discussions years ago was to add=20 testing capabilities to remove manual work and improve quality. There=20 has been a huge effort over the last year to enable a "core" CI system,=20 practical/easy to use unit testing capabilities, and most recently=20 "platform" CI. These features where developed and enabled to give=20 contributors clear expectations for the quality needed for successful=20 contribution. In all of these efforts, my hope has been to enable more=20 people to join this project. Anyway, for what it is worth, I hope this long winded response provides=20 some background into my perspective. I'll respond to other comments=20 below. On 5/19/2020 1:41 PM, Laszlo Ersek wrote: > (+Leif, +Andrew) >=20 > Sean, >=20 > On 05/19/20 18:54, Sean Brogan wrote: >> Nate/Laszlo, >> >> Regarding a squash merge workflow.=C2=A0 I agree it can be abused and we= all >> have seen terrible examples.=C2=A0 But a patch series that contains 500+= file >> changes isn't really much better.=C2=A0 Just because it is broken into >> 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.=C2=A0 It allows a use= r to >> respond to changes, fix issues, learn thru the PR process, all while >> keeping complete track of the progression.=C2=A0 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.=C2=A0 Those >> reviewing the PR can decide what is appropriate for the PR content >> submitted.=C2=A0 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 >> leverage squash merges and this workflow significantly lowers the burden >> of the contribution and review process.=C2=A0 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.=C2=A0 First step i= s to >> get alignment that a squash merge workflow, while not appropriate for >> all contributions, is not something to be excluded. >=20 > the scope for migrating the contribution & review workflows off the > mailing list and to github.com was set many months ago. That scope does > not include institutionalized changes to patch set structuring criteria. > The "git forge" evaluations that we had spent weeks/months on also > focused on how candidate systems would honor a patch series' structure; > i.e., how faithful the system would remain to the contributors' and > reviewers' shared intent, with a specific patch set. I hope the scope is to build an effective and efficient contribution=20 process that helps current contributors deliver more while opening the=20 door to the rest of the firmware community. It should require less=20 effort to contribute a change to edk2 than to maintain a downstream=20 fork. Today this is not true. >=20 > Your proposal to "don't exclude squash merge workflows" is a trap. If we > tolerate that option -- which is obviously the sloppy, and hence more > convenient, option for some maintainers and some contributors, to the > detriment of the git history --, then almost every core maintainer will > use it as frequently as they can. In the long term, that will hurt many > consumers of the core code. It will limit the ability of people not > regularly dealing with a particular core module to file a fine-grained > bug report for that module, maybe even propose a fix. From the > regression analyst's side, if the bug report starts with "I have a > bisection log", that's already a good day. And your proposal would > destroy that option, because maintainers and people in general are > irrepairably lazy and undisciplined. We cannot post a community member > shoulder-by-shoulder with every core package reviewer/maintainer to > prevent the latter from approving a squash-on-merge, out of pure > laziness. I'm 100% sure the "option" to squash-on-merge would > *immediately* be abused for a lot more than just "typo fixes". There > isn't enough manpower to watch the watchers, so "no squash-on-merge" > needs to be a general rule. I have trouble with this line of thinking. The maintainers are and=20 should be considered the representatives of this code base. They have=20 a vested interest to enable this repository to work for them. If they=20 really are viewed as "sloppy" or "lazy" then we are destined to fail=20 anyway. Nothing in my statement of "don't exclude squash merge workflow"=20 requested that we allow a PR to be squashed into a single commit that=20 you believe should be a patch series. I do think those rules will need=20 to be defined but that is needed today anyway. >=20 > I'm very sad that you're trying to wiggle such a crucial and intrusive > workflow change into the scope of this transition.=20 Not "trying to wiggle" anything, just focused on providing feedback and=20 hoping to help develop an efficient and effective process using the=20 tools available. See intro paragraph. > Every time > squash-on-merge has come up over the years (regardless of this > transition), we've labeled it as one thing never to do, because it > destroys information (and/or even encourages not *creating* that > historical information in the first place, which is of course important > in reality). >=20 You may have labelled it that way but given the wide spread use of this=20 practice and my own great experiences enabling a broad team with mixed=20 backgrounds using this practice, I personally haven't. This community=20 is a quiet one and I believe instead of speaking up, members just choose=20 not to get involved. > Well, anyway, here's my feedback: if squash-on-merge is permitted in > edk2 or in basetools (or in any other external repository that's a hard > requirement for building edk2), that's a deal breaker for me, and I'll > hand in my resignation as a steward. >=20 > Maybe you'd consider that a win, I don't know -- but I couldn't remain a > steward with a straight face after failing to protect what I consider > one of the core values of open source / distributed development. >=20 > Thanks, > Laszlo >=20 Thanks Sean