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

Thu Jun 24 01:20:31 UTC 2021

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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of gaoliming
> Sent: Wednesday, June 23, 2021 6:10 PM
> To: devel at edk2.groups.io; Kinney, Michael D <michael.d.kinney at intel.com>; lersek at redhat.com; spbrogan at outlook.com;
> ardb at kernel.org
> Cc: 'Peter Grehan' <grehan at freebsd.org>; 'Ard Biesheuvel' <ardb+tianocore at kernel.org>; Justen, Jordan L
> <jordan.l.justen at intel.com>; 'Sean Brogan' <sean.brogan at microsoft.com>; 'Rebecca Cran' <rebecca at 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 at edk2.groups.io <devel at edk2.groups.io> 代表 Michael D
> > Kinney
> > 发送时间: 2021年6月24日 6:08
> > 收件人: devel at edk2.groups.io; lersek at redhat.com; spbrogan at outlook.com;
> > ardb at kernel.org; Kinney, Michael D <michael.d.kinney at intel.com>
> > 抄送: Peter Grehan <grehan at freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore at kernel.org>; Justen, Jordan L <jordan.l.justen at intel.com>;
> > Sean Brogan <sean.brogan at microsoft.com>; Rebecca Cran
> > <rebecca at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > > Sent: Wednesday, June 23, 2021 12:45 PM
> > > To: Kinney, Michael D <michael.d.kinney at intel.com>;
> > devel at edk2.groups.io; spbrogan at outlook.com; ardb at kernel.org
> > > Cc: Peter Grehan <grehan at freebsd.org>; Ard Biesheuvel
> > <ardb+tianocore at kernel.org>; Justen, Jordan L
> > > <jordan.l.justen at intel.com>; Sean Brogan <sean.brogan at microsoft.com>;
> > Rebecca Cran <rebecca at 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
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
> >
> 
> 
> 
> 
> 
> 
> 

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#77027): https://edk2.groups.io/g/devel/message/77027
Mute This Topic: https://groups.io/mt/83751887/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-