r209319 - Sema: Implement DR244

[Alp Toker]

On 22/05/2014 03:29, Chandler Carruth wrote:
> On Wed, May 21, 2014 at 6:25 PM, Richard Smith <richard at metafoo.co.uk 
> <mailto:richard at metafoo.co.uk>> wrote:
>
>     On Wed, May 21, 2014 at 5:21 PM, Alp Toker <alp at nuanti.com
>     <mailto:alp at nuanti.com>> wrote:
>
>
>         On 22/05/2014 03:04, Chandler Carruth wrote:
>
>
>             On Wed, May 21, 2014 at 5:38 PM, Alp Toker <alp at nuanti.com
>             <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>             <mailto:alp at nuanti.com>>> wrote:
>
>                 On 21/05/2014 23:19, David Majnemer wrote:
>
>                     Author: majnemer
>                     Date: Wed May 21 15:19:59 2014
>                     New Revision: 209319
>
>                     URL:
>             http://llvm.org/viewvc/llvm-project?rev=209319&view=rev
>                     Log:
>                     Sema: Implement DR244
>
>                     Summary:
>                     Naming the destructor using a typedef-name for the
>             class-name is
>                     well-formed.
>
>                     This fixes PR19620.
>
>                     Reviewers: rsmith, doug.gregor
>
>
>                 Did Doug participate in review for this patch?
>
>
>             No, Richard Smith did. I see a pretty complete review
>             thread for the DR244 patch in my inbox. I've even checked
>             and none of the emails were lost in the recent email list
>             snafu.
>
>
>         Take a look at the Reviewers line?
>
>
>
>                 Looking through SVN history, I'm seeing an alarming
>             number of
>                 confusing review trails.
>
>
>             Can you point them out specifically? I too keep a pretty
>             close eye on these kinds of things and I'm not seeing any
>             examples.
>
>
>                 If review happened off-list that's fine, but it needs
>             to be stated
>                 clearly because the system works on trust.
>
>
>             I don't think off-list review is fine... It happens some
>             times, for good or bad reasons, and its not the end of the
>             world. But it is definitely not SOP, and I haven't seen
>             any evidence of it becoming more pervasive. If you see it,
>             call it out. When patches merit pre-commit review, they
>             should get it from the whole community.
>
>
>         I'd expect that off-list review would be mentioned explicitly
>         rather than everyone making the implicit assumption that's the
>         case.
>
>         In the case of r209319, Doug is mentioned as reviewer but I
>         only see Richard's review comments on the list.
>
>         That's why I asked for clarification on Doug's participation
>         -- was it off-list, or is it a mistake in the commit log?
>
>
>
>                 (In fact, I'm seeing relatively inactive developer
>             names showing
>                 up suspiciously in these "Reviewers" lines while some
>             of the most
>                 active reviewers barely appear at all. Could this be a
>             problem
>                 with Phabricator or some internal system you guys are
>             using?)
>
>
>             I'm really not sure what you're worried about here. Again,
>             specific examples?
>
>
>         So I want to know if Doug was involved with review of this
>         change as stated in the commit log. That's a good place to start.
>
>
>     The "Reviewers" line that (IIUC) arcanist adds lists the reviewers
>     in Phab, which includes Doug, and doesn't indicate that anyone
>     mentioned actually reviewed anything. This is a list of people who
>     were asked to review, not a list of people who did so.
>
>
> And considering that not everyone (not even most) use arcanist to 
> submit, this hasn't really even come up as an interesting bit of 
> metadata, much less an authoritative one.

It's concerning that we lack authoritative data on something as 
fundamental as authorship/review/attribution, let alone how many 
incidents there have been.

> I don't know whether anyone would want to actually fix arcanist to not 
> have this behavior -- the tool is not easy to hack IIUC. =/ I think 
> the only useful thing to include would be a link to the review.

It's actually not OK that we have several commits with unclear 
reviewership, going back weeks or months.

When a reviewer is named in the commit log, that's an explicit way of 
saying "This patch has been looked over by X" and communicating that the 
patch doesn't strictly need further post-commit review.

You usually name reviewers when a change is unconventional/unexpected, 
say, breaking a stable API for some reason that would normally raise 
questions. So it does matter somewhat if the information has been 
misleading reviewers.

Now we're in a situation where changes like that have landed which 
haven't necessarily been looked over.

It's OK to disable the tool until it gets fixed, but that doesn't mean 
we don't still need to look back and at least try to understand if 
patches have been going in through the side entrance.

Alp.


>         (No sleight towards David, we're working together on plenty of
>         stuff day to day. It's just good practice to keep a reasonable
>         paper trail for authorship / reviewership and ask questions
>         when things look out of place.)
>
>         Alp.
>
>
>
>         -- 
>         http://www.nuanti.com
>         the browser experts
>
>         _______________________________________________
>         cfe-commits mailing list
>         cfe-commits at cs.uiuc.edu <mailto:cfe-commits at cs.uiuc.edu>
>         http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>

-- 
http://www.nuanti.com
the browser experts