git: af366d353b84 - main - amd64: implement strlen in assembly

[Ed Maste]

On Tue, 9 Feb 2021 at 08:41, Mateusz Guzik <mjguzik at gmail.com> wrote:
>
> Examples from recent past (all fixed):

I don't think the examples are all good ones - several are failures of
our tooling, not of code review. The limited review effort we have
shouldn't be spent pointing out style(9) violations or build-breaking
typos - our CI should be doing this. Reviewers should be able to
expect that a patch builds and boots, and that it is sufficiently
compliant with coding style, etc.

I agree there's little value in putting changes into review solely
because it's some sort of checklist item. That said, several of the
reviews you identified  had value in refining the change prior to
commit, even if an issue escaped.

> Would a review prevent the problem? That is plausible, but given track
> record indicated above, I would not count on it.

It wouldn't guarantee it, but the review could serve as a rendezvous
point for testing - one of the examples even demonstrates that use. Of
course we'd still need to get someone with an interest in the
associated platform(s) to test, regardless of where/how the patch is
conveyed.

> And now we reach this change.
>
> I did the due diligence in testing (glibc, my test jig, actual
> workloads). Because of this, while I can never claim the routine is
> bug free, I doubt bugs (if any) would be found in a review and this is
> why I did not ask for one.

IMO back to (missing) tooling - eventually pre-commit CI build/test
(whether integrated into Phabricator or elsewhere) should find these
sorts of issues.