[LLVMdev] Phabricator and private reviews

[Manuel Klimek]

On Fri, Jun 27, 2014 at 6:07 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

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

Hm, we might be able to set up dummy repositories (because we only have a
single large LLVM repository from the code view point of view).


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

That's something we want to fix long-term - that is, we'll want to fix the
emails to send mails as they look on the first mail when you add people to
a review; I've already filed a bug for that.


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

Oh, we can do anything ourselves, but we'd need somebody to spend some time
to hack on it.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140627/3ffda100/attachment.html>