[LLVMdev] Phabricator and private reviews

[Eli Bendersky]

On Wed, Jun 25, 2014 at 12:32 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 25/06/2014 21:18, Eli Bendersky wrote:
>
>>
>>
>>
>> On Wed, Jun 25, 2014 at 11:10 AM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>
>>     On 25/06/2014 21:03, Eli Bendersky wrote:
>>
>>         On Wed, Jun 25, 2014 at 10:44 AM, Alp Toker <alp at nuanti.com
>>         <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>>
>>         <mailto:alp at nuanti.com>>> wrote:
>>
>>             For whatever reason, patches posted to the Phabricator website
>>             still aren't being sent to the mailing list, making it
>>         difficult
>>             for us to review them.
>>
>>             I've raised this issue a couple of times in the last few
>>         weeks.
>>
>>             In practice this has a detrimental effect to the development
>>             workflow because it means that code is being seen only by
>>         a small
>>             group of individuals who have web accounts. The code isn't
>>         hitting
>>             llvm-commits or cfe-commits where the majority of code
>>         maintainers
>>             use the mailing lists for review.
>>
>>             At this point I think Phabricator should be disabled and
>>         patches
>>             should be send to the mailing lists *until* the technical
>>         issue is
>>             confirmed resolved.
>>
>>             It's really uncool that code is entering ToT through this
>>             back-channel -- I appreciate that it might not be
>>         intentional, but
>>             every single patch that gets committed this way is a real
>>         problem
>>             for the project.
>>
>>
>>         Phabricator has certainly had its share of technical
>>         difficulties lately. Just last week it suppressed all email to
>>         llvm-commits for many hours. These problems should be solved.
>>         That said, talking of "private reviews" and "back-channels"
>>         doesn't strike me as constructive.
>>
>>
>>     Eli, I wasn't making a value judgement. That's exactly what they are:
>>
>>       1) They're private reviews because they're conducted away from
>>     the LLVM community.
>>       2) It's a back-channel because the only means of veto is to
>>     revert the patch or attempt to "fix forward" post-commit.
>>
>>     I already pointed out that it may not be intentional --
>>
>> "May" not be intentional suggests that it also "may" be intentional. Or
>> is it English comprehension failing me? [sorry, 3rd language...]
>>
>
> Hi Eli,
>
> The sentence is definitely written as intended and it's that way in order
> to avoid making value judgements.
>
> I don't have any evidence whether the public lists have been excluded
> intentionally or due to technical issues so I can't say "is" or "isn't".
>
> As I understand, some people legitimately use Phabricator for internal
> review, while others *think* they're submitting public patches but the
> system doesn't forward them to the reviews lists so my usage of "may" is
> entirely correct.
>
>
>
>  When llvm-commits is CCd on the Phabricator review, any suggestion of
>> intentional hiding is not only inappropriate, but also somewhat ridiculous.
>> Unless you're suggesting someone is planting those PHP bugs? [that could be
>> unintentional, I concede, since PHP is just one big bug in general]
>>
>
> Clearly I'm not suggesting that someone is planting PHP bugs :-)
>
> But yes, you can probably tell I'm disappointed that it's still happening,
> because ultimately I care about the quality of the code and delivering a
> great compiler to our users. So how about we get to the bottom of these
> issues?


Right. It would be useful to see actual examples and discuss them on a
case-by-case basis. I imagine you sent the original email when observing
such a case (or a few) - could you share the review link? I think this
thread discussed a number of ways in which such things could go wrong, and
it would be interesting to see which of those applies to the example you're
presenting.

Eli
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140625/9d71150e/attachment.html>