svn commit: r367695 - in head/sys: kern sys

[Mark Johnston]

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 at 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.