From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web11.10329.1624883007536706555 for ; Mon, 28 Jun 2021 05:23:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=c4/7UCes; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624883006; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=YP2FMOeToHyi9FByi6kjRkHdk+/2xXKLqGdn/w7dFzM=; b=c4/7UCesZ+B5ZAQp39mM/y5mEZnIGH9JHOLZ4RTT+FG5swNj2itjmu7KzJpRQ0IQGOBZTv fAJCAQYFYgWcqEcqthWqTsixthbOgNN4BZbFPHuZQLX6li9+vfZ5sSnySv1G9ZhK/nGjLx jBvPgBhUXtCKNkM7v+OhJEc1zEFpgQ8= 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-143-WJL-__wLMWKZtYtvchF6TQ-1; Mon, 28 Jun 2021 08:23:23 -0400 X-MC-Unique: WJL-__wLMWKZtYtvchF6TQ-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 66F6B19611BF; Mon, 28 Jun 2021 12:23:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-81.ams2.redhat.com [10.36.114.81]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 3B6C55D6AD; Mon, 28 Jun 2021 12:23:09 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants To: "Kinney, Michael D" , "devel@edk2.groups.io" , "spbrogan@outlook.com" , "ardb@kernel.org" Cc: Peter Grehan , Ard Biesheuvel , "Justen, Jordan L" , Sean Brogan , Rebecca Cran References: <20210612204340.52290-1-rebecca@bsdio.com> <8539c5bc-0092-0c48-7555-76dafcfe18ba@redhat.com> <6e8381ff-d7bc-655e-4794-3ffa0b548ac3@redhat.com> <9ca73814-77fa-121e-0c8b-25e050e0a880@redhat.com> From: "Laszlo Ersek" Message-ID: <74bf0a61-7da7-4061-d591-c8eadff41889@redhat.com> Date: Mon, 28 Jun 2021 14:23:07 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/24/21 00:07, Kinney, Michael D wrote: > Hi Laszlo, > > I understand your point. > > I am trying to balance the ease of use for developers, reducing overhead for maintainers, and > prevent bad commits. > > I think you are saying that you want to make sure a maintainer carefully reviews changes > across multiple PRs that are in the same area of code. The CI checks will of course make > sure the code builds and passes the basic boot tests, but those tests do not have full > coverage so an interaction issue between two PRs that pass build and boot but have > unintended behavior side effects are what require detailed manual review. > > I am going to remove the auto-rebase by default and add a optional label that can > be set by a maintainer to enable auto-rebase. If a maintainer is confident that > a set of PRs being submitted at the same time with the 'push' label are independent, > then the maintainer can also set 'auto-rebase'. If they are not confident, then > they can send PRs one at a time with only 'push' label and manually rebase each > additional PR and review the manual rebase to make sure there are no unintended > side effects. Sounds great, thank you! Laszlo > > Any objections to this direction? > > Thanks, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of Laszlo Ersek >> Sent: Wednesday, June 23, 2021 12:45 PM >> To: Kinney, Michael D ; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org >> Cc: Peter Grehan ; Ard Biesheuvel ; Justen, Jordan L >> ; Sean Brogan ; Rebecca Cran >> Subject: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants >> >> On 06/23/21 20:44, Kinney, Michael D wrote: >>> >>> Hi Laszlo, >>> >>> Thank you for the test case. >>> >>> I created 2 PRs against edk2-codereview using your patches. >>> I made minor update to commit messages to pass patch check. >>> >>> https://github.com/tianocore/edk2-codereview/pull/18 >>> https://github.com/tianocore/edk2-codereview/pull/19 >>> >>> Found another issue with PatchCheck for the Mergify merge commit and >>> fixed that. >>> >>> Mergify did process #18 and merged it in after passing all CI. Mergify >>> rebased #19 successfully and merged it after passing all CI. I do not >>> think this was your expected result. >> >> Indeed, my "desired" result at least would have been for mergify to >> reject the rebase. >> >>> I looked more closely at the patches you provided. They were not >>> overlapping in the lines of Readme.rst. This is why no merge conflict >>> was detected. >> >> More precisely, a contextual conflict *was* determined between the >> patches, but that conflict was auto-resolved. >> >> This is risky when done in an automated fashion. It is an extremely >> convenient feature of git, when used interactively; that is, when the >> auto-merge (automatic conflict resolution) is semantically verified by a >> human. Git takes away the chore of conflict resolution, presents a >> "likely good" end result, and a human only needs to *look* at the end >> result, not *implement* it. >> >> But that "human look" is exactly what's missing from mergify. >> >> Basically what I'd like for mergify is to turn off automatic conflict >> resolution. >> >> More or less, speaking in terms of the stand-alone "patch" utility >> , my preference is >> to set the "fuzz factor" to zero. >> >> >> One way a human reviews such context differences is with git-range-diff. >> Continuing my previous example commands: >> >> $ git range-diff --color master..b2 b1..b2-rebase >> >> 1: 02dc81e58bd6 ! 1: 2cf39d4b1790 world >> @@ -6,8 +6,8 @@ >> --- a/ReadMe.rst >> +++ b/ReadMe.rst >> @@ >> - >> A modern, feature-rich, cross-platform firmware development >> + HELLO >> environment for the UEFI and PI specifications from www.uefi.org. >> + WORLD >> >> This output shows that the "world" addition is the same (it is identical >> between pre-rebase and post-rebase in the commit), but the context has >> changed. During the rebase, the leading empty line of the context >> disappeared, and a HELLO line in the middle of the leading context >> appeared. >> >> This result may or may not be semantically correct; it needs a human >> decision. What if the original purpose of the "world" patch author was >> to say WORLD but only without HELLO? When they looked at the code, there >> was no HELLO yet. >> >> git-range-diff is very powerful, but reading its output takes some >> getting used to. (Colorization with the "--color" option is basically >> required for understanding; I can't reproduce it in this email, alas.) >> >> I don't want to obsess about this forever, I just want us all to be >> aware that this risk exists. >> >> Thanks, >> Laszlo >> >>> >>> I then created 2 new PRs that added text to the same line # in Readme.rst. >>> >>> https://github.com/tianocore/edk2-codereview/pull/21 >>> https://github.com/tianocore/edk2-codereview/pull/22 >>> >>> PR #21 passed all CI tests and was merged. Mergify then attempted to >>> rebase #22 and got a merge conflict and is still in the open state waiting >>> for the developer to manually handle the merge conflict. >> >> This case is not worrisome; when there is a clear conflict that cannot be auto-resolved, I'm not concerned. >> >> My concern is the sneaky contextual conflict that *appears* auto-resolvable, but is semantically broken. Those things >> exist. >> >> Thanks >> Laszlo >> >> >> >> >> >