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 _______________________________________________ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
