From owner-svn-src-stable-11@freebsd.org Thu Mar 29 17:51:28 2018 Return-Path: Delivered-To: svn-src-stable-11@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 1AD9AF67897; Thu, 29 Mar 2018 17:51:28 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from mail.baldwin.cx (bigwig.baldwin.cx [96.47.65.170]) (using TLSv1 with cipher DHE-RSA-CAMELLIA256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 265337AF37; Thu, 29 Mar 2018 17:51:25 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from ralph.baldwin.cx (ralph.baldwin.cx [66.234.199.215]) by mail.baldwin.cx (Postfix) with ESMTPSA id AE1C710A87D; Thu, 29 Mar 2018 13:51:18 -0400 (EDT) From: John Baldwin To: rgrimes@freebsd.org Cc: Ian Lepore , Marcelo Araujo , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: Re: svn commit: r331728 - in stable/11/etc: . rc.d Date: Thu, 29 Mar 2018 10:45:15 -0700 Message-ID: <1736582.AJYahrK36B@ralph.baldwin.cx> User-Agent: KMail/4.14.10 (FreeBSD/11.1-STABLE; KDE/4.14.30; amd64; ; ) In-Reply-To: <201803291633.w2TGXinX064128@pdx.rh.CN85.dnsmgr.net> References: <201803291633.w2TGXinX064128@pdx.rh.CN85.dnsmgr.net> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.4.3 (mail.baldwin.cx); Thu, 29 Mar 2018 13:51:18 -0400 (EDT) X-Virus-Scanned: clamav-milter 0.99.2 at mail.baldwin.cx X-Virus-Status: Clean X-BeenThere: svn-src-stable-11@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: SVN commit messages for only the 11-stable src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 29 Mar 2018 17:51:28 -0000 On Thursday, March 29, 2018 09:33:44 AM Rodney W. Grimes wrote: > > On Thu, 2018-03-29 at 06:20 -0700, Rodney W. Grimes wrote: > > That's not a blocker for committing; plenty of time elapsed to allow > > anyone to reject the change. IMO, even a flat-out rejection isn't a > > blocker to committing except for things like random or crypto code that > > require formal approval (but I'd certainly think hard about committing > > if people rejected the change, and put some effort into finding a > > compromise first). > > It seems that the Phabricator review system is somewhat disfunctional > in that actual review is only happening in some cases. Some people > have even stated they flat out hate it. Others say that it is the > way to go. Despite its limitations, we are in a far better shape with phab than we were without it. > As araujo@ pointed out he was a "reviewer", but as I'll point out > he didnt accept the review, which causes the landing state to be > wrong, and is kinda implied that anyone commiting a phabricator > change has reviewed it anyway. Eh, I treat the 'reviewed by' line in the commit log itself as the authoritative list. For phab I think folks who use it understand that only those who "accepted" it are the actual approvers (this is true in LLVM as well). > The problem is that most people are not notified that a review > of a change is even in process until the commit lands, this is > not a functional communications system. > > Requring us all to go sign up like imp@ did to receive all > submitted reviews, imho, is also a non functional situation. People are welcome to create herald rules to sign up for notifications for specific parts of the tree. That is open even to non-committers to do. I don't think that is all that onerous as it gives users the control to decide which parts of the tree they want to monitor. > > ?I usually also add a note such as '(timed out)' > > after the url, but I've noticed that doing so ruins the automatic > > closing of the review and requires you to manually abandon it instead. > > There isnt a way to close it as commited/fixed in rXXXXXX manually? > If not that is yet another shortcoming of phabricator that should > be looked at. You can close without abandoning. (I've done it a few times by just using "Close Revision"). I do think this is something that didn't used to work. I've also noticed that recently phab has grown a "Edit Related Objects" that seems to let you associate commits with a commit so that you can fix a review to be tagged to the commit that closed it if it doesn't auto-close (this seems to be a very recent change). Prior to that, I've closed the revision with a comment using the right syntax (rS) that turns into a hyperlink to the commit in the comment closing the review (a bare SVN-style r won't do the hyperlink). -- John Baldwin