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

["Laszlo Ersek" <lersek@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4624 bytes --]

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@redhat.com>
>> Sent: Tuesday, June 22, 2021 8:17 AM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; devel@edk2.groups.io; spbrogan@outlook.com; ardb@kernel.org
>> Cc: Peter Grehan <grehan@freebsd.org>; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
>> <jordan.l.justen@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Rebecca Cran <rebecca@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: b1.patch --]
[-- Type: text/x-patch; name="b1.patch", Size: 583 bytes --]

From 04970caf98f8deeecec2d0bc8808cd8bd34cc530 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 23 Jun 2021 16:19:25 +0200
Subject: [PATCH] hello

---
 ReadMe.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ReadMe.rst b/ReadMe.rst
index 8f5db11281bf..30f890ef532e 100644
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@ -3,6 +3,7 @@ EDK II Project
 ==============
 
 A modern, feature-rich, cross-platform firmware development
+  HELLO
 environment for the UEFI and PI specifications from www.uefi.org.
 
 Core CI Build Status
-- 
2.19.1.3.g30247aa5d201


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: b2.patch --]
[-- Type: text/x-patch; name="b2.patch", Size: 589 bytes --]

From 925e93af76af27526011ad33875fa26cc4cdc6c0 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Wed, 23 Jun 2021 16:19:51 +0200
Subject: [PATCH] world

---
 ReadMe.rst | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ReadMe.rst b/ReadMe.rst
index 8f5db11281bf..cfc364db7406 100644
--- a/ReadMe.rst
+++ b/ReadMe.rst
@@ -4,6 +4,7 @@ EDK II Project
 
 A modern, feature-rich, cross-platform firmware development
 environment for the UEFI and PI specifications from www.uefi.org.
+  WORLD
 
 Core CI Build Status
 --------------------
-- 
2.19.1.3.g30247aa5d201