From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ma1-aaemail-dr-lapp02.apple.com (ma1-aaemail-dr-lapp02.apple.com [17.171.2.68]) by mx.groups.io with SMTP id smtpd.web10.6637.1590116407048836217 for ; Thu, 21 May 2020 20:00:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@apple.com header.s=20180706 header.b=rB8USy7X; spf=pass (domain: apple.com, ip: 17.171.2.68, mailfrom: afish@apple.com) Received: from pps.filterd (ma1-aaemail-dr-lapp02.apple.com [127.0.0.1]) by ma1-aaemail-dr-lapp02.apple.com (8.16.0.42/8.16.0.42) with SMTP id 04M304xW016833; Thu, 21 May 2020 20:00:05 -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=esBvCvuEW9b5VQz5hOU8Am+BqLMJLn2yzvaDF9fDoNY=; b=rB8USy7XFXE8ZAfOFvhEmvrUBJEV4+pidIpgx7C5mv/N11PekjWQ+GCIIqFRupscPtA8 /BX+Ynd6svRHzbygDV3rPHsf9B4XYifZBc5pX/Et/+kt4vrIEHSIcAOBS/AcABNrfpCt B2jjItvFnWLxzdzRzjhsM/cKXwLDXEgC/YI44yEh0hpOdgzOLb/cCszI+evwOa3RpcML 2ly9S/Q/52AEmPvXEANdlqEi06qc6qSiCAPIl1au6BqfZTnmbBCK6DmTH5iEGX4Y/DtU 03UeelJYUGGvLFoJxjaj9eFR8jC+JICH7BqzG+5OpJqDaT6JR6D8s5R5RqK/KoXS8VZU bA== Received: from rn-mailsvcp-mta-lapp01.rno.apple.com (rn-mailsvcp-mta-lapp01.rno.apple.com [10.225.203.149]) by ma1-aaemail-dr-lapp02.apple.com with ESMTP id 312cuup1yv-4 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NO); Thu, 21 May 2020 20:00:05 -0700 Received: from rn-mailsvcp-mmp-lapp01.rno.apple.com (rn-mailsvcp-mmp-lapp01.rno.apple.com [17.179.253.14]) by rn-mailsvcp-mta-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPS id <0QAP010OPPNW4K80@rn-mailsvcp-mta-lapp01.rno.apple.com>; Thu, 21 May 2020 19:59:56 -0700 (PDT) Received: from process_milters-daemon.rn-mailsvcp-mmp-lapp01.rno.apple.com by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) id <0QAP00M00O7NRC00@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Thu, 21 May 2020 19:59:56 -0700 (PDT) X-Va-A: X-Va-T-CD: a896650d2b2ee8463fb615bccff4df55 X-Va-E-CD: ff21abde8ed1c0764e3ec6fb23700577 X-Va-R-CD: 4e3bd3f1c67f42e8057338bddf30a448 X-Va-CD: 0 X-Va-ID: d0b1799a-12e2-481c-b241-8f88ef43f6ef X-V-A: X-V-T-CD: a896650d2b2ee8463fb615bccff4df55 X-V-E-CD: ff21abde8ed1c0764e3ec6fb23700577 X-V-R-CD: 4e3bd3f1c67f42e8057338bddf30a448 X-V-CD: 0 X-V-ID: a04f63e3-6a85-4ba0-94d6-e9b05ea9a650 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.44.50] (unknown [17.235.44.50]) by rn-mailsvcp-mmp-lapp01.rno.apple.com (Oracle Communications Messaging Server 8.1.0.5.20200312 64bit (built Mar 12 2020)) with ESMTPSA id <0QAP00IDGPNVJ800@rn-mailsvcp-mmp-lapp01.rno.apple.com>; Thu, 21 May 2020 19:59:56 -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 19:59:54 -0700 Cc: devel@edk2.groups.io, spbrogan@outlook.com, rfc@edk2.groups.io, nathaniel.l.desimone@intel.com, "bret.barkelew@microsoft.com" , Mike Kinney , "Leif Lindholm (Nuvia address)" Message-id: References: To: Laszlo Ersek 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 21, 2020, at 6:30 AM, Laszlo Ersek wrote: >=20 > On 05/20/20 00:25, Sean wrote: >> On 5/19/2020 1:41 PM, Laszlo Ersek wrote: >=20 >>> 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. =46rom 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. >>=20 >>=20 >> I have trouble with this line of thinking. The maintainers are and >> should be considered the representatives of this code base. They >> have a vested interest to enable this repository to work for them. = If >> they really are viewed as "sloppy" or "lazy" then we are destined to >> fail anyway. >=20 > You put it very well. "They have a vested interest to enable this > repository to work for them." Key part being "*for them*". >=20 > Core maintainers are responsible for making this repository work for a > lot larger camp than just themselves. Even if squash-on-merge = satisfied > the requirements that core maintainers presented, squash-on-merge = would > still hurt the larger community that depends on those packages. >=20 > The core-consumer community may not necessarily participate in the > day-to-day maintenance of the core packages, but they do report bugs = and > even contributes bugfixes / occasional features, when their particular > use cases require those actions. >=20 > And squash-on-merge hurts those activities, down the road, because the > git history is instrumental to analyzing and learning the code base. >=20 > For example, the question "why do we call this function here?" > immediately leads to running "git blame" (possibly a series of = git-blame > commands, to navigate past code movements and such). In the end > git-blame leads to a particular commit, and that commit is supposed to > answer the question. If the commit is huge (e.g. a squash of an entire > feature), then the question is not answered, and git-blame has been > rendered useless. >=20 >=20 >> Nothing in my statement of "don't exclude squash merge workflow" >> requested that we allow a PR to be squashed into a single commit that >> you believe should be a patch series. >=20 > If the button is there, maintainers will click it even in cases when > they shouldn't, and I won't be able to catch them. The result will not > necessarily hurt the maintainer (not at once, anyway), but it will = harm > others that investigate the git history afterwards -- possibly years > later. >=20 > I can't watch all CoreFoobarPkg pull requests on github to prevent = this. > On the other hand, I can, and do, monitor the edk2-devel list for > seriously mis-organized patch sets, especially for core packages where > I've formed an "I had better watch out for this core package" > impression. >=20 > I have made requests under core patch sets where I was mostly = unfamiliar > with the technical subject *for the time being*, asking just for > improvements to the granularity of the series. Knowing the improved > granularity might very well help me *in the future*. >=20 >=20 > The mailing list equivalent of "squash-on-merge" would be the = following: >=20 > - contributor posts v1 with patches 1/5 .. 5/5 (for example), >=20 > - reviewer requests updates A, B, and C, >=20 > - contributor posts (in response to the v1 blurb, i.e. 0/5) further > patches 6/8, 7/8, 8/8 >=20 > - reviewer checks the new patches and approves them, functionally, >=20 > - maintainer says "OK let me merge this", >=20 > - maintainer applies the patches (all 8 of them) from the list, on a > local branch, >=20 > - maintainer runs a git rebase squashing the whole thing into a single > patch, >=20 > - maintainer does *not* review the result, >=20 > - maintainer opens a PR with the resultant single patch, >=20 > - CI passes, >=20 > - the patch is merged. >=20 >=20 > With the list-based process, the disaster in the last step is = mitigated > in at least three spots: >=20 > - All subscribers have a reasonably good chance to notice and = intervene > when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to > the v1 blurb, clearly with an intent to squash. >=20 > - Because the maintainer has to do *extra work* for the squashing, the > natural laziness of the maintainer works *against* the disaster. Thus > he or she will likely not perform the local rebase & squash. Instead > they will ask the contributor to perform a *fine-grained* squash = (i.e. > squash each fixup into the one original patch where the fixup > belongs), and to submit a v2 series. >=20 > - If someone interested in the git history catches (after the fact) = that > the maintainer merged a significantly different patch set from what > had been posted and reviewed, they can raise a stern complaint on the > list, and next time the maintainer will now better. >=20 > (This is not a theoretical option; I low-key follow both the list > traffic and the new commits in the git history (whenever I pull). In = the > past I had reported several patch application violations (mismanaged > feedback tags, intrusive updates post-review, etc). Nowadays it's = gotten > quite OK, thankfully, and I'm terrified of losing those improvements.) >=20 >=20 > If we just plaster a huge squash-on-merge button or checkbox over the > web UI, it *will* be abused (maintainer laziness will work *towards* = the > disaster), with only a microscopic chance for me to prevent the abuse. >=20 > It's not that "I believe" that this or that *particular* series should > not be squashed. "Not squashing" is not the exception but the rule. = The > *default* approach is that the submitter incorporates incremental = fixes > into the series at the right stages, they maintain a proper series > structure over the iterations, and they propose revised versions of = the > series in full. Squashing is the exception; for example one reason is, > "if you separate these changes from each other, then the tree will not > build in the middle; they belong together, please squash them, and > resubmit for review". >=20 >=20 >> I do think those rules will need to be defined but that is needed >> today anyway. >=20 > Rules are only as good as their enforcement is. >=20 In my work world we require code review by a manager and that is the de = facto enforcement mechanism. Basically there is always an owner to make = sure the process was followed :) Also in our world the squash is a developer choice. But we do have tools = that insert the Bugzilla number in all the commits of the series, assist = with the squash, etc.=20 > The question is not how nice it is to use squash-on-merge in the > minuscule set of situations when it might be justified; the question = is > how difficult it would be to prevent the inevitable abuses. >=20 > The list lets me advocate for proper git history hygiene reasonably > efficiently (although I still miss a bunch of warts due to lack of > capacity). With the squash-on-merge button or checkbox, the flood = gates > would fly open. I won't stand for that (not as a steward anyway). >=20 > I think our world views differ fundamentally. I value the git history > *way* above my own comfort, and everyone else's (accounting for both > contributor and day-to-day maintainer roles). I guess you prefer the > reciprocal of that ratio. >=20 I'd also point out that the processes you chose kind of defines your = quanta of work. It is likely you would be willing to tackle a really big = change as a large patch set, that you would likely break up into = multiple PRs in a squash on commit world. In a squash on commit world = you also might break a Bugzilla (BZ) up into dependent BZs, a tree of = BZs. That might sound crazy, but when you work on a bigger project and = there are BZs for EFI, T2, macOS, the Installer, and the RecoveryOS for = a customer visible feature this tree of BZ might be familiar and make = sense to you.=20 But I think the real argument for consistency is we have a rich git = history that has value. We have made resource tradeoffs to have that = rich git history so to me it makes the most sense, for these project, to = try to preserve our past investment in git history.=20 Thanks, Andrew Fish > Thanks, > Laszlo >=20