From owner-dev-commits-src-all@freebsd.org Tue Feb 9 22:17:08 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 2F75452CE53; Tue, 9 Feb 2021 22:17:08 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DZy1J0j9Bz4Rl4; Tue, 9 Feb 2021 22:17:08 +0000 (UTC) (envelope-from jhb@FreeBSD.org) Received: from John-Baldwins-MacBook-Pro.local (unknown [IPv6:2601:648:8681:1cb0:cd55:671f:3576:6456]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: jhb) by smtp.freebsd.org (Postfix) with ESMTPSA id 4344B4977; Tue, 9 Feb 2021 22:17:07 +0000 (UTC) (envelope-from jhb@FreeBSD.org) To: Alexey Dokuchaev , Mateusz Guzik Cc: Jessica Clarke , Kevin Bowling , "src-committers@freebsd.org" , "dev-commits-src-all@freebsd.org" , "dev-commits-src-main@freebsd.org" References: <202102081915.118JFXkJ067892@gitrepo.freebsd.org> <3E64387A-42DD-4470-8893-5B774F19754E@freebsd.org> <8E61EA5C-39D1-49CC-8319-06E9192FF735@freebsd.org> <20210209145348.GA70871@FreeBSD.org> From: John Baldwin Subject: Re: git: af366d353b84 - main - amd64: implement strlen in assembly Message-ID: <3e6dfdae-48a8-2d6c-1f42-c92554d74f82@FreeBSD.org> Date: Tue, 9 Feb 2021 14:17:03 -0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:78.0) Gecko/20100101 Thunderbird/78.7.0 MIME-Version: 1.0 In-Reply-To: <20210209145348.GA70871@FreeBSD.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit 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 22:17:08 -0000 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