Re: [edk2-devel] [Patch wiki v2] EDK II CI: Update Phase 1 details and admin settings

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

On 12/03/19 22:53, Kinney, Michael D wrote:
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Tuesday, December 3, 2019 9:08 AM
>> To: devel@edk2.groups.io; Kinney, Michael D
>> <michael.d.kinney@intel.com>
>> Cc: Sean Brogan <sean.brogan@microsoft.com>; Bret
>> Barkelew <Bret.Barkelew@microsoft.com>; Gao, Liming
>> <liming.gao@intel.com>; Andrew Fish <afish@apple.com>;
>> Leif Lindholm <leif.lindholm@linaro.org>
>> Subject: Re: [edk2-devel] [Patch wiki v2] EDK II CI:
>> Update Phase 1 details and admin settings

>>> +3) TianoCore EDK II Maintainers Team permissions
>> reduced from 'Write" to "Triage"
>>> +4) EDK II Maintainers must use GitHub pull request
>> with 'push' label to request
>>> +   a branch to be strict rebase merged into
>> `edk2/master`.  If all checks pass,
>>
>> Perhaps replace "strict rebase" with "strict fast-
>> forward"?
> 
> Please look at these descriptions:
> 
>   https://doc.mergify.io/actions.html#git-merge-workflow-and-mergify-equivalent-configuration
> 
> We are currently using strict: true, method: rebase
> 
>   https://github.com/tianocore/edk2/blob/master/.mergify/config.yml
> 
> Let me know if you what is the best way to describe this.
> I am currently using terms for the Mergify settings and not
> the git terminology.

Thanks for the link to the mergify documentation.

The article lists 9 strategies. From these, we can rule out all that do
*not* use

  (on base branch) $ git merge --ff head

as last step. This leaves us with 5 strategies.

Then we can rule out all methods that mention "Squash all commits". This
leaves us with two strategies:

(1)
(on head branch) $ git rebase base
(on base branch) $ git merge --ff head

(2)
(on head branch) $ git merge --no-ff base
(on head branch) # Wait for CI to go green
(on head branch) $ git rebase base
(on base branch) $ git merge --ff head

Among these, (1) looks like the obvious choice to me ("method: rehead").

However, I realize that (1) does not run CI. Which leaves us with (2)
only. And that's what "strict: true, method: rebase" selects in fact.

Note that (2) is basically (1), prefixed with two additional steps, for CI.

Now, what I don't understand in (2) is why it first merges "base" (i.e.
edk2 master) into "head" (i.e. the topic branch), before setting off CI.
Instead of that, I would propose inserting the CI step between the two
steps of (1): as follows:

(*)
(on head branch) $ git rebase base
(on head branch) # Wait for CI to go green
(on base branch) $ git merge --ff head

Either way, given the current palette of strategies, I agree that
"strict: true, method: rebase" is our only choice.

The "strict merge" explanation at
<https://doc.mergify.io/strict-workflow.html#strict-merge> states the
correct *goal*, but I don't think the implementation is ideal. The end
state that we're going to push to the repository consists of the topic
branch rebased on the master branch. But that's not what the "strict
merge" strategy subjects to CI: instead, it merges the (possibly
advanced) master branch temporarily back into the topic branch.

So... in light of this, I think my suggestion to use the "native" git
terminology would actually be incorrect. It would be suggesting a
workflow (*) that mergify does not seem to implement at this time. So
ultimately I agree with your current wording.

May I ask for just one improvement: wherever you use the term "strict
rebase merged", can you please turn that into a link to
<https://doc.mergify.io/actions.html#git-merge-workflow-and-mergify-equivalent-configuration>?

Thank you,
Laszlo