From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.1619.1589994351745886201 for ; Wed, 20 May 2020 10:05:52 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=HjwrsSD8; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589994350; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=yBaikWBXKdVTgyao8BGniAviombyNft0yMR/bOMLoFE=; b=HjwrsSD8rb7naRam3LwJ2x+87l+YbPyNczsrz2qo6DT/1G2ugzkB1djSMwOG2beMkyoFWm +xew7DnR0B9W1MJzw+sbwgWFviRO1SabvnZuqXeksri7MCwo+2tP3jJkA1Z8jH3rwDk0Gf kLyMvWEhR/6PpeeSFg29s5MFdnWnX48= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-402-50u-FYp7NYqBzOMT3BF6QA-1; Wed, 20 May 2020 13:05:49 -0400 X-MC-Unique: 50u-FYp7NYqBzOMT3BF6QA-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id AFEAD1005510; Wed, 20 May 2020 17:05:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-121.ams2.redhat.com [10.36.115.121]) by smtp.corp.redhat.com (Postfix) with ESMTP id 1CEE46AD16; Wed, 20 May 2020 17:05:45 +0000 (UTC) Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: "Desimone, Nathaniel L" , "rfc@edk2.groups.io" , "bret.barkelew@microsoft.com" , "devel@edk2.groups.io" , "spbrogan@outlook.com" , "Kinney, Michael D" References: <1DF48A10-F119-4FAE-9963-E95A48D82648@intel.com> From: "Laszlo Ersek" Message-ID: <2d42ff42-e028-96aa-6397-284f730d5dab@redhat.com> Date: Wed, 20 May 2020 19:05:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <1DF48A10-F119-4FAE-9963-E95A48D82648@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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