[LLVMdev] Phabricator and private reviews

[Alp Toker]

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?

Alp.


>
> Again, no disagreement whatsoever that the bugginess is harmful and 
> should be addressed. But the tone could be friendlier.
>
> Eli
>
>

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