Date: Thu, 29 Mar 2018 10:45:15 -0700 From: John Baldwin <jhb@freebsd.org> To: rgrimes@freebsd.org Cc: Ian Lepore <ian@freebsd.org>, Marcelo Araujo <araujo@freebsd.org>, 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 Message-ID: <1736582.AJYahrK36B@ralph.baldwin.cx> In-Reply-To: <201803291633.w2TGXinX064128@pdx.rh.CN85.dnsmgr.net> References: <201803291633.w2TGXinX064128@pdx.rh.CN85.dnsmgr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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<xxxx>) that turns into a hyperlink to the commit in the comment closing the review (a bare SVN-style r<xxxx> won't do the hyperlink). -- John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1736582.AJYahrK36B>