From owner-dev-commits-src-all@freebsd.org Tue Feb 9 23:36:11 2021 Return-Path: Delivered-To: dev-commits-src-all@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id C89C252F752 for ; Tue, 9 Feb 2021 23:36:11 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qv1-xf2b.google.com (mail-qv1-xf2b.google.com [IPv6:2607:f8b0:4864:20::f2b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DZzmW4tRSz4Xj6 for ; Tue, 9 Feb 2021 23:36:11 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qv1-xf2b.google.com with SMTP id f18so28750qvm.9 for ; Tue, 09 Feb 2021 15:36:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=G7bHsYwj1JEJA/EPcQhvC6M+ZINR6iXPdiazVMQxB/Y=; b=c70ztciz2Yqrpp86oOtKBSJK+Agc7QV/XhoFurVsnYOS0zyfpvjchUQdEOnYBlmdfH 3P9Y78MhR4K4TF2P1+b3aitaSAzo98tX/EyECLNvQ023BEIkyN1LXH9+Hd+U6PXJ9JTd RpbMokFPiH5kMyhNS65FPSlj+EiXWIi7NuNBVyQLYPneTvlSmK4/7ixassb0VHbFVXTR VO13iyqwiybrDQISw1+5G1urXgxwHuczWuI+JmZT52dd5FGOra6O++hVOW3x/TwWQKOo dZkOuNacoqwag8e/b5rX4IkrakkvXHGkEcmIzFPOqQWV/fJ38GdI0TBl7vb6tecw4IDq kCCg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=G7bHsYwj1JEJA/EPcQhvC6M+ZINR6iXPdiazVMQxB/Y=; b=QBohtkHvUprRiFL2zPvFK0K8/FQafzuLwz9ujTT6bN1ZOEnQAXY9L8DI99j6/9bgGY 9iUkfkY23UrdsoqtuvacEylzmO2VBl9MMMkMk8FNC+mpWjkNXE6aQWh2M2jYKvhceG9s ARGrmOA7IU6kVnMkMugexIP7Dp7WTiDLO/yKQBCu9vj67T9DrPyiX+0DhsAUH6hnPcTJ 5wdd0d1aaSvN8G+ENFivVAvGaVeXO+AUVC392q/juanjpa2Fra8ZTpzJtCl79IROXK7p zJkoRWTBhw5NzSlDX8t2J9ebIxtJXwKChDzs39cJIAfJJKeH/NncPcmg46QOazASWzXd xYjQ== X-Gm-Message-State: AOAM533Vapv4SYv2TnteZ8672TJdPj0d7b/tdMBojctcbP+r2ftfq13N F7Xsg30AtMmgJcHnIOBKcgTC2mURLGCVKeau4Akdnw== X-Google-Smtp-Source: ABdhPJxhScIp30j1IFAQG2/W/F8wXsSUgChbOnAodKr056aGpW1fSAUYtIYXLN5BQZyG6uB85RioSNjLNDO/HOGa/14= X-Received: by 2002:a0c:abce:: with SMTP id k14mr204741qvb.23.1612913770760; Tue, 09 Feb 2021 15:36:10 -0800 (PST) MIME-Version: 1.0 References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> <20210209145348.GA70871@FreeBSD.org> <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@FreeBSD.org> In-Reply-To: <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@FreeBSD.org> From: Warner Losh Date: Tue, 9 Feb 2021 16:35:59 -0700 Message-ID: Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly To: John Baldwin Cc: Alexey Dokuchaev , Mateusz Guzik , Jessica Clarke , Kevin Bowling , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" X-Rspamd-Queue-Id: 4DZzmW4tRSz4Xj6 X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.34 X-BeenThere: dev-commits-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commit messages for all branches of the src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Feb 2021 23:36:11 -0000 On Tue, Feb 9, 2021 at 3:17 PM John Baldwin 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