Date: Wed, 18 Nov 2020 23:52:57 -0500 From: Mark Johnston <markj@freebsd.org> To: John Baldwin <jhb@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: <X7X6KSF66QZbOZCI@raichu> 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 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. So, I personally don't really like the idea of mandatory reviews. It's a drag, especially for volunteers. It works ok in areas that have active maintainers, but lots of FreeBSD does not. For instance, pretty much every change to sys/vm gets a review, but only by convention. It's not perfect, and in particular it discourages me from making some changes both because getting review is a hassle and because I don't want to spam other VM developers (especially volunteers) with minor things. If all changes require some sort of sign-off, even if it's effectively rubber-stamping, I fear it'll discourage a lot of small, worthwhile contributions while also burning out reviewers. Meanwhile, we had issues for a long time where changes to the CSPRNG code were blocked for lack of review, and we have some problems with the LOCALBASE changes despite review. For better or worse, FreeBSD has no sole technical authority to direct development. More than many other projects, we rely on consensus and more broadly the ability of developers to engage with each other even when their communication styles differ. Since Phabricator was introduced it's been a lot easier to get feedback on patches, and non-committers have a better vehicle to propose patches. No edict was needed for that. On the other hand, FCP doesn't seem to have been successful. I suspect that mailing lists are sufficient for the same purpose that FCP is for; if someone doesn't care to socialize some changes on the lists, it's unlikely that they'll do so in a FCP, and because FreeBSD has no one who can really mandate development process, FCP doesn't buy us much. I tried adding RELNOTES so that it's easier for us to advertise our work to users; it's worked so-so, and I suspect that a lot of developers don't care about advertising their work or just don't know about RELNOTES. Our development processes end up being largely dictated by social norms, which are influenced by the most prolific committers. Mateusz is making a lot of commits to the kernel without any review. Most other committers would try to get a review for similar changes. For what it's worth I personally trust Mateusz to make well-reasoned changes, and to own his bugs. He does regularly ask for review and testing and I don't spend as much time as I should on his reviews. The problem, though, is the default assumption that most changes are not worth reviewing at all because they're small, mechanical, no one cares, whatever. It's worth soliciting review if only so that at least one other person has an idea of what's changing in the tree, because there _will_ come a time when it helps that person make changes of their own, or find a bug. Even if a reviewer doesn't deeply understand a diff, they might suggest stylistic changes that make the code more readable, or comment on related code in a way that's useful later. Similarly, verbose commit logs can seem pointless but are a huge help down the road. I know this is a common observation but it comes up all the time for me. 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. 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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?X7X6KSF66QZbOZCI>