From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web11.503.1589920871262754720 for ; Tue, 19 May 2020 13:41:11 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Impl7yET; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1589920870; 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=BQm9t3cMpLQqZfL/d6yMFr5xJURV7/zf6KfvYpEuf2g=; b=Impl7yETekm+F4x2/52vSvThgRrv3/uIsWVcjtNG8J0cSBDjvbnhJC0M4vAr6vWEnBKHIh wGgNgpLITsjgcVcZydLeMH1BYniEHBv+W6jojsqDvrCi2gw9b2/pvXEbMEeRK43AfiJgmc F9qG/ugLEhrHJwpYNYXq6pdBMlmytJY= 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-166-8kLZZE1ANoC7bUmPg2b5og-1; Tue, 19 May 2020 16:41:06 -0400 X-MC-Unique: 8kLZZE1ANoC7bUmPg2b5og-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id BF3D7835B8B; Tue, 19 May 2020 20:41:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-149.ams2.redhat.com [10.36.114.149]) by smtp.corp.redhat.com (Postfix) with ESMTP id B977670879; Tue, 19 May 2020 20:41:02 +0000 (UTC) Subject: Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: Sean Brogan , 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: "Laszlo Ersek" Message-ID: Date: Tue, 19 May 2020 22:41:01 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit (+Leif, +Andrew) Sean, On 05/19/20 18:54, Sean Brogan wrote: > Nate/Laszlo, > > Regarding a squash merge workflow.  I agree it can be abused and we all > have seen terrible examples.  But a patch series that contains 500+ file > changes isn't really much better.  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.  It allows a user to > respond to changes, fix issues, learn thru the PR process, all while > keeping complete track of the progression.  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.  Those > reviewing the PR can decide what is appropriate for the PR content > submitted.  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.  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.  First step is to > get alignment that a squash merge workflow, while not appropriate for > all contributions, is not something to be excluded. 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. 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'm very sad that you're trying to wiggle such a crucial and intrusive workflow change into the scope of this transition. 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). 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. 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. Thanks, Laszlo