On 07/07/21 08:00, Kinney, Michael D wrote: > Hi Laszlo, > > I did many experiments and could not get the exact behavior I proposed. > > Here is the best I can do with the behavior of GitHub and Mergify: > > 1) I further simplified Mergify configuration so personal builds ('push' > label not set) are no longer auto closed. Any developer doing a > personal build that wants to abandon the change should manually close > the PR.
This sounds OK to me. > We may need to periodically review PRs that have been open > for an extended period of time and close them and developers can reopen > if that was a mistake. Yes, I've been periodically checking PRs that were stuck open anyway. > > 2) The 'push' label always does the safest possible rebase and merge. > If many PRs are queued at a time, then each one is rebased in turn, > all CI checks are run and if all CI checks pass, then PR is > added with linear history to the base branch. OK. > > 3) If a maintainer wants to manually send a sequence of PRs through > one at a time and review the state before sending the next one, then > I recommending sending each PR as a personal build (always rebase against > latest base branch before submitting PR). Review the commits and status > checks. If the PR looks good and passes all status checks and the state > from GitHub is that the PR is 'up-to-date' with the base branch, then > the maintainer can set the 'push' label and the PR is merged immediately > without re-running the status checks since there have been no changes. So this is the key development. OK. > > If other PRs were merged into the base branch while the PR status checks > were run, then the personal build will show that the branch is out-of-date > with the base branch. The maintainer can do a local rebase and review > the changes and do a forced push of the updated PR. This will trigger the > personal build again. If that passes and the branch is 'up-to-date', then > the 'push' label can be set to immediately merge. Repeat as needed. > > NOTE: If there is a lot of PR activity from other maintainers at the same > time as this manual process is being used, then the manual process may > have to rebase and force push several times until there is a quiet window. Makes sense. > > > The simplified Mergify configuration file is shown below. > > queue_rules: > - name: default > conditions: > - base~=(^main|^master|^stable/) > - label=push > > pull_request_rules: > - name: Automatically merge a PR when all required checks pass and 'push' > label is present > conditions: > - base~=(^main|^master|^stable/) > - label=push > actions: > queue: > method: rebase > rebase_fallback: none > name: default > > - name: Post a comment on a PR that can not be merged due to a merge > conflict > conditions: > - base~=(^main|^master|^stable/) > - conflict > actions: > comment: > message: PR can not be merged due to conflict. Please rebase and > resubmit > > > Let me know if this process of using personal builds and setting 'push' label > if PR is 'up-to-date' is acceptable. I have tested this process with a few > different scenarios in the edk2-codereview repo, and they all worked as > expected. Sounds all fine to me; thank you for working it out! Laszlo > > Best regards, > > Mike > >> -----Original Message----- >> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek >> Sent: Monday, June 28, 2021 5:23 AM >> 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/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 (#77549): https://edk2.groups.io/g/devel/message/77549 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] -=-=-=-=-=-=-=-=-=-=-=-