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.kin...@intel.com>; > ler...@redhat.com; spbro...@outlook.com; > a...@kernel.org > Cc: 'Peter Grehan' <gre...@freebsd.org>; 'Ard Biesheuvel' > <ardb+tianoc...@kernel.org>; Justen, Jordan L > <jordan.l.jus...@intel.com>; 'Sean Brogan' <sean.bro...@microsoft.com>; > 'Rebecca Cran' <rebe...@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; ler...@redhat.com; spbro...@outlook.com; > > a...@kernel.org; Kinney, Michael D <michael.d.kin...@intel.com> > > 抄送: Peter Grehan <gre...@freebsd.org>; Ard Biesheuvel > > <ardb+tianoc...@kernel.org>; Justen, Jordan L <jordan.l.jus...@intel.com>; > > Sean Brogan <sean.bro...@microsoft.com>; Rebecca Cran > > <rebe...@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.kin...@intel.com>; > > devel@edk2.groups.io; spbro...@outlook.com; a...@kernel.org > > > Cc: Peter Grehan <gre...@freebsd.org>; Ard Biesheuvel > > <ardb+tianoc...@kernel.org>; Justen, Jordan L > > > <jordan.l.jus...@intel.com>; Sean Brogan <sean.bro...@microsoft.com>; > > Rebecca Cran <rebe...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
