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

Wed Jun 23 15:16:40 UTC 2021

On 06/22/21 17:38, Kinney, Michael D wrote:
> Hi Laszlo,
>
> I am trying the following configuration that is very conservative:
>
>     actions:
>       queue:
>         method: rebase
>         rebase_fallback: none
>         name: default
>
> The auto rebase only attempts a strict rebase.  If that attempt at a
> strict rebase fails then it will show that there is a conflict that
> the developer must take care of.
>
> I believe any combination of 2 PRs that have overlapping diff stat
> should fail a strict rebase.  The following link describes the method
> and rebase_fallback settings in the queue command.
>
> 	https://docs.mergify.io/actions/queue/#id2
>
> I would be more concerned if we used a method of merge or a
> rebase_fallback of merge.
>
> Are there examples you can think of where the diff stat overlap and
> the strict rebase will succeed?

I've read the strict rebase definition and the above link in the mergify
documentation, but I'm none the wiser.

Consider the following test case (with master @ 7471751a4d81):

  git checkout -b b1 master
  git am b1.patch           # attached
  git checkout -b b2 master
  git am b2.patch           # attached
  git branch b2-rebase b2
  git rebase b1 b2-rebase

Locally, this produces the following message for me:

> First, rewinding head to replay your work on top of it...
> Applying: world
> Using index info to reconstruct a base tree...
> M       ReadMe.rst
> Falling back to patching base and 3-way merge...
> Auto-merging ReadMe.rst

The rebase succeeds and produces the expected result, but that result is
*exactly* what a human should review.

I don't know if mergify catches the above. While the rebase succeeds
locally, it should not succeed in mergify.

Using the "git rebase -i" (interactive) command, which uses a different
rebase backend (based on git-cherry-pick, not on git-am), and specifying
a single "pick" command, the rebase still succeeds; this time without
producing any diagnostic messages even. So from an auto-rebase
perspective, it's even less desirable.

Thanks
Laszlo

>
> Another option to consider is to define an additional 'auto-rebase' label that is
> off by default to enable the auto rebase feature.  By default the PR must be synced
> with head when submitted.  Only if a maintainer sets the 'auto-rebase' label will
> an auto-rebase be attempted.
>
> I also want to make it easy for non-maintainers to submit PRs and get CI test results.
> So auto rebase may be useful for that use case.  Perhaps the 'auto-rebase' label
> can be considered when the 'push' label is also set.
>
> Thanks,
>
> Mike
>
>> -----Original Message-----
>> From: Laszlo Ersek <lersek at redhat.com>
>> Sent: Tuesday, June 22, 2021 8:17 AM
>> 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/17/21 23:53, Kinney, Michael D wrote:
>>> Hi Sean,
>>>
>>> Mergify had added a queue feature to handle the rebases automatically and make sure
>>> CI passes in the order that the PRs are being applied to the base branch.
>>
>> I'm opposed to *unconditional* auto-rebase.
>>
>> On one hand, it is indeed unreasonable to require a human to manually
>> rebase a "ShellPkg/Application/AcpiViewApp" series just because a series
>> for "SecurityPkg/FvReportPei" was merged a bit earlier. In other words,
>> merge requests for unrelated modules should not block each other.
>>
>> On the other hand, auto-rebase is a bad idea if both series modify at
>> least one module in common (especially if both series modify at least
>> one *file* in common). In case there is a contextual conflict, even if
>> the conflict can be auto-resolved, and even if that resolution
>> *compiles*, it has to be reviewed by a human first.
>>
>> I regularly use the git-range-diff command for this.
>>
>> At Red Hat we've seen obscure bugs due to silent mis-merges (not in edk2
>> -- in different packages); such issues are difficult to debug.
>>
>> Bisectability helps for sure, but only if the community treats
>> bisectability with high priority in the first place. (That is, if every
>> contributor builds their patch set at every stage, before submitting it
>> for review.)
>>
>> Can we restrict the auto-rebase feature to such merge requests whose
>> cumulative diffstats do not intersect?
>>
>> Thanks,
>> Laszlo

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: b1.patch
Type: text/x-patch
Size: 583 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210623/0ed023d4/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: b2.patch
Type: text/x-patch
Size: 589 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210623/0ed023d4/attachment-0001.bin>