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.87]) by mx.groups.io with SMTP id smtpd.web12.1966.1589995288623093004 for ; Wed, 20 May 2020 10:21:33 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@outlook.com header.s=selector1 header.b=a+stkW7p; spf=pass (domain: outlook.com, ip: 40.92.21.87, mailfrom: spbrogan@outlook.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=IZ1P+k6YCy3QRKrbhl3pD4QT1HbSBSrmvD7wkAq2DtLng75jbrYu0yecr90SxnC4GKMbTWREFrlbUHIoJcOWwxjdUWJg7qxN7BP2znsRwsH1bbatFBweUJ+nBj73ZdpuaFCXdvVQ8tiVIRdZSDGo/o0j5Vhx1IsdaCv3Zp6EIDT88aywNSRM3Id5Xi3IDyxEbsXgmdy7tJTZOjtYM6C/eSm7knnzEpbjMBG1c3LgMiXDVNV1hitf66kPnfv6MXBvcQ3VzW1T3qgj/Bk/dJPHHzkAWrd8K8MXc9uBqzv9raMiSzAjKzlAuApr4xaH0dG5imgzwC2VXaxKbWIod+9IuQ== 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=qdxslgT9OeP0QmPoVavWxDHdt62tbN9dRqNczoGy9iE=; b=WWXpz0pGjCJ+j1B5sbzTOSs9e1MsdrQuqxyket7d42w/KX+3nzahefCqX5MP9+y6SvLoJQNcNg54wKLWHKSxI8WIdYWlA5OsHlhSKUs8MMwYfdtfiJ0w7ZF0D61bGPA+GGuy7j+iqH/L7EkXSG6MYkuYWqadTYUJiE7yk88zbZgigYTqe4TIkqACGCPQv9s60Zajh3YwA9y3GexstFGZyTX8XxA3md8sOsS5WsuQ91DF2jyworBAQQruWt5yPY3QVTB7tkSc5mJPgaWrH/dHaibpWtijh8pVAU/zplT7EkFS5zlCjksAmLJVI2I9DMOMPCD3VlbsEdoGPRz8+v1MBg== 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=qdxslgT9OeP0QmPoVavWxDHdt62tbN9dRqNczoGy9iE=; b=a+stkW7pTE49gtQNSZ9DXEhxQUh3BHumasXDVW4vkHQ7HYRftX8f0JI20ZtC8FN27L/NTM9b/HwjK6Eh0xlOilVLG3qIUlBnnKWU5tqN8ddxGEWVDlYAnb9uXrIcp3ymMMieowCmmxYgvfbnQh100UPhQ8jKqjXWDSvucT9h8mPXdw2Shp4V/qIItLnnDUU7meejW/uE8ckfTL6r7Z1QqmyT2b+4m9c4amOWhr1R95H0SPCfdbV1igMmydRVv/FIXu2GMfh5uUdwc1YBO3Exg6DL/F/SxPpMssK7j106OPe0SW0CMMvlGY5S/XF0eoJNKKLIobsC+y9qWzLQz9YduA== Received: from BN8NAM12FT037.eop-nam12.prod.protection.outlook.com (2a01:111:e400:fc66::42) by BN8NAM12HT007.eop-nam12.prod.protection.outlook.com (2a01:111:e400:fc66::195) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.11; Wed, 20 May 2020 17:21:27 +0000 Received: from BN8PR07MB6962.namprd07.prod.outlook.com (2a01:111:e400:fc66::53) by BN8NAM12FT037.mail.protection.outlook.com (2a01:111:e400:fc66::128) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.11 via Frontend Transport; Wed, 20 May 2020 17:21:27 +0000 X-IncomingTopHeaderMarker: OriginalChecksum:30172E7E7619A4A82F9DF2AF39C38287CFD5D58B46D5B4ECF127443697310310;UpperCasedChecksum:C4BCAE2E6DE06774646BF53F6826781AC645343146ECBE4E484460E2D1D10882;SizeAsReceived:9530;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; Wed, 20 May 2020 17:21:27 +0000 Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: rfc@edk2.groups.io, lersek@redhat.com, "Desimone, Nathaniel L" , "bret.barkelew@microsoft.com" , "devel@edk2.groups.io" , "Kinney, Michael D" References: <1DF48A10-F119-4FAE-9963-E95A48D82648@intel.com> <2d42ff42-e028-96aa-6397-284f730d5dab@redhat.com> From: "Sean" Message-ID: Date: Wed, 20 May 2020 10:21:24 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 In-Reply-To: <2d42ff42-e028-96aa-6397-284f730d5dab@redhat.com> X-ClientProxiedBy: MWHPR1401CA0011.namprd14.prod.outlook.com (2603:10b6:301:4b::21) To BN8PR07MB6962.namprd07.prod.outlook.com (2603:10b6:408:d6::11) Return-Path: spbrogan@outlook.com X-Microsoft-Original-Message-ID: MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 Received: from [192.168.2.78] (50.47.113.221) by MWHPR1401CA0011.namprd14.prod.outlook.com (2603:10b6:301:4b::21) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.3021.23 via Frontend Transport; Wed, 20 May 2020 17:21:26 +0000 X-Microsoft-Original-Message-ID: X-TMN: [g2K/sNCwUfX4xqK2u+2Ta2XLOmePB0lE] X-MS-PublicTrafficType: Email X-IncomingHeaderCount: 49 X-EOPAttributedMessage: 0 X-MS-Office365-Filtering-Correlation-Id: f38853c3-f106-4d3e-67f9-08d7fce23a71 X-MS-TrafficTypeDiagnostic: BN8NAM12HT007: X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: /XF3ezbcdQVrLV+RF4+BayJ5YispCP8g8+TN+f+HQYqJtlegqN6DSMF326CsuYoi+DUxB6FUzeXgqcSgejKMcU3zwwPB9/jE+LyK+tGAu3mplYVweTd6b62BVuMu0bOXDdC6Gdba6AlkD6dbYQUkUZJxxqszATf12a1mT+JicJRqUVJdEUzHOz9RPD8SFy7d+hWquQcJxu4/Iif6ALXMepokX1CsHcFLk5iSc0rKfXVo3de8cj1r0vpe5FKFFMcF 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: eykQqShDw3fmsgP6JKHk75jpMrUK7Alvc9sJk8pPivRYYqFUqfitylyi8s6dzgniurCrNG4s4PF9oOI/QFWyfDEyl3Fp78uVix/o1HVJ0pNjCymSZVj7NXyFa/7Ak1yGC/Lm6ZpFGJAePFRc3io4RQ== X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: f38853c3-f106-4d3e-67f9-08d7fce23a71 X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 May 2020 17:21:27.2441 (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: BN8NAM12HT007 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit When this is done in a PR with branch protections this works out differently and in my view your concerns are mitigated. 1. There isn't a partial squash operation. All reviewers know that the final output of the PR is going to 1 commit. Thus there is no confusion of what or how it is being committed to the target branch. 2. With GitHub branch protections requiring the PR only being merged if it is up-to-date with the target branch. This means you have to push the button in github to merge in target and if any conflicts occur the PR is flagged and can't be completed without user involvement. This would also give reviewers an opportunity to review the merge commit if necessary. 3. With GitHub status checks and branch policies correctly configured the builds are re-run every time the target branch changes. This means that if you have confidence in your PR gates catching most practical merge errors (at least the ones the submitter would catch) you have avoided this issue. This is why the PR builds check every thing in the tree rather than just the incoming patch. Again, this ask was not to create a lazy process or lower the quality of the code tree. If there are legitimate gaps that a squash merge workflows creates, I am interested in finding solutions. For example, the DCO requirement would need to be addressed. But we can only start those conversations if we can get aligned on the idea. Thanks Sean On 5/20/2020 10:05 AM, Laszlo Ersek wrote: > On 05/19/20 23:02, Desimone, Nathaniel L wrote: > >> Of course, there may be other patch series that would be logical to >> squash, especially if the author has not been careful to maintain >> bisectability. For example, I think of some patch series went a >> little overboard and could have been done in maybe 1-2 patches >> instead of 8-10. I would be happy to compromise with you and say that >> squashes can be done in circumstances where both the maintainer and >> the author agree to it. > > Important distinction: > > (a) "squashing patches" is a 100% valid operation that some situations > fully justifiedly call for. Maintainers may ask for it, and contributors > may use it with or without being asked, if the situation calls for it. > > (b) "squashing patches *on merge*" is intolerable. > > The difference is whether there is a final human review for the > *post-squash* state before the merge occurs. > > The valid case is when the contributor squashes some patches, resubmits > the review/pull request, the reviewer approves the *complete* work > (after performing another review, which may of course be incremental in > nature), and then the series is merged exactly as it was submitted. > > The invalid case (squash on merge) is when the reviewer checks / > approves the series when it still contains incremental fixes as > broken-out patches, then squashes some patches (in the worst case: all > patches into one), and then merges the result. In this (invalid) case, > the complete work, in its final state (in the way it's going to land in > the git history) has not been reviewed by either submitter or reviewer, > incrementally or otherwise. This is why squash on merge is intolerable: > it places a sequence of commits into the git history that has never been > reviewed *verbatim* by either submitter or reviewer. It's a "blind > merge", to make up another term for illustration > > Squashing is a 100% valid tool, I use it all the time. Squash-on-merge > is a catastrophic process failure. > > Thanks > Laszlo > > > >