Date: Thu, 19 Nov 2020 11:00:26 -0800 From: John Baldwin <jhb@FreeBSD.org> To: Mark Johnston <markj@freebsd.org> Cc: Mateusz Guzik <mjguzik@gmail.com>, 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: <617c0b5a-8295-8c53-ff18-6c7a5ace8a68@FreeBSD.org> In-Reply-To: <X7X6KSF66QZbOZCI@raichu> 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> <X7X6KSF66QZbOZCI@raichu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 11/18/20 8:52 PM, Mark Johnston wrote: > On Wed, Nov 18, 2020 at 03:37:36PM -0800, John Baldwin 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. >> >>>> 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. > > With regard to the future direction of src development, I would propose > a middle ground. Most, if not all, changes should get a Phabricator > review. There should be some minimum period between creation of that > review and a commit. The developer should make some effort to cc active > committers to the code. Some areas of the tree will have stricter > rules, but in general absence of feedback means that it's ok to commit. > Exceptions might apply to build fixes, etc.. This still imposes some > friction on the development process, but I have trouble seeing why > someone's contibution might be gated on their ability to commit at a > moment's notice. Mmm, I think I agree fully with this, and that perhaps the terminology is not clear as different folks have different perceptions of what "mandatory reviews" means perhaps. I know that some projects I work with have a fully "mandatory" requirement (OpenSSL seems to), and others have some exceptions (the "obvious" rule in FSF projects like GDB which the note in the committers guide does include a variant of). It is true though that in practice sometimes changes just time out due to lack of review (the OCF refactor is one of those in which I was able to get some partial review of some pieces or some of the concepts, but not the change as a whole). I do think we want to be in a place where we do at least seek review for most changes with an understanding that a change can "timeout" on review and be merged without always having review approval. > There are some technical issues around Phabricator that would need to be > ironed out before this is really doable. For me, the main one is that > email notifications are all-or-nothing: I would very much like to be > able to get email for each new review without automatically being > subscribed. That would indeed be interesting. In all of the Projects I've worked with using GH or e-mail, it does seem to be all-or-nothing if you are on the notify list. Hmm, looks like you can create a Herald rule to do this btw. Let's see if this works: https://reviews.freebsd.org/H138 -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?617c0b5a-8295-8c53-ff18-6c7a5ace8a68>