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

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

On 07/07/21 08:00, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> I did many experiments and could not get the exact behavior I proposed.
> 
> Here is the best I can do with the behavior of GitHub and Mergify:
> 
> 1) I further simplified Mergify configuration so personal builds ('push'
>    label not set) are no longer auto closed.  Any developer doing a
>    personal build that wants to abandon the change should manually close
>    the PR.

This sounds OK to me.

>    We may need to periodically review PRs that have been open
>    for an extended period of time and close them and developers can reopen
>    if that was a mistake.

Yes, I've been periodically checking PRs that were stuck open anyway.

> 
> 2) The 'push' label always does the safest possible rebase and merge.
>    If many PRs are queued at a time, then each one is rebased in turn,
>    all CI checks are run and if all CI checks pass, then PR is 
>    added with linear history to the base branch.

OK.

> 
> 3) If a maintainer wants to manually send a sequence of PRs through 
>    one at a time and review the state before sending the next one, then
>    I recommending sending each PR as a personal build (always rebase against
>    latest base branch before submitting PR).  Review the commits and status
>    checks.  If the PR looks good and passes all status checks and the state
>    from GitHub is that the PR is 'up-to-date' with the base branch, then
>    the maintainer can set the 'push' label and the PR is merged immediately
>    without re-running the status checks since there have been no changes.  

So this is the key development. OK.

> 
>    If other PRs were merged into the base branch while the PR status checks
>    were run, then the personal build will show that the branch is out-of-date
>    with the base branch.  The maintainer can do a local rebase and review
>    the changes and do a forced push of the updated PR.  This will trigger the
>    personal build again.  If that passes and the branch is 'up-to-date', then
>    the 'push' label can be set to immediately merge.  Repeat as needed.
> 
>    NOTE: If there is a lot of PR activity from other maintainers at the same
>    time as this manual process is being used, then the manual process may
>    have to rebase and force push several times until there is a quiet window.

Makes sense.

> 
> 
> The simplified Mergify configuration file is shown below.
> 
> queue_rules:
>   - name: default
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - label=push
> 
> pull_request_rules:
>   - name: Automatically merge a PR when all required checks pass and 'push' label is present
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - label=push
>     actions:
>       queue:
>         method: rebase
>         rebase_fallback: none
>         name: default
> 
>   - name: Post a comment on a PR that can not be merged due to a merge conflict
>     conditions:
>       - base~=(^main|^master|^stable/)
>       - conflict
>     actions:
>       comment:
>         message: PR can not be merged due to conflict.  Please rebase and resubmit
> 
> 
> Let me know if this process of using personal builds and setting 'push' label
> if PR is 'up-to-date' is acceptable.  I have tested this process with a few
> different scenarios in the edk2-codereview repo, and they all worked as expected.

Sounds all fine to me; thank you for working it out!
Laszlo


> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
>> Sent: Monday, June 28, 2021 5:23 AM
>> 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/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
>>>>
>>>>
>>>>
>>>>
>>>>
>>>
>>
>>
>>
>> 
>>
>