From owner-svn-src-all@freebsd.org Thu Nov 19 04:53:02 2020 Return-Path: Delivered-To: svn-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 237A047F514; Thu, 19 Nov 2020 04:53:02 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-qv1-xf42.google.com (mail-qv1-xf42.google.com [IPv6:2607:f8b0:4864:20::f42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Cc6kQ0Bl2z3hrf; Thu, 19 Nov 2020 04:53:01 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: by mail-qv1-xf42.google.com with SMTP id b11so2267596qvr.9; Wed, 18 Nov 2020 20:53:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=sender:date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=jW5rxoUTZfm+CegqOObnIauwX5H1l1yqCAIjj76wE68=; b=V2fa/DildPT21R464aR7E3bNf/IZVYBPJA6eYL797J2SyCQ+d6KJs7WJCSnE2hWz70 6gqkXXAZbObalChL6KhfManI+9CzerECsOeJxryEmW9qgIOGn6agVPwHxjaKhTsV4cv1 PF0abN2rbMWa/I//6//NGOcyCgyMT25NQ2uNcWV5xGSEi0Al4l5xZLPccr1xaEaYaEBM /SLsjxUUdhL0uwtUhEqj5mmsd5shA4qqCFMsHNlL5kinXIiDE0uDVSGkvmRQO6QB93H6 XWon5vxfS8uOnUbTcggRYNN8ggr7NQlh/z+hT5OUQdupwY0apgkyJsVHgnwGASiJqMNt 8mNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:sender:date:from:to:cc:subject:message-id :references:mime-version:content-disposition:in-reply-to; bh=jW5rxoUTZfm+CegqOObnIauwX5H1l1yqCAIjj76wE68=; b=S+vPgxepeE3H2ZSFBDFnsM6M+lTYnDfRT5TTkJqh+/o2zA4rEqrlQy2hg75VqUAX80 xrDRBmZbXV1ohmXkTm2dR7wnI03Ruy8ZNOZGFaMrzhW1ogOuBHlX749EG1RLTpzu3Aaj TB0dXVnXusqpLdPz/RkWrMLqwVhK5ZzZkqXLkwrsRAJaYEyrz+wDgIx9OwEwIgrkl/QZ Br4qoVymcoJGbZuqtPNNe8DG5Plc6ZH2eJyJMdGvKB0OLtvZuvMpK7H1+t+YPGXN6UgG YDP6MizEbbd7fWiU8koZVtn6+O2klUZiXQZw1+Th+jgAyXyR5pn8WvaVp3j8LSrjisry RIeQ== X-Gm-Message-State: AOAM530uoDPkv+pfxBdoWkzvjPuk4eaEKJtbZg2wpTvgdQsiomtek1WQ /ebLuR/0o7bw2IW89MgmHcJxkBDWLts= X-Google-Smtp-Source: ABdhPJzRexCsaRrpMPiu1MTJFbls9pEuLKoppycmchcCr7DPB6aE54wqdzXo+F/s+LoJAXg1KOAP3A== X-Received: by 2002:a0c:b34a:: with SMTP id a10mr9305790qvf.15.1605761580461; Wed, 18 Nov 2020 20:53:00 -0800 (PST) Received: from raichu ([142.126.164.150]) by smtp.gmail.com with ESMTPSA id t205sm8063364qke.35.2020.11.18.20.52.59 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 18 Nov 2020 20:52:59 -0800 (PST) Sender: Mark Johnston Date: Wed, 18 Nov 2020 23:52:57 -0500 From: Mark Johnston To: John Baldwin Cc: Mateusz Guzik , 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: References: <202011141922.0AEJM2ld055995@repo.freebsd.org> <4f6f6b0a-e71c-a286-507e-abf2522c142c@FreeBSD.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4f6f6b0a-e71c-a286-507e-abf2522c142c@FreeBSD.org> X-Rspamd-Queue-Id: 4Cc6kQ0Bl2z3hrf X-Spamd-Bar: ---- Authentication-Results: mx1.freebsd.org; none X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 19 Nov 2020 04:53:02 -0000 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 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.