On Tue, Feb 9, 2021 at 3:17 PM John Baldwin <j...@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 _______________________________________________ dev-commits-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/dev-commits-src-all To unsubscribe, send any mail to "dev-commits-src-all-unsubscr...@freebsd.org"
