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

[gaoliming]

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 (#77026): https://edk2.groups.io/g/devel/message/77026
Mute This Topic: https://groups.io/mt/83751672/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-