similar to: Policy question about Phabricator reviews

At 8/16/2020 10:39 AM, Stefan Stipanovic wrote: >Hi Paul, > >I've read "LLVM Code-Review Policies and Practices," but I remain unsure of a couple of things. Do I always wait for an actual "LGTM", or can people approve the patch for submission in other ways? > >The patch is accepted through Phabricator with or without a message (the message is usually LGTM or

On Tue, Jul 21, 2020 at 11:07 AM David Blaikie <dblaikie at gmail.com> wrote: > +Mehdi AMINI <joker.eph at gmail.com> who's taking some (shared?) > ownership of Phabricator these days. > > Mehdi - was Phab updated recently (such that we might've picked up new > semantics)? > No: I upgraded the hardware and the OS, but not Phab itself yet. I have a test

On Thu, 16 Jan 2020 at 18:45, David Blaikie <dblaikie at gmail.com> wrote: > I'm not sure where the idea that a patch series is anything other than that ^ came from. When I was talking about a patch series, it was/is with that definition in mind - ordered/dependent commits. I said "dependent series" to reinforce this idea that the kind of situation I was describing was one

On Tue, 28 Jan 2020 at 16:09, David Greene <dag at cray.com> wrote: > The question is if everything is approved and the author does a final > cleanup as alluded to above, does that final cleanup also need to go > through review? We don't enforce that now, so I see no reason to start doing it. Phab reviews, once approved, can have last-minute modifications and direct commits.

On 01/22, David Greene via cfe-dev wrote: > Renato Golin via cfe-dev <cfe-dev at lists.llvm.org> writes: > > >> My understanding from other people's comments (I've not tried it myself) is that updating a patch series in github is problematic - that it sends new/separate review email rather than being able to associate each patch in the series with ongoing review

On Fri, 10 Jan 2020 at 13:43, Nicolai Hähnle via llvm-dev <llvm-dev at lists.llvm.org> wrote: > It's worth pointing out that GitHub is not able to do this properly, > either. The problem on GitHub's side is that while a pull request can > contain multiple commits, one cannot properly review those commits > individually, and it is not at all possible to approve individual

Has anyone else noticed Phabricator sending emails saying: This revision was not accepted when it landed; it landed in state "Needs Review". when the review clearly has been accepted by someone? Some recent examples: https://reviews.llvm.org/D83952 https://reviews.llvm.org/D80116 https://reviews.llvm.org/D81267 Thanks, Jay.

> > Only a single LGTM is required. Reviewers are expected to only LGTM > patches they're confident in their knowledge of. Reviewers may review > and provide suggestions, but explicitly defer LGTM to someone else. > This is encouraged and a good way for new contributors to learn the code. Whilst I get what you're trying to say, I'm not particularly comfortable with

On Tue, Jan 14, 2020 at 11:32 AM Renato Golin via llvm-dev < llvm-dev at lists.llvm.org> wrote: > On Wed, 8 Jan 2020 at 02:26, Daniel Sanders via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > It's worth mentioning that Phabricator can read strings of the format > 'Depends on D1234' from commit messages and create those relationships for > you. >

On Mon, Nov 18, 2019 at 4:53 PM Finkel, Hal J. via llvm-dev <llvm-dev at lists.llvm.org> wrote: > On 11/18/19 4:29 AM, James Henderson wrote: >> >> Only a single LGTM is required. Reviewers are expected to only LGTM >> patches they're confident in their knowledge of. Reviewers may review >> and provide suggestions, but explicitly defer LGTM to someone else.

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

I know you worked hard to make sure that updating a revision doesn't send email unless there is text typed into one of the boxes Manuel, but I think we should by default put some text into a box (and send the email unless the user deletes that text) when accepting a revision. Otherwise, the final LGTM can accidentally happen on Phab and not reach the mailing list (D4178 for example).

On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoerfert at anl.gov> wrote: > Hi Bill, > > On 01/07, Bill Wendling via llvm-dev wrote: > > Then perhaps those opposed could suggest how to use Phabricator/Arcanist > so > > that I don't throw my keyboard through my monitor? > > Please explain your problems, w/o the hyperbole, so people can actually do >

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

I have seen the "Too many recipients to the message" several times. A limit of 10 includes the patch author and the list leaving just 8 subscribers/reviewers is way too low. Given that these e-mails can be sent only by a Phab. user I'm not sure that spam is a problem at all: A potential spammer would first have to subscribe to Phab. then create a proper diff,... far easier just to

On Mon, Jun 30, 2014 at 10:48 AM, Renato Golin <renato.golin at linaro.org> wrote: > On 27 June 2014 21:53, Duncan P. N. Exon Smith <dexonsmith at apple.com> > wrote: > > Then the only way for people to "accept" something is by typing text > > to that effect. > > If you don't "Accept" the patch, you can't close the issue. So, either

Yeah, +1 that people from the same organization are sometimes the only ones working on a certain feature/area. (certainly I'd expect some discussion about the feature in general to be discussed outside that group if it's in any way contentious - but some stuff's clear enough (I think I implemented debug_types years ago, likely with only Eric's approval, both of us being at Google

On Wed, Jan 15, 2020 at 01:30:34PM -0600, David Greene via cfe-dev wrote: > Emilio Cobos Álvarez <emilio at crisal.io> writes: > > > [1] or [2] are recentish examples that come to mind, but it happens > > fairly often. Of course for a bunch of simpler changes one revision is > > enough. > > I think you forgot to include links. :) > > > The use cases

+1 in general, and Philip has good suggestions as well! -- Mehdi On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev < llvm-dev at lists.llvm.org> wrote: > + 1 in general, a couple of suggestions > > On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote: > > Hi, everyone, > > > > I've been fielding an increasing number of questions about how our

On Wed, Jun 25, 2014 at 3:30 PM, John Criswell <criswell at illinois.edu> wrote: > On 6/25/14, 5:15 PM, Vadim Chugunov wrote: > > In a recent review via Phabricator, I was receiving bounce notifications for > mail being sent to llvm-commits because of "Too many recipients to the > message", even though I am a subscriber. I wonder how common is that. > > >