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

["Michael D Kinney" <michael.d.kinney@intel.com>]

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.

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.

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.

Mike

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Wednesday, June 23, 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/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