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


Sean
 

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

Join rfc@edk2.groups.io to automatically receive all group messages.