Date: Thu, 29 Mar 2018 12:05:27 -0600 From: Warner Losh <imp@bsdimp.com> To: John Baldwin <jhb@freebsd.org> Cc: "Rodney W. Grimes" <rgrimes@freebsd.org>, Ian Lepore <ian@freebsd.org>, Marcelo Araujo <araujo@freebsd.org>, src-committers <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: <CANCZdfpJnW2uuLuMz=usNjFN8XfRoTpXMSP4ZNmYNFW=QvBTeQ@mail.gmail.com> In-Reply-To: <1736582.AJYahrK36B@ralph.baldwin.cx> References: <201803291633.w2TGXinX064128@pdx.rh.CN85.dnsmgr.net> <1736582.AJYahrK36B@ralph.baldwin.cx>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Mar 29, 2018 at 11:45 AM, John Baldwin <jhb@freebsd.org> wrote: > 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. The number of reviews isn't onerous. However, as jhb points out, I have lots of reviews that have people added automatically in stand, for man pages, parts of the networking stack, etc. It's really not that hard to do. Lots of people monitor what's going on. > > ?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). > Now that's handy for when I misspell Differential Revision: or some other crazy thing happens so the revision isn't closed. Thanks for the useful tip. Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfpJnW2uuLuMz=usNjFN8XfRoTpXMSP4ZNmYNFW=QvBTeQ>