[LLVMdev] Phabricator and private reviews

[Manuel Klimek]

On Thu, Jun 26, 2014 at 12:01 PM, Daniel Sanders <Daniel.Sanders at imgtec.com>
wrote:

> > From: Manuel Klimek [mailto:klimek at google.com]
> > Sent: 26 June 2014 10:40
> > To: Daniel Sanders
> > Cc: Alp Toker; Eli Bendersky; llvmdev at cs.uiuc.edu
> > Subject: Re: [LLVMdev] Phabricator and private reviews
> >
> > > On Thu, Jun 26, 2014 at 11:34 AM, Daniel Sanders <
> Daniel.Sanders at imgtec.com> wrote:
> > > > As I understand, some people legitimately use Phabricator for
> internal
> > > > review, ...
> > >
> > > MIPS currently do this for patches that only touch the MIPS backend
> (details can be found
> > > at
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140602/220385.html
> ).
> > > Any patch that touches common code is sent to the list. One thing I
> haven't said on that
> > > thread yet is that I'm considering trying the 'everything to
> llvm-commits' workflow myself
> > > in a couple weeks when my urgent work is finished. I have some
> reservations about that
> > > workflow but I'm willing to try it to see if my concerns are justified
> or not.
> >
> > Not knowing your concerns, I'd say they are mostly unjustified ;)
>
> That will probably turn out to be the case.
>
> Here are the concerns copy/pasted from that thread:
>   These are mostly my opinion with little evidence to back them up but
> there's a couple fairly
>   weak reasons [not to post everything to the list]. The first is that I'd
> like to avoid the scenario
>   where MIPS patches that need wider review regularly find it hard to
> attract reviewers because
>   they are difficult to spot among the more mundane patches. To put it
> another way, my concern
>   is that CC'ing llvm-commits on everything may encourage many people to
> think of '[mips]' as a
>   spam tag. The second is that CC'ing llvm-commits seems to make people
> nervous, occasionally
>   to the point of asking for a pre-pre-commit review. Interestingly, the
> current approach doesn't
>   seem to have the same effect even though the outcome is pretty much the
> same. It seems that
>   people don't mind admitting mistakes too much, but they like to know
> what those mistakes are
>   before they reveal them to the world.
>

Funnily enough I think that sending to a larger audience earlier has the
reverse effect - when people see that patches get thorough review (by
seeing problems addressed) they develop trust, which helps with reviews
later.


>
> > > Manuel: I'm aware of one other way for a review to be invisible to
> llvm-commits despite being
> > > CC'd. If you forget to CC llvm-commits when creating the differential
> revision and add it later in
> > > the web interface, Phabricator doesn't send an email to the list.
> Adding a comment (at the same
> > > time or afterwards) triggers an email.
> > > The same thing happens when adding reviewers without a comment.
> >
> > That is intentional. We don't want to spam the list with changes in phab
> - use phab like you would use email, and everything should be fine.
>
> Hmm. If I reply-all to an email, add a new recipient but forget to write
> anything, everyone still receives the email. Admittedly not writing
> anything would be unusual.
>

Well, if you don't hit "send" nothing gets sent :)


> As an alternative, perhaps the web interface should ask the user if they
> are sure they want to add a recipient without notifying them about it.
>

That would be awesome (patches welcome - phab is an open source project ;)
- in absence of that, I think it takes a contributor only once to learn to
write something when wanting to send out a patch.

But we also want to improve the workflow around this in general (so that
the first email that gets sent actually looks like a review request), which
would allow us to revisit the "do not send when adding reviewers"
decisions. As I've said, we would welcome contributions to the phabricator
stuff from the community, so far I've done all the adaptions to the LLVM
workflow myself (I've so far probably spent > 1 month of effort over the
years on phab development).

Cheers,
/Manuel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140626/c2aae9b5/attachment.html>