From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp01.apple.com (ma1-aaemail-dr-lapp01.apple.com [17.171.2.60]) by mx.groups.io with SMTP id smtpd.web10.5872.1590112601982139578 for ; Thu, 21 May 2020 18:56:42 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=geU9vl1n; spf=pass (domain: apple.com, ip: 17.171.2.60, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp01.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp01.apple.com (8.16.0.42/8.16.0.42) with SMTP id 04M1tH4x059170; Thu, 21 May 2020 18:56:40 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=apple.com; h=content-type : mime-version : subject : from : in-reply-to : date : cc : content-transfer-encoding : message-id : references : to; s=20180706; bh=lxqTl404/pS805CzKBBv/auz+7z4kTqMGRhgUaV+pnU=; b=geU9vl1nzLpjOp97z2zPpcB+/sYM7Vd7pST0CG4Pl9cZJBtSgUp2t5H3LztvHhxkkcYN 3gv6HShms+Nw6jN5XLPNjCoa0Pl26YX9FO8AQ8F+0q4phAyesOg+DYAyC0/UXw9m3Nhz MPXoXZtcD1h3TNm/h1FwrG/ZDiSvP5R9H9Yl5qjbJc8NfCmF+DkIl71DAxcYjiv/G+h8 DyelFdUqD2pRuU9B8H6O4elR1K4TlOZjaXGwV3A89raTLRBmRCtIrT32jx0+WoCMhTEl 1t0rPeBJu2TVwG/DApMmO1T/i49swTCUBnfHRe+zdnxlzRSXtx3Ghv0iKa4BCxpeAUTU 4A== Received: from rn-mailsvcp-mta-lapp03.rno.apple.com (rn-mailsvcp-mta-lapp03.rno.apple.com [10.225.203.151]) by ma1-aaemail-dr-lapp01.apple.com with ESMTP id 312eu4xvpk-7 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 21 May 2020 18:56:40 -0700 Received: from rn-mailsvcp-mmp-lapp03.rno.apple.com (rn-mailsvcp-mmp-lapp03.rno.apple.com [17.179.253.16]) by rn-mailsvcp-mta-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPS id <0QAP004BRMQFTV80@rn-mailsvcp-mta-lapp03.rno.apple.com>; Thu, 21 May 2020 18:56:39 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp03.rno.apple.com by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0QAP00F00M0BM700@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Thu, 21 May 2020 18:56:39 -0700 (PDT) X-Va-A: X-Va-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-Va-E-CD: ff21abde8ed1c0764e3ec6fb23700577 X-Va-R-CD: 4e3bd3f1c67f42e8057338bddf30a448 X-Va-CD: 0 X-Va-ID: b9b7a7a4-d4ab-4fb3-8164-5f4c5333d3b3 X-V-A: X-V-T-CD: 678bf7de5df0d9ff994f556fd1b44182 X-V-E-CD: ff21abde8ed1c0764e3ec6fb23700577 X-V-R-CD: 4e3bd3f1c67f42e8057338bddf30a448 X-V-CD: 0 X-V-ID: 9ae660e0-a04c-4da1-845e-a92b94534694 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.676 definitions=2020-05-21_16:2020-05-21,2020-05-21 signatures=0 Received: from [17.235.27.104] (unknown [17.235.27.104]) by rn-mailsvcp-mmp-lapp03.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0QAP0039TMQETM00@rn-mailsvcp-mmp-lapp03.rno.apple.com>; Thu, 21 May 2020 18:56:39 -0700 (PDT) MIME-version: 1.0 (Mac OS X Mail 13.0 \(3594.4.17\)) Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process From: "Andrew Fish" In-reply-to: Date: Thu, 21 May 2020 18:56:38 -0700 Cc: rfc@edk2.groups.io, lersek@redhat.com, "Desimone, Nathaniel L" , "bret.barkelew@microsoft.com" , Mike Kinney Message-id: References: <1DF48A10-F119-4FAE-9963-E95A48D82648@intel.com> <2d42ff42-e028-96aa-6397-284f730d5dab@redhat.com> To: devel@edk2.groups.io, spbrogan@outlook.com X-Mailer: Apple Mail (2.3594.4.17) X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.216,18.0.676 definitions=2020-05-21_16:2020-05-21,2020-05-21 signatures=0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: quoted-printable > On May 20, 2020, at 10:21 AM, Sean wrote: >=20 > When this is done in a PR with branch protections this works out differe= ntly and in my view your concerns are mitigated. >=20 > 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. >=20 I use Stash/Bitbucket but even the UI is biased towards this. There is an = Overview, Diff, and Commits tab. The default diff is the entire PR, you can= go to commits tab and see a list of the commits/patch set and see only the= diffs for those. I'm not sure how github does it.=20 In our world we don't require the squash. We also have a set of command li= ne tools that help automate common operations.=20 Thanks, Andrew Fish > 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 b= utton in github to merge in target and if any conflicts occur the PR is fla= gged and can't be completed without user involvement. This would also give= reviewers an opportunity to review the merge commit if necessary. >=20 > 3. With GitHub status checks and branch policies correctly configured th= e builds are re-run every time the target branch changes. This means that i= f 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. >=20 > 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 requi= rement would need to be addressed. But we can only start those conversatio= ns if we can get aligned on the idea. >=20 > Thanks > Sean >=20 >=20 >=20 >=20 > 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 contributor= s >> 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 bee= n >> 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 >=20 >=20 >=20