svn commit: r331728 - in stable/11/etc: . rc.d

[John Baldwin]

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