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> > Yaron > > > 2014-06-27 0:40 GMT+03:00 Justin Bogner <mail at justinbogner.com>: > >> "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes: >> >> On 2014-Jun-25, at 12:32, Alp Toker <alp at nuanti.com> wrote: >> >> >> >> As I understand, some people legitimately use Phabricator for >> >> internal review, >> > >> > Is this is a use case we need to support on <http://reviews.llvm.org>? >> > >> >> while others *think* they're submitting public patches but the >> >> system doesn't forward them to the reviews lists >> > >> > Aside from temporary infrastructure failures, which Manuel responds to >> > quickly, there seem to be two main reasons patches (and comments) >> > don't get sent to the list: >> > >> > 1. Reviewer neglects to add llvm-commits (or adds it after the fact, >> > and the patch isn't sent). >> > >> > This seems easy to fix by requiring llvm-commits on llvm patches >> > (and cfe-commits on clang patches). Is there a technical hurdle >> > to requiring this? >> >> +1. This is the situation I've seen most often, and while innocuous it >> can be pretty annoying. >> >> > 2. Reviewer (or commenter) has an invalid or un-subscribed email on >> > Phab. >> > >> > Is it technically feasible to disallow reviews (or comments on a >> > review) from logins that aren't subscribed to the appropriate list >> > and/or have invalid email addresses? >> > >> > I suspect designing away these failures would fix most of the >> > problems. >> > >> > Since the purpose of this Phab instance is to support on-list reviews, >> > we should make it hard to get that wrong (even if it means making >> > off-list reviews harder). >> > >> > -- dpnes >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> > > > _______________________________________________ > LLVM Developers mailing list > LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev > >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140627/b0f43870/attachment.html>
Thanks! 2014-06-27 10:36 GMT+03:00 Manuel Klimek <klimek at google.com>:> 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 > > >> >> Yaron >> >> >> 2014-06-27 0:40 GMT+03:00 Justin Bogner <mail at justinbogner.com>: >> >>> "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes: >>> >> On 2014-Jun-25, at 12:32, Alp Toker <alp at nuanti.com> wrote: >>> >> >>> >> As I understand, some people legitimately use Phabricator for >>> >> internal review, >>> > >>> > Is this is a use case we need to support on <http://reviews.llvm.org>? >>> > >>> >> while others *think* they're submitting public patches but the >>> >> system doesn't forward them to the reviews lists >>> > >>> > Aside from temporary infrastructure failures, which Manuel responds to >>> > quickly, there seem to be two main reasons patches (and comments) >>> > don't get sent to the list: >>> > >>> > 1. Reviewer neglects to add llvm-commits (or adds it after the fact, >>> > and the patch isn't sent). >>> > >>> > This seems easy to fix by requiring llvm-commits on llvm patches >>> > (and cfe-commits on clang patches). Is there a technical hurdle >>> > to requiring this? >>> >>> +1. This is the situation I've seen most often, and while innocuous it >>> can be pretty annoying. >>> >>> > 2. Reviewer (or commenter) has an invalid or un-subscribed email on >>> > Phab. >>> > >>> > Is it technically feasible to disallow reviews (or comments on a >>> > review) from logins that aren't subscribed to the appropriate list >>> > and/or have invalid email addresses? >>> > >>> > I suspect designing away these failures would fix most of the >>> > problems. >>> > >>> > Since the purpose of this Phab instance is to support on-list reviews, >>> > we should make it hard to get that wrong (even if it means making >>> > off-list reviews harder). >>> > >>> > -- dpnes >>> _______________________________________________ >>> LLVM Developers mailing list >>> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >>> >> >> >> _______________________________________________ >> LLVM Developers mailing list >> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu >> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev >> >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20140627/6f800fb1/attachment.html>
> 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?
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>