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.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 (#77180): https://edk2.groups.io/g/devel/message/77180 Mute This Topic: https://groups.io/mt/83497624/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
