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

["Michael D Kinney" <michael.d.kinney@intel.com>]

Hi,

I have checked in a new Mergify configuration file into edk2-codereview.  Have not had a chance to 
test it yet, but I think it has all the updates that have been discussed:

1) Allows non EDK II Maintainers to open a PR and for an EDK II Maintainers to set labels.
2) Removed status check names from Mergify config and use the status checks listed in branch protections
3) Simplify the number of rules in Mergify config
4) Add support for 'main' branch.
5) Optional support for auto-rebase using 'auto-rebase' label.

https://github.com/tianocore/edk2-codereview/blob/master/.mergify/config.yml

Please try doing some additional tests to see if it behaves as expected.

I will get back to this on Monday or Tuesday next week and will start preparing a 
few code reviews to update the edk2 repo with these changes.

Thanks,

Mike

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Wednesday, June 23, 2021 6:21 PM
> To: devel@edk2.groups.io; gaoliming@byosoft.com.cn; lersek@redhat.com; spbrogan@outlook.com; ardb@kernel.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> 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
> 
> Liming,
> 
> Yes.  I am working on a configuration with will keep the current 'push' behavior
> and add the 'auto-rebase' option.
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of gaoliming
> > Sent: Wednesday, June 23, 2021 6:10 PM
> > To: devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; 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: 回复: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE remnants
> >
> > Mike:
> >   If 'auto-rebase' label is not set, the behavior is still same to current one. Right?
> >   If yes, I agree this proposal. The maintainer can optionally set 'auto-rebase'.
> >
> > Thanks
> > Liming
> > > -----邮件原件-----
> > > 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Michael D
> > > Kinney
> > > 发送时间: 2021年6月24日 6:08
> > > 收件人: devel@edk2.groups.io; lersek@redhat.com; spbrogan@outlook.com;
> > > ardb@kernel.org; Kinney, Michael D <michael.d.kinney@intel.com>
> > > 抄送: 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>
> > > 主题: Re: [edk2-devel] [PATCH] OvmfPkg/Bhyve: clean up TPM_ENABLE
> > > remnants
> > >
> > > 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.
> > >
> > > 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
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> > 
> >