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

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

Laszlo,

I appreciate the back and forth.  I find email a challenge for this type 
of discussion because it leaves so much to individual interpretation and 
bias.  Anyway Thank you for having the discussion.  I hope others with 
opinions feel empowered to chime in and help this RFC go in the 
direction the community and stewards desire.

I am still in full support of the RFC and am ok to table my concerns 
that changing the tools without adapting the process will lead to a less 
than optimal workflow.  Anyway, I look forward to seeing if the "pull 
request based code review process" can help improve the communities 
collaboration and efficiency.

I have a few additional responses below that will clarify my thoughts 
but hopefully not invoke responses. :)

On 5/21/2020 6:30 AM, Laszlo Ersek wrote:
> On 05/20/20 00:25, Sean wrote:
>> On 5/19/2020 1:41 PM, Laszlo Ersek wrote:
> 
>>> 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.
> 
> You put it very well. "They have a vested interest to enable this
> repository to work for them." Key part being "*for them*".
> 
> Core maintainers are responsible for making this repository work for a
> lot larger camp than just themselves. Even if squash-on-merge satisfied
> the requirements that core maintainers presented, squash-on-merge would
> still hurt the larger community that depends on those packages.
> 
> The core-consumer community may not necessarily participate in the
> day-to-day maintenance of the core packages, but they do report bugs and
> even contributes bugfixes / occasional features, when their particular
> use cases require those actions.
> 
> And squash-on-merge hurts those activities, down the road, because the
> git history is instrumental to analyzing and learning the code base.
> 
> For example, the question "why do we call this function here?"
> immediately leads to running "git blame" (possibly a series of git-blame
> commands, to navigate past code movements and such). In the end
> git-blame leads to a particular commit, and that commit is supposed to
> answer the question. If the commit is huge (e.g. a squash of an entire
> feature), then the question is not answered, and git-blame has been
> rendered useless.
> 
> 
>> 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.
> 
> If the button is there, maintainers will click it even in cases when
> they shouldn't, and I won't be able to catch them. The result will not
> necessarily hurt the maintainer (not at once, anyway), but it will harm
> others that investigate the git history afterwards -- possibly years
> later.
> 
> I can't watch all CoreFoobarPkg pull requests on github to prevent this.
> On the other hand, I can, and do, monitor the edk2-devel list for
> seriously mis-organized patch sets, especially for core packages where
> I've formed an "I had better watch out for this core package"
> impression.
> 
> I have made requests under core patch sets where I was mostly unfamiliar
> with the technical subject *for the time being*, asking just for
> improvements to the granularity of the series. Knowing the improved
> granularity might very well help me *in the future*.
> 
> 
> The mailing list equivalent of "squash-on-merge" would be the following:
> 
> - contributor posts v1 with patches 1/5 .. 5/5 (for example),
> 
> - reviewer requests updates A, B, and C,
> 
> - contributor posts (in response to the v1 blurb, i.e. 0/5) further
>    patches 6/8, 7/8, 8/8
> 
> - reviewer checks the new patches and approves them, functionally,
> 
> - maintainer says "OK let me merge this",
> 
> - maintainer applies the patches (all 8 of them) from the list, on a
>    local branch,
> 
> - maintainer runs a git rebase squashing the whole thing into a single
>    patch,
> 
> - maintainer does *not* review the result,
> 
> - maintainer opens a PR with the resultant single patch,
> 
> - CI passes,
> 
> - the patch is merged.
> 
> 

The above example should not be allowed in any process.
If a contribution was submitted as a patch series with 5 patches 
intentionally, then it would not be a candidate for a squash merge.  The 
squash merge workflow is only acceptable when it is agreed that the end 
result should be 1 patch.


> With the list-based process, the disaster in the last step is mitigated
> in at least three spots:
> 
> - All subscribers have a reasonably good chance to notice and intervene
>    when the incremental fixups 6/8, 7/8, 8/8 are posted as followups to
>    the v1 blurb, clearly with an intent to squash.
> 
> - Because the maintainer has to do *extra work* for the squashing, the
>    natural laziness of the maintainer works *against* the disaster. Thus
>    he or she will likely not perform the local rebase & squash. Instead
>    they will ask the contributor to perform a *fine-grained* squash (i.e.
>    squash each fixup into the one original patch where the fixup
>    belongs), and to submit a v2 series.
> 
> - If someone interested in the git history catches (after the fact) that
>    the maintainer merged a significantly different patch set from what
>    had been posted and reviewed, they can raise a stern complaint on the
>    list, and next time the maintainer will now better.
> 
> (This is not a theoretical option; I low-key follow both the list
> traffic and the new commits in the git history (whenever I pull). In the
> past I had reported several patch application violations (mismanaged
> feedback tags, intrusive updates post-review, etc). Nowadays it's gotten
> quite OK, thankfully, and I'm terrified of losing those improvements.)
> 
> 
> If we just plaster a huge squash-on-merge button or checkbox over the
> web UI, it *will* be abused (maintainer laziness will work *towards* the
> disaster), with only a microscopic chance for me to prevent the abuse.
> 
> It's not that "I believe" that this or that *particular* series should
> not be squashed. "Not squashing" is not the exception but the rule. The
> *default* approach is that the submitter incorporates incremental fixes
> into the series at the right stages, they maintain a proper series
> structure over the iterations, and they propose revised versions of the
> series in full. Squashing is the exception; for example one reason is,
> "if you separate these changes from each other, then the tree will not
> build in the middle; they belong together, please squash them, and
> resubmit for review".
> 
> 
>> I do think those rules will need to be defined but that is needed
>> today anyway.
> 
> Rules are only as good as their enforcement is.
> 
> The question is not how nice it is to use squash-on-merge in the
> minuscule set of situations when it might be justified; the question is
> how difficult it would be to  prevent the inevitable abuses.
> 

At time of writing i don't know any way to enforce this if the 
maintainers can not be relied upon.  Given my strong agreement with 
"Rules are only as good as their enforcement is." I don't see a 
practical way to resolve this and you seem content with the current 
solution.  Thanks for your diligence here.

> The list lets me advocate for proper git history hygiene reasonably
> efficiently (although I still miss a bunch of warts due to lack of
> capacity). With the squash-on-merge button or checkbox, the flood gates
> would fly open. I won't stand for that (not as a steward anyway).
> 
> I think our world views differ fundamentally. I value the git history
> *way* above my own comfort, and everyone else's (accounting for both
> contributor and day-to-day maintainer roles). I guess you prefer the
> reciprocal of that ratio.
> 
> Thanks,
> Laszlo
> 


Thanks
Sean