Date: Tue, 9 Feb 2021 14:17:03 -0800 From: John Baldwin <jhb@FreeBSD.org> To: Alexey Dokuchaev <danfe@freebsd.org>, Mateusz Guzik <mjguzik@gmail.com> Cc: Jessica Clarke <jrtc27@freebsd.org>, Kevin Bowling <kevin.bowling@kev009.com>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@freebsd.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@freebsd.org> Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly Message-ID: <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@FreeBSD.org> In-Reply-To: <20210209145348.GA70871@FreeBSD.org> References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <CAK7dMtA5rbT6QoCK2HQfzLY2f%2B3kbhdJp0ERmRpc_CVAog%2B6bg@mail.gmail.com> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> <CAGudoHG%2BiKB4q=QoFEadtHsXy9ODX%2BkEHn-hU71F8yGEbzP5MA@mail.gmail.com> <20210209145348.GA70871@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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. -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3e6dfdae-48a8-2d6c-1f42-c92554d74f82>