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 <ler...@redhat.com>
> Sent: Wednesday, June 23, 2021 8:17 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/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 <ler...@redhat.com>
> >> Sent: Tuesday, June 22, 2021 8:17 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/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 (#77005): https://edk2.groups.io/g/devel/message/77005
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]
-=-=-=-=-=-=-=-=-=-=-=-
