Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants

["Laszlo Ersek" <lersek@redhat.com>]

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 <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Wednesday, June 23, 2021 12:45 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@bsdio.com>
>> 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
>> <https://man7.org/linux/man-pages/man1/patch.1.html>, 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
>>
>>
>>
>> 
>>
>