Re: [edk2-devel] [edk2-rfc] GitHub Pull Request based Code Review Process
On 05/19/20 23:02, Desimone, Nathaniel L wrote:
Of course, there may be other patch series that would be logical toImportant distinction:
(a) "squashing patches" is a 100% valid operation that some situations
fully justifiedly call for. Maintainers may ask for it, and contributors
may use it with or without being asked, if the situation calls for it.
(b) "squashing patches *on merge*" is intolerable.
The difference is whether there is a final human review for the
*post-squash* state before the merge occurs.
The valid case is when the contributor squashes some patches, resubmits
the review/pull request, the reviewer approves the *complete* work
(after performing another review, which may of course be incremental in
nature), and then the series is merged exactly as it was submitted.
The invalid case (squash on merge) is when the reviewer checks /
approves the series when it still contains incremental fixes as
broken-out patches, then squashes some patches (in the worst case: all
patches into one), and then merges the result. In this (invalid) case,
the complete work, in its final state (in the way it's going to land in
the git history) has not been reviewed by either submitter or reviewer,
incrementally or otherwise. This is why squash on merge is intolerable:
it places a sequence of commits into the git history that has never been
reviewed *verbatim* by either submitter or reviewer. It's a "blind
merge", to make up another term for illustration
Squashing is a 100% valid tool, I use it all the time. Squash-on-merge
is a catastrophic process failure.