[LLVMdev] Phabricator and private reviews

[Daniel Sanders]

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

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

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.