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

[Warner Losh]

On Tue, Feb 9, 2021 at 3:17 PM John Baldwin <jhb at freebsd.org> wrote:

> On 2/9/21 6:53 AM, Alexey Dokuchaev wrote:
> > On Tue, Feb 09, 2021 at 02:41:15PM +0100, Mateusz Guzik wrote:
> >> ...
> >> More, if reviews were mandatory, I would expect their quality to go
> >> down even further, making them even less likely to prevent breakage.
> >
> > Exactly that.  In fact, the good reviews are typically coming from
> > people who care.  But those you'll get regardless of whether you've
> > asked for them.
>
> No, that's not quite true.  Committing without asking for any review at
> all is not the same as requesting review and then timing out when it
> doesn't occur.
>
> Also, as has been noted multiple times now, people do point out questions
> that can't easily be fixed post-commit such as too-terse commit logs.
> Those are quite easily caught in review if one makes the effort to ask.
>
> If we want to cherry-pick examples, we can also find examples where
> reviews do find issues pre-commit.  Look at all the back and forth on
> Warner's doc change about libraries and symbol versioning in D28486 for
> an example.
>
> The discussion in D28453 has led to a better approach I still need to
> update the review with that moves the handling of pollable sims one
> layer up.
>
> Rob Wing found an issue I had missed in my bhyve config change (D26035)
> in terms of new warnings from iasl during pre-commit testing.
>
> Kostik posted a possible patch trying to address a PR in D28485 that is
> probably not valid (see my review comments) and thus saved having
> something committed that then had to be reverted.
>
> Kostik's review on D28342 forced me to rework the change to only scan
> segments and not ELF sections since valid ELF executables and DSO's
> aren't required to have section headers.
>
> Review on D27454 led to acclerated AES-GCM for ARMv8 that was targeted
> at KTLS being reimplemented in a more generic fashion that also
> accelerates IPsec and other users of AES-GCM in the kernel.
>
> I could keep going listing changes that benefit from cooperation among
> developers.  However, cooperation does mean one has to be a bit more
> patient and be willing to work on follow-on work while letting review
> feedback come in.  Using tools like git make this fairly easy as you
> can apply fixups to the earlier changes and rebase the follow-on changes
> under active development afterwards.  It is true that you can't get
> meaningful review on all changes (or all aspects of a change), but I
> think the notion that review _never_ helps is not supported by the
> evidence.
>
> The fact that Jess found a bug in the assembly code in question the day
> after it was committed indicates that pre-commit review would have
> been beneficial for this commit.
>

Just to draw back to the main point I was trying to make:

Let's not let the perfect be the enemy of the better. Reviews are better
today than we were last year and even better than two years ago. While we
still have a ways to go, we've gotten better and will continue to get
better as more people give things another try.

Warner