Date: Thu, 19 Nov 2020 05:51:32 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r367695 - in head/sys: kern sys Message-ID: <CAGudoHH=tiw=QvKZPkdr-9YPoRfcHoBH7ZhZVKfh5wbEtp2jcw@mail.gmail.com> In-Reply-To: <4f6f6b0a-e71c-a286-507e-abf2522c142c@FreeBSD.org> References: <202011141922.0AEJM2ld055995@repo.freebsd.org> <d3624785-1bc4-43d1-37b6-12a989444505@FreeBSD.org> <CAGudoHGyyJRRmEUuoVkjowO5kRLpVz_VFha3MQwu%2B1P_4MU1kg@mail.gmail.com> <4f6f6b0a-e71c-a286-507e-abf2522c142c@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/19/20, John Baldwin <jhb@freebsd.org> wrote: > On 11/18/20 2:16 PM, Mateusz Guzik wrote: >> On 11/17/20, John Baldwin <jhb@freebsd.org> wrote: >>> On 11/14/20 11:22 AM, Mateusz Guzik wrote: >> Interested parties can check the consumer (also seen in the diff) to >> see this is for consistency. I don't think any comments are warranted >> in the header. > > I did read the consumer, and there didn't seem tremendous value in the > extra line there. > One thing to note is that there are more thing to batch than currently implemented, meaning the established pattern is going get more users. With everyone implementing the same routines, even if nops, it is pretty clear what's going on. In contrast if random calls are missing the reader is left wondering if there is a bug. Either way I see no reason to either comment add a comment in the header nor to remove the nop func. >>> These changes would benefit from review. >>> >> >> I don't think it's feasible to ask for review for everything lest it >> degardes to rubber stamping and I don't think this change warranted >> it, regardless of the cosmetic issues which can always show up. > > That is not consistent with the direction the project is moving. If you > check the commit logs of other high-volume committers such as markj@, > kib@, or myself, you will find that a substantial number of those commits > are reviewed (typically in phabricator) without preventing us from > making useful progress. Also, while the previous core did not mandate > reviews, we moved closer to it when the Pre-Commit Review chapter was > added to the Committer's Guide: > > https://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-guide/pre-commit-review.html > > In the related thread on developers@ we indicated that while weren't yet > making pre-commit review mandatory, we collectively want to move in that > direction. > If you exclude bite-size commits, you will see I am getting reviews for thing which are outside of my work area or which include design choices. Past that other people keep committing without reviews anyway. That said, it may be this patch indeed should have been reviewed, but as it is I don't think this is the case. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHH=tiw=QvKZPkdr-9YPoRfcHoBH7ZhZVKfh5wbEtp2jcw>