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.web10.9791.1589533686921413397 for ; Fri, 15 May 2020 02:08:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=aTtPWTia; 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=1589533685; 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=ji3HagUJ8z+QHzKra9WsHSD/mmxdbEKT63FIEvcv0Vc=; b=aTtPWTiaPzeG8kNQh8665MyPHaKDnYdg1uQoKJgdO0vFG5mDS/lZkNBqnWRFJAGTK/t5Eo 1/S2KsODs6RDuoNgjNEcSyqyUJ5yCcMzWQbHkXPJPezL7GwDmGUDILuOU+A9g2ypeOD/Me 3a8kb6Tjc10XOYuoH4c28siZrr6MAcY= 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-308-dI0UQJ1RMPuySJNCMvxL_Q-1; Fri, 15 May 2020 05:08:00 -0400 X-MC-Unique: dI0UQJ1RMPuySJNCMvxL_Q-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 1F4B41054F99; Fri, 15 May 2020 09:07:54 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-113-220.ams2.redhat.com [10.36.113.220]) by smtp.corp.redhat.com (Postfix) with ESMTP id 22B45620AF; Fri, 15 May 2020 09:07:51 +0000 (UTC) Subject: Re: [EXTERNAL] Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process To: rfc@edk2.groups.io, bret.barkelew@microsoft.com, "devel@edk2.groups.io" , "Kinney, Michael D" References: <8389d6a6-aaf5-3c0e-904f-84f814c9d385@redhat.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 15 May 2020 11:07:51 +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.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: 8bit On 05/15/20 06:49, Bret Barkelew via groups.io wrote: > I would far prefer the approach of individual PRs for commits to > allow for the squash flexibility (and is the strategy I think I would > pursue with my PRs). For example, the VarPol PR would be broken up > into 9 PRs for each final commit, and we can get them in one by one. > Ideally, each one would be a small back and forth and then in. If it > had been done that way to begin with, it would be over in a week and > a half or so, rather than the multiple months that we’re now verging > on. This differs extremely from how we've been working on edk2-devel (or from how any git-based project works that I've ever been involved with). And I think the above workflow is out of scope, for migrating the edk2 process to github. Again, the structuring of a patch series is a primary trait. Iterating only on individual patches does not allow for the reordering / restructuring of the patch series (dropping patches, reordering patches, inserting patches, moving hunks between patches). It's common that the necessity to revise an earlier patch emerges while reworking a later patch. For instance, the git-rebase(1) manual dedicates a separate section to "splitting commits". In the initial evaluation of "web forges", Phabricator was one of the "contestants". Phabricator didn't support the "patch series" concept at all, it only supported review requests for individual patches, and it supported setting up dependencies between them. So, for example, a 27-patch series would require 27 submissions and 26 dependencies. Lacking support for the patch series concept was an immediate deal breaker with Phabricator. The longest patch series I've ever submitted to edk2-devel had 58 patches. It was SMM enablement for OVMF. It went from v1 to v5 (v5 was merged), and the patch count varied significantly: v1: 58 patches (25 Jul 2015) v2: 41 patches ( 9 Oct 2015) v3: 52 patches (15 Oct 2015) v4: 41 patches ( 3 Nov 2015) v5: 33 patches (27 Nov 2015) (The significant drop in the patch count was due to Mike Kinney open sourcing and upstreaming the *real* PiSmmCpuDxeSmm driver (which was huge work in its own right), allowing me to drop the Quark-originated 32-bit-only PiSmmCpuDxeSmm variant, from my series.) The contribution process should make difficult things possible, even if that complicates simple things somewhat. A process that makes simple things simple and difficult things impossible is useless. This is what the Instagram generation seems to be missing. I don't know why the VariablePolicy work took months. I can see the following threads on the list: * [edk2-devel] [PATCH v1 0/9] Add the VariablePolicy feature Fri, 10 Apr 2020 11:36:01 -0700 * [edk2-devel] [PATCH v2 00/12] Add the VariablePolicy feature Mon, 11 May 2020 23:46:23 -0700 I have two sets of comments: (1) It's difficult to tell in retrospect (because the series seem to have been posted with somewhat problematic threading), but the delay apparently came from multiple sources. (1a) Review was slow and spotty. The v1 blurb received some comments in the first week after it was posted. But the rest of the v1 series (the actual patches) received feedback like this: - v1 1/9: no feedback - v1 2/9: 12 days after posting - v1 3/9: 16 days after posting - v1 4/9: no feedback - v1 5/9: no feedback - v1 6/9: no feedback - v1 7/9: no feedback - v1 8/9: no feedback - v1 9/9: no feedback (1b) There was also quite some time between the last response in the v1 thread (Apr 26th, as far as I can see), and the posting of the v2 series (May 11th). (1c) The v2 blurb got almost immediate, and numerous feedback (on the day of posting, and the day after). Regarding the individual patches, they didn't fare too well: - v2 01/12: superficial comment on the day of posting from me (not a designated MdeModulePkg review), on the day of posting; no other feedback thus far - v2 02/12: ditto - v2 03/12: no feedback - v2 04/12: superficial (coding style) comments on the day of posting - v2 05/12: no feedback - v2 06/12: no feedback - v2 07/12: no feedback - v2 08/12: no feedback - v2 09/12: no feedback - v2 10/12: no feedback - v2 11/12: reasonably in-depth review from responsible co-maintainer (yours truly), on the day of posting - v2 12/12: no feedback In total, I don't think the current process takes the blame for the delay. If reviewers don't care (or have no time) now, that problem will not change with the transition to github.com. (2) The VariablePolicy series is actually a good example that patch series restructuring is important. (2a) The patch count went from 9 (in v1) to 12 (in v2). (2b) And under v2, Liming still pointed out: "To keep each commit build pass, the patch set should first add new library instance, then add the library instance into each platform DSC, last update Variable driver to consume new library instance." Furthermore, I requested enabling the feature in ArmVirtPkg too, and maybe (based on owner feedback) UefiPayloadPkg. Thus, the v2->v3 update will most likely bring about both patch order changes, and an increased patch count. Thanks Laszlo