From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web08.126.1624477504544191865 for ; Wed, 23 Jun 2021 12:45:04 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JgbDanVo; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1624477503; 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=cbSGdWIRgOptItFzvqabXyIjUu9VV1pTLyOnS9N9DFg=; b=JgbDanVohsyKbqqSAZ4x49dFeuiQiiwV6XXpzyecn4+/Kco3oz7gX+SXtXERXBfSCcjiH+ yNUpsEFlfY3KZloj5XlR4D34Bin7WaPa5+jPVHmLABqZS2SCOcbFfeoxp0vbYipcq1w5H1 Ic77xhTAAz+BRoINXNqh+/bx4WOUSDw= 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-498-bmH5ZtLbPgy6Yy2Hqc96uA-1; Wed, 23 Jun 2021 15:44:57 -0400 X-MC-Unique: bmH5ZtLbPgy6Yy2Hqc96uA-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 2F5E4800D55; Wed, 23 Jun 2021 19:44:56 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-37.ams2.redhat.com [10.36.112.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id DBE3860583; Wed, 23 Jun 2021 19:44:53 +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> From: "Laszlo Ersek" Message-ID: <9ca73814-77fa-121e-0c8b-25e050e0a880@redhat.com> Date: Wed, 23 Jun 2021 21:44:52 +0200 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 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/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