From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM02-CY1-obe.outbound.protection.outlook.com (NAM02-CY1-obe.outbound.protection.outlook.com [40.92.4.29]) by mx.groups.io with SMTP id smtpd.web10.857.1590083627872270902 for ; Thu, 21 May 2020 10:53:48 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=fGWuQkI8; spf=pass (domain: outlook.com, ip: 40.92.4.29, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=KJokhoq8GXDNv+toiiLe5ljagHKhb/Fk7dVx8a5PxKx5oM5g51mKWkvlVJ9mlWGFHVzVdHoJ9ofkvN0TOOkCWxyP5Pu6EEBFuwHsFT7xSZr7mhB7Hq3Te84NCPci1jSQ/9KDZFhrvG1o8wLBpIiUacXmCiXgVUOFTcw8ruqUen+dLEyTp4LSa4ESr239iYjeQelheUnjKv4jhUe6K7tRTBoszTdipAwg6kij1JYlSTUziv0LeAdAmbMxi5xGQfUBqOWIFDbVLLWbadbXpFfa3KnLnUaRBDjwJF0HOUHqM3olbZxLBCkHIJX1CifNqTOj/N92RM6qmAkX+vwnicUgXQ== 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=RnvfavNRcpxeytLgoio1LZlYzbDvIxkvKi8VkN917VU=; b=LtUOVoa1iAJ11lvjpYMea0QNR2B6ELFjeTVBsc8lwZT9M2MRWT9g8TebZsv4t9ytodSbBjfp4bBMLfT+XuocZlt0BhG1NPF1+ayyeSxVxWPltg+M4/yGWFGNRza9CUk+ZROEH/xh/TpF31GsV4dyp9KgbG9VtDQnDbMWxplRN95k8FBRZ9FNCw64KdEAXluEyQt0v8icSZR1aE/s2//36XjhOCmsfm67Q8LXwpOkeVGxKsJfuc8F2yHITkscNJcv5kzGIuk058POqSxrH5iCrjoWwyP8agWT+/VUpc8fRV5QlKxoRMOs9Zzngn0Hl4fU4GNU8dlgO6Q9lrHPCkzfog== 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=RnvfavNRcpxeytLgoio1LZlYzbDvIxkvKi8VkN917VU=; b=fGWuQkI8khWcsIXU9hcnOHFWorXJ7osPhdYyVps+KnbW14801Ot+Yhb8KXlQXuHN8jydTAfLSZQWz6z2naLEUyBmULETE9hkB98Osl5qsj/kj+MLut7v12dc2c4Xm64A3u4m+hyLHUmtozhi6utOxEi1KthUACoGwBf20MkCgbLEm382lhtZ+UikUTClwW3YGSivEHg7cQRuRYS9jSxaplxgjdguYg8zCRF0ChyNaLcDuoFNnXRoQ3gmaFHBylSNHt3hzQxn5wCnVWsenumHNXKVp6QxZMGIq4X4N4LztAfoI9tZd6iObW3bKpSu6ZHtBJBEMPavdkdF/46YeWAzwA== Received: from CY1NAM02FT061.eop-nam02.prod.protection.outlook.com (10.152.74.59) by CY1NAM02HT029.eop-nam02.prod.protection.outlook.com (10.152.75.100) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23; Thu, 21 May 2020 17:53:47 +0000 Received: from BN8PR07MB6962.namprd07.prod.outlook.com (2a01:111:e400:7e45::48) by CY1NAM02FT061.mail.protection.outlook.com (2a01:111:e400:7e45::286) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23 via Frontend Transport; Thu, 21 May 2020 17:53:46 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:BDB3227AD5AC827EF384DC81062563D7365596881A3D2F4DB6EBC2DCDF0E20AB;UpperCasedChecksum:95405246CE9EA6D65A2E7F773E60DD82D08072FE8DC085C363421FF4E55724E0;SizeAsReceived:9359;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%8]) with mapi id 15.20.3021.026; Thu, 21 May 2020 17:53:46 +0000 Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: Laszlo Ersek , devel@edk2.groups.io, rfc@edk2.groups.io, nathaniel.l.desimone@intel.com, "bret.barkelew@microsoft.com" , "Kinney, Michael D" , "Leif Lindholm (Nuvia address)" , Andrew Fish References: From: "Sean" Message-ID: Date: Thu, 21 May 2020 10:53:43 -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: MWHPR1601CA0006.namprd16.prod.outlook.com (2603:10b6:300:da::16) To BN8PR07MB6962.namprd07.prod.outlook.com (2603:10b6:408:d6::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: <5baa9bcc-35dd-9b22-a8b1-6cdbd4e01563@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.2.78] (50.47.113.221) by MWHPR1601CA0006.namprd16.prod.outlook.com (2603:10b6:300:da::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23 via Frontend Transport; Thu, 21 May 2020 17:53:45 +0000 X-Microsoft-Original-Message-ID: <5baa9bcc-35dd-9b22-a8b1-6cdbd4e01563@outlook.com> X-TMN: [zKFm2CZi39aCwwlVOyBuXNqgsnlgxEkD] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 49 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: ffa70465-6d94-4fdd-225a-08d7fdafe900 X-MS-TrafficTypeDiagnostic: CY1NAM02HT029: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: MWxMVrBg9mQOs7i2f6+w4Qqim4P/Nxvbt8jqKdWXKUlMgjX0/6Fs78GzasEEbZnzlwxhotVWElYN1LI2oFbVUf3N3GuAREzeBKrd3XNHOGJArAng1V1rchkrRRXVrp+OLtJbCnfYtGmubFjiwY4vfGeSVMpBVGgGJeIGZJurC3kqFsEnaK26Cz6Fo3q6bW3O0oyEQikrH5Yc/w5URFo2vg== 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: KrekyRlCJhb3DFegKZrB+0E/FBVQQPfV8k2OBAK3CR9qVT5xdcpu2+dr74U841i4+W3wJBaiuwSQagLZHXNJMaVbOvmRgNaZWr/f1q3lPaPWsPWlqjt/wOQZYiQUCK2ltzwSS1UWBRQoVqGDjq49tQ== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: ffa70465-6d94-4fdd-225a-08d7fdafe900 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 May 2020 17:53:46.8365 (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: CY1NAM02HT029 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Laszlo, I appreciate the back and forth. I find email a challenge for this type of discussion because it leaves so much to individual interpretation and bias. Anyway Thank you for having the discussion. I hope others with opinions feel empowered to chime in and help this RFC go in the direction the community and stewards desire. I am still in full support of the RFC and am ok to table my concerns that changing the tools without adapting the process will lead to a less than optimal workflow. Anyway, I look forward to seeing if the "pull request based code review process" can help improve the communities collaboration and efficiency. I have a few additional responses below that will clarify my thoughts but hopefully not invoke responses. :) On 5/21/2020 6:30 AM, Laszlo Ersek wrote: > On 05/20/20 00:25, Sean wrote: >> On 5/19/2020 1:41 PM, Laszlo Ersek wrote: > >>> 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 >> should be considered the representatives of this code base. They >> have a vested interest to enable this repository to work for them. If >> they really are viewed as "sloppy" or "lazy" then we are destined to >> fail anyway. > > You put it very well. "They have a vested interest to enable this > repository to work for them." Key part being "*for them*". > > Core maintainers are responsible for making this repository work for a > lot larger camp than just themselves. Even if squash-on-merge satisfied > the requirements that core maintainers presented, squash-on-merge would > still hurt the larger community that depends on those packages. > > The core-consumer community may not necessarily participate in the > day-to-day maintenance of the core packages, but they do report bugs and > even contributes bugfixes / occasional features, when their particular > use cases require those actions. > > And squash-on-merge hurts those activities, down the road, because the > git history is instrumental to analyzing and learning the code base. > > For example, the question "why do we call this function here?" > immediately leads to running "git blame" (possibly a series of git-blame > commands, to navigate past code movements and such). In the end > git-blame leads to a particular commit, and that commit is supposed to > answer the question. If the commit is huge (e.g. a squash of an entire > feature), then the question is not answered, and git-blame has been > rendered useless. > > >> Nothing in my statement of "don't exclude squash merge workflow" >> requested that we allow a PR to be squashed into a single commit that >> you believe should be a patch series. > > If the button is there, maintainers will click it even in cases when > they shouldn't, and I won't be able to catch them. The result will not > necessarily hurt the maintainer (not at once, anyway), but it will harm > others that investigate the git history afterwards -- possibly years > later. > > I can't watch all CoreFoobarPkg pull requests on github to prevent this. > On the other hand, I can, and do, monitor the edk2-devel list for > seriously mis-organized patch sets, especially for core packages where > I've formed an "I had better watch out for this core package" > impression. > > I have made requests under core patch sets where I was mostly unfamiliar > with the technical subject *for the time being*, asking just for > improvements to the granularity of the series. Knowing the improved > granularity might very well help me *in the future*. > > > The mailing list equivalent of "squash-on-merge" would be the following: > > - contributor posts v1 with patches 1/5 .. 5/5 (for example), > > - reviewer requests updates A, B, and C, > > - contributor posts (in response to the v1 blurb, i.e. 0/5) further > patches 6/8, 7/8, 8/8 > > - reviewer checks the new patches and approves them, functionally, > > - maintainer says "OK let me merge this", > > - maintainer applies the patches (all 8 of them) from the list, on a > local branch, > > - maintainer runs a git rebase squashing the whole thing into a single > patch, > > - maintainer does *not* review the result, > > - maintainer opens a PR with the resultant single patch, > > - CI passes, > > - the patch is merged. > > The above example should not be allowed in any process. If a contribution was submitted as a patch series with 5 patches intentionally, then it would not be a candidate for a squash merge. The squash merge workflow is only acceptable when it is agreed that the end result should be 1 patch. > With the list-based process, the disaster in the last step is mitigated > in at least three spots: > > - All subscribers have a reasonably good chance to notice and intervene > when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to > the v1 blurb, clearly with an intent to squash. > > - Because the maintainer has to do *extra work* for the squashing, the > natural laziness of the maintainer works *against* the disaster. Thus > he or she will likely not perform the local rebase & squash. Instead > they will ask the contributor to perform a *fine-grained* squash (i.e. > squash each fixup into the one original patch where the fixup > belongs), and to submit a v2 series. > > - If someone interested in the git history catches (after the fact) that > the maintainer merged a significantly different patch set from what > had been posted and reviewed, they can raise a stern complaint on the > list, and next time the maintainer will now better. > > (This is not a theoretical option; I low-key follow both the list > traffic and the new commits in the git history (whenever I pull). In the > past I had reported several patch application violations (mismanaged > feedback tags, intrusive updates post-review, etc). Nowadays it's gotten > quite OK, thankfully, and I'm terrified of losing those improvements.) > > > If we just plaster a huge squash-on-merge button or checkbox over the > web UI, it *will* be abused (maintainer laziness will work *towards* the > disaster), with only a microscopic chance for me to prevent the abuse. > > It's not that "I believe" that this or that *particular* series should > not be squashed. "Not squashing" is not the exception but the rule. The > *default* approach is that the submitter incorporates incremental fixes > into the series at the right stages, they maintain a proper series > structure over the iterations, and they propose revised versions of the > series in full. Squashing is the exception; for example one reason is, > "if you separate these changes from each other, then the tree will not > build in the middle; they belong together, please squash them, and > resubmit for review". > > >> I do think those rules will need to be defined but that is needed >> today anyway. > > Rules are only as good as their enforcement is. > > The question is not how nice it is to use squash-on-merge in the > minuscule set of situations when it might be justified; the question is > how difficult it would be to prevent the inevitable abuses. > At time of writing i don't know any way to enforce this if the maintainers can not be relied upon. Given my strong agreement with "Rules are only as good as their enforcement is." I don't see a practical way to resolve this and you seem content with the current solution. Thanks for your diligence here. > The list lets me advocate for proper git history hygiene reasonably > efficiently (although I still miss a bunch of warts due to lack of > capacity). With the squash-on-merge button or checkbox, the flood gates > would fly open. I won't stand for that (not as a steward anyway). > > I think our world views differ fundamentally. I value the git history > *way* above my own comfort, and everyone else's (accounting for both > contributor and day-to-day maintainer roles). I guess you prefer the > reciprocal of that ratio. > > Thanks, > Laszlo > Thanks Sean