<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, May 21, 2014 at 6:07 PM, Alp Toker <span dir="ltr"><<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
On 22/05/2014 03:29, Chandler Carruth wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
On Wed, May 21, 2014 at 6:25 PM, Richard Smith <<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a> <mailto:<a href="mailto:richard@metafoo.co.uk" target="_blank">richard@metafoo.co.uk</a>><u></u>> wrote:<br>

<br>
    On Wed, May 21, 2014 at 5:21 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div><div class="">
    <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>> wrote:<br>
<br>
<br>
        On 22/05/2014 03:04, Chandler Carruth wrote:<br>
<br>
<br>
            On Wed, May 21, 2014 at 5:38 PM, Alp Toker <<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><br></div>
            <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>> <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a><div><div class="h5"><br>
            <mailto:<a href="mailto:alp@nuanti.com" target="_blank">alp@nuanti.com</a>>>> wrote:<br>
<br>
                On 21/05/2014 23:19, David Majnemer wrote:<br>
<br>
                    Author: majnemer<br>
                    Date: Wed May 21 15:19:59 2014<br>
                    New Revision: 209319<br>
<br>
                    URL:<br>
            <a href="http://llvm.org/viewvc/llvm-project?rev=209319&view=rev" target="_blank">http://llvm.org/viewvc/llvm-<u></u>project?rev=209319&view=rev</a><br>
                    Log:<br>
                    Sema: Implement DR244<br>
<br>
                    Summary:<br>
                    Naming the destructor using a typedef-name for the<br>
            class-name is<br>
                    well-formed.<br>
<br>
                    This fixes PR19620.<br>
<br>
                    Reviewers: rsmith, doug.gregor<br>
<br>
<br>
                Did Doug participate in review for this patch?<br>
<br>
<br>
            No, Richard Smith did. I see a pretty complete review<br>
            thread for the DR244 patch in my inbox. I've even checked<br>
            and none of the emails were lost in the recent email list<br>
            snafu.<br>
<br>
<br>
        Take a look at the Reviewers line?<br>
<br>
<br>
<br>
                Looking through SVN history, I'm seeing an alarming<br>
            number of<br>
                confusing review trails.<br>
<br>
<br>
            Can you point them out specifically? I too keep a pretty<br>
            close eye on these kinds of things and I'm not seeing any<br>
            examples.<br>
<br>
<br>
                If review happened off-list that's fine, but it needs<br>
            to be stated<br>
                clearly because the system works on trust.<br>
<br>
<br>
            I don't think off-list review is fine... It happens some<br>
            times, for good or bad reasons, and its not the end of the<br>
            world. But it is definitely not SOP, and I haven't seen<br>
            any evidence of it becoming more pervasive. If you see it,<br>
            call it out. When patches merit pre-commit review, they<br>
            should get it from the whole community.<br>
<br>
<br>
        I'd expect that off-list review would be mentioned explicitly<br>
        rather than everyone making the implicit assumption that's the<br>
        case.<br>
<br>
        In the case of r209319, Doug is mentioned as reviewer but I<br>
        only see Richard's review comments on the list.<br>
<br>
        That's why I asked for clarification on Doug's participation<br>
        -- was it off-list, or is it a mistake in the commit log?<br>
<br>
<br>
<br>
                (In fact, I'm seeing relatively inactive developer<br>
            names showing<br>
                up suspiciously in these "Reviewers" lines while some<br>
            of the most<br>
                active reviewers barely appear at all. Could this be a<br>
            problem<br>
                with Phabricator or some internal system you guys are<br>
            using?)<br>
<br>
<br>
            I'm really not sure what you're worried about here. Again,<br>
            specific examples?<br>
<br>
<br>
        So I want to know if Doug was involved with review of this<br>
        change as stated in the commit log. That's a good place to start.<br>
<br>
<br>
    The "Reviewers" line that (IIUC) arcanist adds lists the reviewers<br>
    in Phab, which includes Doug, and doesn't indicate that anyone<br>
    mentioned actually reviewed anything. This is a list of people who<br>
    were asked to review, not a list of people who did so.<br>
<br>
<br>
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.<br>
</div></div></blockquote>
<br>
It's concerning that we lack authoritative data on something as fundamental as authorship/review/attribution, let alone how many incidents there have been.</blockquote><div><br></div><div>The commit logs from the first N years don't really mention review state. I don't think this is a big deal. (The patches that have a link to phab are better than the email-reviewed patches with this regard, as the phab link has visible comments on phab, while email reviews are only findable by looking at threads that occurred at the same time, or sometimes by looking for "rXXXX" to find the "submitted in rXXXX" email on the review thread.)</div>
<div><br></div><div>I agree not adding misleading "Reviewers:" lines would be good.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">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.<br>

</blockquote>
<br></div>
It's actually not OK that we have several commits with unclear reviewership, going back weeks or months.<br>
<br>
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.<br>

<br>
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.<br>

<br>
Now we're in a situation where changes like that have landed which haven't necessarily been looked over.<br></blockquote><div><br></div><div>Which change?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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.<br>
<br>
Alp.<br>
<br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
        (No sleight towards David, we're working together on plenty of<br>
        stuff day to day. It's just good practice to keep a reasonable<br>
        paper trail for authorship / reviewership and ask questions<br>
        when things look out of place.)<br>
<br>
        Alp.<br>
<br>
<br>
<br>
        --         <a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
        the browser experts<br>
<br>
        ______________________________<u></u>_________________<br>
        cfe-commits mailing list<br></div>
        <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a> <mailto:<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.<u></u>edu</a>><br>
        <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
<br>
<br>
<br>
</blockquote><div class="HOEnZb"><div class="h5">
<br>
-- <br>
<a href="http://www.nuanti.com" target="_blank">http://www.nuanti.com</a><br>
the browser experts<br>
<br>
______________________________<u></u>_________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/<u></u>mailman/listinfo/cfe-commits</a><br>
</div></div></blockquote></div><br></div></div>