Phabricator + 'Reviewed by' [was Re: svn commit: r278472 - in head/sys: netinet netinet6]

[Ian Lepore]

On Sat, 2015-02-14 at 13:59 -0600, Bryan Drewery wrote:
> On 2/14/2015 9:17 AM, Steven Hartland wrote:
> > 
> > On 13/02/2015 23:56, Bryan Drewery wrote:
> >> On 2/9/2015 3:45 PM, Bjoern A. Zeeb wrote:
> >>>>   Commented upon by hiren and sbruno
> >>>>   See Phabricator D1777 for more details.
> >>>>
> >>>>   Commented upon by hiren and sbruno
> >>>>   Reviewed by:    adrian, jhb and bz
> >>> I have not reviewed this;  as a matter of fact you are aware that I
> >>> still wanted to do that.
> >>>
> >> Something about Phabricator is not jiving with our commit terminology.
> >> This has happened before as well with other commits. I'm sure everyone
> >> is good-intentioned as well.
> >>
> >> There's not 1 person on D1777 who has 'accepted' it. That is what
> >> warrants a 'Reviewed by' to me.
> >>
> >> It's clear to me, but seems unclear to others. I really think the
> >> reviewer list needs to be split up. Rather than using icons, use
> >> separate lists. Reviewers requested: accepted: commented: changes
> >> requested:.
> > I don't think it needs to be split up, that feels unnecessary, if
> > someone hasn't accepted it then they haven't review it period IMO.
> 
> Yes I too think it's obvious, yet I've seen at least 2 commits where the
> reviewed by line was essentially a lie. It's in SVN forever now with
> those names stamped as reviewers.
> 

You make that sound like some sort of huge crisis, but we have glitches
in commit messages (occasionally even a missing/empty message) from time
to time, and life goes on.  Phabricator is supposed to be a tool to make
our lives better and easier, but it could all too easily turn into a
stick to hit people with, and the first step on that path is making a
bunch of rigid formal rules and procedures.

-- Ian