[LLVMdev] Phabricator and private reviews

[Duncan P. N. Exon Smith]

> On 2014-Jun-27, at 00:36, Manuel Klimek <klimek at google.com> wrote:
> 
> On Fri, Jun 27, 2014 at 8:13 AM, Yaron Keren <yaron.keren at gmail.com> wrote:
> Happened to me twice, it would be really nice if Phab would require confirmation of patches created without CCing one of the two lists, something like:
> 
> "You have not CCed llvm-commits or cfe-commits, are you creating a private patch?"
> 
> I filed
> https://secure.phabricator.com/T5495
> 
> Cheers,
> /Manuel
>  

Thanks for filing this!

A couple of comments on epriestley's comments; perhaps you can
forward these, if you agree?

> I don't immediately see a reasonable approach to implement this in a general way. In other cases, I think rules like this are usually expressed as "always use Herald to CC <some list>", but it sounds like there is no way to automatically determine the correct list in this case?

Users must specify a "repository", and the repository determines
which list needs to be included.  Once the config allows for a
mandatory (or default) list on a per-repository basis, we're golden
(cfe => cfe-commits, llvm => llvm-commits, compiler-rt =>
llvm-commits, lldb => lldb-commits, etc.).

> We have a buildRevisionWarnings() method, and we can move that into CustomFields and then I can write you a tiny piece of plugin code. This wouldn't prompt or pop a dialog, but would look like this:

This looks like a good step, but isn't sufficient long-term -- adding
(e.g.) llvm-commits *after the fact* fails to send the patch to the
list.

Also: given that there's a `buildRevisionWarnings()` hook, I wonder:
is there a custom hook for errors, which would reject the revision?