Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process

["Sean" <spbrogan@outlook.com>]

Laszlo,

First let me be clear, there is no desire or intent in any of these 
conversations/discussions for anyone to feel so distraught to give up 
this project, let alone someone so active and involved as yourself.

The basis for my perspective goes back to the conversations we had 
numerous years ago about being more inclusive and enabling more of the 
firmware development community to contribute and be involved in this 
project.  In my opinion this project needs help. It needs more 
maintainers, contributors, reviewers, testers, and active users. It 
needs people to write documentation, answer questions, triage bugs, and 
plan release cycles.  Without removing some of today's barriers, support 
will continue to decline and relevancy of this project will decline with it.

One of the first suggestions was to evaluate the contribution and review 
process, looking for places in the current process that are confusing, 
error prone, inefficient, or hard to drive consistently.  It was also 
important to evaluate trends in other successful open source projects. 
 From this the process moved towards evaluating GitHub based pull 
requests for the contribution and review process. That gets us to this 
discussion and in my opinion a slightly larger scope in that we are not 
trying to reproduce the current process using new tools but rather 
adjust the process to address the discussed issues leveraging these tools.

Another request from the community discussions years ago was to add 
testing capabilities to remove manual work and improve quality.  There 
has been a huge effort over the last year to enable a "core" CI system, 
practical/easy to use unit testing capabilities, and most recently 
"platform" CI. These features where developed and enabled to give 
contributors clear expectations for the quality needed for successful 
contribution.  In all of these efforts, my hope has been to enable more 
people to join this project.

Anyway, for what it is worth, I hope this long winded response provides 
some background into my perspective.  I'll respond to other comments 
below.




On 5/19/2020 1:41 PM, Laszlo Ersek wrote:
> (+Leif, +Andrew)
> 
> Sean,
> 
> On 05/19/20 18:54, Sean Brogan wrote:
>> Nate/Laszlo,
>>
>> Regarding a squash merge workflow.  I agree it can be abused and we all
>> have seen terrible examples.  But a patch series that contains 500+ file
>> changes isn't really much better.  Just because it is broken into
>> multiple commits doesn't mean it is the right set of commits.
>>
>> Anyway a squash merge workflow works amazingly well with and is
>> optimized for a web based review and PR processes.  It allows a user to
>> respond to changes, fix issues, learn thru the PR process, all while
>> keeping complete track of the progression.  Then once all "status"
>> checks and reviews are complete, it is squashed into a neat commit for
>> mainline, containing only the relevant data in the message.
>>
>> So, the ask is that we don't exclude squash merge workflows.  Those
>> reviewing the PR can decide what is appropriate for the PR content
>> submitted.  Just as you would request changes to the contents (or
>> ordering) of a commit in a series, if the reviewers don't agree that the
>> PR contents should be in a single commit then obviously it shouldn't be
>> squashed to one.
>>
>> Contributions like spelling fixes, typos, minor bug fixes, documentation
>> additions/fixes, etc all are great examples of PRs that can easily
>> leverage squash merges and this workflow significantly lowers the burden
>> of the contribution and review process.  This workflow is also are much
>> easier for casual or first time contributors.
>>
>> I don't exactly know how we would enable this but I assume we could
>> leverage tags or make it clear in the PR description.  First step is to
>> get alignment that a squash merge workflow, while not appropriate for
>> all contributions, is not something to be excluded.
> 
> the scope for migrating the contribution & review workflows off the
> mailing list and to github.com was set many months ago. That scope does
> not include institutionalized changes to patch set structuring criteria.
> The "git forge" evaluations that we had spent weeks/months on also
> focused on how candidate systems would honor a patch series' structure;
> i.e., how faithful the system would remain to the contributors' and
> reviewers' shared intent, with a specific patch set.

I hope the scope is to build an effective and efficient contribution 
process that helps current contributors deliver more while opening the 
door to the rest of the firmware community.  It should require less 
effort to contribute a change to edk2 than to maintain a downstream 
fork.  Today this is not true.

> 
> Your proposal to "don't exclude squash merge workflows" is a trap. If we
> tolerate that option -- which is obviously the sloppy, and hence more
> convenient, option for some maintainers and some contributors, to the
> detriment of the git history --, then almost every core maintainer will
> use it as frequently as they can. In the long term, that will hurt many
> consumers of the core code. It will limit the ability of people not
> regularly dealing with a particular core module to file a fine-grained
> bug report for that module, maybe even propose a fix. From the
> regression analyst's side, if the bug report starts with "I have a
> bisection log", that's already a good day. And your proposal would
> destroy that option, because maintainers and people in general are
> irrepairably lazy and undisciplined. We cannot post a community member
> shoulder-by-shoulder with every core package reviewer/maintainer to
> prevent the latter from approving a squash-on-merge, out of pure
> laziness. I'm 100% sure the "option" to squash-on-merge would
> *immediately* be abused for a lot more than just "typo fixes". There
> isn't enough manpower to watch the watchers, so "no squash-on-merge"
> needs to be a general rule.


I have trouble with this line of thinking. The maintainers are and 
should be considered the representatives of this code base.   They have 
a vested interest to enable this repository to work for them.  If they 
really are viewed as "sloppy" or "lazy" then we are destined to fail 
anyway.

Nothing in my statement of "don't exclude squash merge workflow" 
requested that we allow a PR to be squashed into a single commit that 
you believe should be a patch series. I do think those rules will need 
to be defined but that is needed today anyway.


> 
> I'm very sad that you're trying to wiggle such a crucial and intrusive
> workflow change into the scope of this transition. 

Not "trying to wiggle" anything, just focused on providing feedback and 
hoping to help develop an efficient and effective process using the 
tools available. See intro paragraph.

> Every time
> squash-on-merge has come up over the years (regardless of this
> transition), we've labeled it as one thing never to do, because it
> destroys information (and/or even encourages not *creating* that
> historical information in the first place, which is of course important
> in reality).
> 

You may have labelled it that way but given the wide spread use of this 
practice and my own great experiences enabling a broad team with mixed 
backgrounds using this practice, I personally haven't.  This community 
is a quiet one and I believe instead of speaking up, members just choose 
not to get involved.


> Well, anyway, here's my feedback: if squash-on-merge is permitted in
> edk2 or in basetools (or in any other external repository that's a hard
> requirement for building edk2), that's a deal breaker for me, and I'll
> hand in my resignation as a steward.
> 
> Maybe you'd consider that a win, I don't know -- but I couldn't remain a
> steward with a straight face after failing to protect what I consider
> one of the core values of open source / distributed development.
> 
> Thanks,
> Laszlo
> 

Thanks
Sean