subject:"Experimenting with Phabricator for reviews"


Jun Wu  writes:

> Excerpts from Sean Farley's message of 2017-07-14 16:26:34 -0700:
>> Huh? But that's the thing, isn't it? Previously, comments about code
>> review are on the list. One can peruse them at leisure or archive /
>> skip, etc. What you're saying, if I'm not mistaken, is that some
>> comments (no matter the content) will no longer be on the list. That
>> makes perusing them now split into two locations.
>
> I think what I suggested is to move those emails to another list. So if
> people want to read them in an email client, they will still be able to.

Ah, yes, I was confused about your /dev/null comment. I see now; sorry
for the confusion.

Though, I'm not thrilled about the idea of another list but that's
another discussion.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Jun Wu

Excerpts from Sean Farley's message of 2017-07-14 16:26:34 -0700:
> Huh? But that's the thing, isn't it? Previously, comments about code
> review are on the list. One can peruse them at leisure or archive /
> skip, etc. What you're saying, if I'm not mistaken, is that some
> comments (no matter the content) will no longer be on the list. That
> makes perusing them now split into two locations.

I think what I suggested is to move those emails to another list. So if
people want to read them in an email client, they will still be able to.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Jun Wu  writes:

> Excerpts from Sean Farley's message of 2017-07-14 16:10:02 -0700:
>>
>> Jun Wu  writes:
>>
>> > Excerpts from Augie Fackler's message of 2017-07-14 12:09:10 -0400:
>> >> If there's a way to get just "new review" messages from phabricator
>> >> into the list, let's do that. I _do not_ want us to stop notifying the
>> >> list of phabricator reviews, because then people that don't watch
>> >> phabricator have no visibility into changes happening there, and we
>> >> split our community.
>> >
>> > My feeling is that both people who care about, and who don't care about
>> > Phabricator will want to send those emails to /dev/null. Reasons:
>> >
>> >   - For people not using Phabricator, they surely don't care about the
>> > notifications and want to nuke them.
>>
>> This is just plain wrong. For people like Danek and my coworkers (and
>> others, I'm sure), there is value to seeing the discussion and code
>> review comments. Even if it's just to skim and get a gist of things.
>
> Sorry if I was unclear. The category refers to those who completely don't
> care about Phabricator, including comments posting there.

Huh? But that's the thing, isn't it? Previously, comments about code
review are on the list. One can peruse them at leisure or archive /
skip, etc. What you're saying, if I'm not mistaken, is that some
comments (no matter the content) will no longer be on the list. That
makes perusing them now split into two locations.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Jun Wu

Excerpts from Sean Farley's message of 2017-07-14 16:10:02 -0700:
> 
> Jun Wu  writes:
> 
> > Excerpts from Augie Fackler's message of 2017-07-14 12:09:10 -0400:
> >> If there's a way to get just "new review" messages from phabricator
> >> into the list, let's do that. I _do not_ want us to stop notifying the
> >> list of phabricator reviews, because then people that don't watch
> >> phabricator have no visibility into changes happening there, and we
> >> split our community.
> >
> > My feeling is that both people who care about, and who don't care about
> > Phabricator will want to send those emails to /dev/null. Reasons:
> >
> >   - For people not using Phabricator, they surely don't care about the
> > notifications and want to nuke them.
> 
> This is just plain wrong. For people like Danek and my coworkers (and
> others, I'm sure), there is value to seeing the discussion and code
> review comments. Even if it's just to skim and get a gist of things.

Sorry if I was unclear. The category refers to those who completely don't
care about Phabricator, including comments posting there.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Jun Wu  writes:

> Excerpts from Augie Fackler's message of 2017-07-14 12:09:10 -0400:
>> If there's a way to get just "new review" messages from phabricator
>> into the list, let's do that. I _do not_ want us to stop notifying the
>> list of phabricator reviews, because then people that don't watch
>> phabricator have no visibility into changes happening there, and we
>> split our community.
>
> My feeling is that both people who care about, and who don't care about
> Phabricator will want to send those emails to /dev/null. Reasons:
>
>   - For people not using Phabricator, they surely don't care about the
> notifications and want to nuke them.

This is just plain wrong. For people like Danek and my coworkers (and
others, I'm sure), there is value to seeing the discussion and code
review comments. Even if it's just to skim and get a gist of things.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Danek Duvall  writes:

> Gregory Szorc wrote:
>
>> > On Jul 14, 2017, at 06:30, Yuya Nishihara  wrote:
>> > 
>> >> On Wed, 12 Jul 2017 11:52:23 -0700, Sean Farley wrote:
>> >> Sean Farley  writes:
>> >>> Adrian Buehlmann  writes:
>> > On 2017-07-10 22:00, Kevin Bullock wrote:
>> > Greetings, Mercurial hackers!
>> > 
>> > We've set up an instance of Phabricator that we're going to experiment 
>> > with for code reviews. At this time, we're not planning on using any 
>> > of the other features of Phabricator, and the use of Phabricator for 
>> > reviews is very much *an experiment* - we intend to see how this is 
>> > going in a little while (probably around the start of the 4.4 cycle). 
>> > If you're more comfortable submitting and reviewing patches as plain 
>> > email, that's fine - use of Phabricator is optional. In the meantime, 
>> > you'll see e-mails coming to the mailing list when activity happens in 
>> > Phabricator--hopefully we've hit the right settings to make this 
>> > useful without being too chatty.
>> > 
>> > See https://www.mercurial-scm.org/wiki/Phabricator for a little more 
>> > detail, including how to submit new reviews.
>>  
>>  Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
>>  lurking the list).
>>  
>>  I'm likely going to unsubscribe Mercurial-devel, if it stays like 
>>  that...
>> >>> 
>> >>> Yeah, I've found this to be pretty ugly as well; though I don't think
>> >>> it's a function of chatty-ness but rather lack of threading :-(
>> >> 
>> >> Another thing that I'd like: can we drop the "[Differential] [Foo
>> >> blah]"? It seems mostly useless information and wastes precious
>> >> horizontal real estate.
>> > 
>> > Agrees, unfortunately. It's too much for casual contributors. I've set up
>> > filtering rule to blackhole these emails so I can stay sane.
>> 
>> It hasn't helped that the list has been extremely active this week. I took 
>> most of a few days off and too was overwhelmed at the volume. Things should 
>> hopefully quiet down next week after the freeze.
>> 
>> Although, given the volume of patches these days, I do question if splitting 
>> up the mailing list might be fruitful. We could establish a new list for the 
>> fire hose of review activity. The dev list could potentially be just for 
>> general discussion. Or we could potentially send just the "new review" 
>> emails to the dev list. I'm not convinced that we should do this. Just 
>> thought I'd throw an idea out there.
>
> Honestly, I'd rather not.  I don't read most of the code review emails, but
> I like seeing them filter through, since it gives me a chance to scan
> through things that might be relevant -- either to Solaris, or our
> extension, or our team, generally.

Yeah, this is my feeling as well. I do find value in skimming messages
and seeing the discussion.

And so far I agree with Augie that my experience has been negative with
phabricator.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Jun Wu

Excerpts from Augie Fackler's message of 2017-07-14 12:09:10 -0400:
> If there's a way to get just "new review" messages from phabricator
> into the list, let's do that. I _do not_ want us to stop notifying the
> list of phabricator reviews, because then people that don't watch
> phabricator have no visibility into changes happening there, and we
> split our community.

My feeling is that both people who care about, and who don't care about
Phabricator will want to send those emails to /dev/null. Reasons:

  - For people not using Phabricator, they surely don't care about the
notifications and want to nuke them.
  - For people active on Phabricator, they read the notification there and
don't need additional email notifications. (I belong to this category
and I send those emails to /dev/null already).

So it seems to me that the only value of getting those emails is the
"achieving" purpose. One brainstorm idea from an internal discussion is to
send the emails to a separated list. It seems the clang community is doing
something similar - they have "-dev" list and "-commits" list. I guess
nobody really reads the "-commits" list and that seems for achieving only.

> (So far I'm not at all happy with Phabricator and how it's split the
> review load in an awkward way - for this reviewer, it's been a clear
> negative.)
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Danek Duvall

Gregory Szorc wrote:

> > On Jul 14, 2017, at 06:30, Yuya Nishihara  wrote:
> > 
> >> On Wed, 12 Jul 2017 11:52:23 -0700, Sean Farley wrote:
> >> Sean Farley  writes:
> >>> Adrian Buehlmann  writes:
> > On 2017-07-10 22:00, Kevin Bullock wrote:
> > Greetings, Mercurial hackers!
> > 
> > We've set up an instance of Phabricator that we're going to experiment 
> > with for code reviews. At this time, we're not planning on using any of 
> > the other features of Phabricator, and the use of Phabricator for 
> > reviews is very much *an experiment* - we intend to see how this is 
> > going in a little while (probably around the start of the 4.4 cycle). 
> > If you're more comfortable submitting and reviewing patches as plain 
> > email, that's fine - use of Phabricator is optional. In the meantime, 
> > you'll see e-mails coming to the mailing list when activity happens in 
> > Phabricator--hopefully we've hit the right settings to make this useful 
> > without being too chatty.
> > 
> > See https://www.mercurial-scm.org/wiki/Phabricator for a little more 
> > detail, including how to submit new reviews.
>  
>  Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
>  lurking the list).
>  
>  I'm likely going to unsubscribe Mercurial-devel, if it stays like that...
> >>> 
> >>> Yeah, I've found this to be pretty ugly as well; though I don't think
> >>> it's a function of chatty-ness but rather lack of threading :-(
> >> 
> >> Another thing that I'd like: can we drop the "[Differential] [Foo
> >> blah]"? It seems mostly useless information and wastes precious
> >> horizontal real estate.
> > 
> > Agrees, unfortunately. It's too much for casual contributors. I've set up
> > filtering rule to blackhole these emails so I can stay sane.
> 
> It hasn't helped that the list has been extremely active this week. I took 
> most of a few days off and too was overwhelmed at the volume. Things should 
> hopefully quiet down next week after the freeze.
> 
> Although, given the volume of patches these days, I do question if splitting 
> up the mailing list might be fruitful. We could establish a new list for the 
> fire hose of review activity. The dev list could potentially be just for 
> general discussion. Or we could potentially send just the "new review" emails 
> to the dev list. I'm not convinced that we should do this. Just thought I'd 
> throw an idea out there.

Honestly, I'd rather not.  I don't read most of the code review emails, but
I like seeing them filter through, since it gives me a chance to scan
through things that might be relevant -- either to Solaris, or our
extension, or our team, generally.  I'm okay subscribing to another list
just to keep getting those, but then we come back to the format of the
emails (subject and body), and those could definitely be cleaned up, IMO.

Danek
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Augie Fackler

On Fri, Jul 14, 2017 at 07:48:24AM -0700, Gregory Szorc wrote:
> > On Jul 14, 2017, at 06:30, Yuya Nishihara  wrote:
> >> On Wed, 12 Jul 2017 11:52:23 -0700, Sean Farley wrote:
> >> Sean Farley  writes:
> >>> Adrian Buehlmann  writes:
> > On 2017-07-10 22:00, Kevin Bullock wrote:
>  Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
>  lurking the list).
> 
>  I'm likely going to unsubscribe Mercurial-devel, if it stays like that...
> >>>
> >>> Yeah, I've found this to be pretty ugly as well; though I don't think
> >>> it's a function of chatty-ness but rather lack of threading :-(
> >>
> >> Another thing that I'd like: can we drop the "[Differential] [Foo
> >> blah]"? It seems mostly useless information and wastes precious
> >> horizontal real estate.
> >
> > Agrees, unfortunately. It's too much for casual contributors. I've set up
> > filtering rule to blackhole these emails so I can stay sane.
>
> It hasn't helped that the list has been extremely active this week. I took 
> most of a few days off and too was overwhelmed at the volume. Things should 
> hopefully quiet down next week after the freeze.
>
> Although, given the volume of patches these days, I do question if splitting 
> up the mailing list might be fruitful. We could establish a new list for the 
> fire hose of review activity. The dev list could potentially be just for 
> general discussion. Or we could potentially send just the "new review" emails 
> to the dev list. I'm not convinced that we should do this. Just thought I'd 
> throw an idea out there.

If there's a way to get just "new review" messages from phabricator
into the list, let's do that. I _do not_ want us to stop notifying the
list of phabricator reviews, because then people that don't watch
phabricator have no visibility into changes happening there, and we
split our community.

(So far I'm not at all happy with Phabricator and how it's split the
review load in an awkward way - for this reviewer, it's been a clear negative.)

> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Gregory Szorc

> On Jul 14, 2017, at 06:30, Yuya Nishihara  wrote:
> 
>> On Wed, 12 Jul 2017 11:52:23 -0700, Sean Farley wrote:
>> Sean Farley  writes:
>>> Adrian Buehlmann  writes:
> On 2017-07-10 22:00, Kevin Bullock wrote:
> Greetings, Mercurial hackers!
> 
> We've set up an instance of Phabricator that we're going to experiment 
> with for code reviews. At this time, we're not planning on using any of 
> the other features of Phabricator, and the use of Phabricator for reviews 
> is very much *an experiment* - we intend to see how this is going in a 
> little while (probably around the start of the 4.4 cycle). If you're more 
> comfortable submitting and reviewing patches as plain email, that's fine 
> - use of Phabricator is optional. In the meantime, you'll see e-mails 
> coming to the mailing list when activity happens in 
> Phabricator--hopefully we've hit the right settings to make this useful 
> without being too chatty.
> 
> See https://www.mercurial-scm.org/wiki/Phabricator for a little more 
> detail, including how to submit new reviews.

 Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
 lurking the list).

 I'm likely going to unsubscribe Mercurial-devel, if it stays like that...
>>> 
>>> Yeah, I've found this to be pretty ugly as well; though I don't think
>>> it's a function of chatty-ness but rather lack of threading :-(
>> 
>> Another thing that I'd like: can we drop the "[Differential] [Foo
>> blah]"? It seems mostly useless information and wastes precious
>> horizontal real estate.
> 
> Agrees, unfortunately. It's too much for casual contributors. I've set up
> filtering rule to blackhole these emails so I can stay sane.

It hasn't helped that the list has been extremely active this week. I took most 
of a few days off and too was overwhelmed at the volume. Things should 
hopefully quiet down next week after the freeze.

Although, given the volume of patches these days, I do question if splitting up 
the mailing list might be fruitful. We could establish a new list for the fire 
hose of review activity. The dev list could potentially be just for general 
discussion. Or we could potentially send just the "new review" emails to the 
dev list. I'm not convinced that we should do this. Just thought I'd throw an 
idea out there.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-14 Thread Yuya Nishihara

On Wed, 12 Jul 2017 11:52:23 -0700, Sean Farley wrote:
> Sean Farley  writes:
> > Adrian Buehlmann  writes:
> >> On 2017-07-10 22:00, Kevin Bullock wrote:
> >>> Greetings, Mercurial hackers!
> >>> 
> >>> We've set up an instance of Phabricator that we're going to experiment 
> >>> with for code reviews. At this time, we're not planning on using any of 
> >>> the other features of Phabricator, and the use of Phabricator for reviews 
> >>> is very much *an experiment* - we intend to see how this is going in a 
> >>> little while (probably around the start of the 4.4 cycle). If you're more 
> >>> comfortable submitting and reviewing patches as plain email, that's fine 
> >>> - use of Phabricator is optional. In the meantime, you'll see e-mails 
> >>> coming to the mailing list when activity happens in 
> >>> Phabricator--hopefully we've hit the right settings to make this useful 
> >>> without being too chatty.
> >>> 
> >>> See https://www.mercurial-scm.org/wiki/Phabricator for a little more 
> >>> detail, including how to submit new reviews.
> >>
> >> Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
> >> lurking the list).
> >>
> >> I'm likely going to unsubscribe Mercurial-devel, if it stays like that...
> >
> > Yeah, I've found this to be pretty ugly as well; though I don't think
> > it's a function of chatty-ness but rather lack of threading :-(
> 
> Another thing that I'd like: can we drop the "[Differential] [Foo
> blah]"? It seems mostly useless information and wastes precious
> horizontal real estate.

Agrees, unfortunately. It's too much for casual contributors. I've set up
filtering rule to blackhole these emails so I can stay sane.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

> Unless you're talking about Phabricator's webui. Then I don't know
because I don't use it.

I was, sorry for the ambiguity.

On Wed, Jul 12, 2017 at 1:42 PM, Sean Farley  wrote:
>
> Phillip Cohen  writes:
>
>> I'm using the webui, though, so it all looks the same.
>
> But with different mailing lists, it's now solvable through your own
> client: filtering based on the headers.
>
> Unless you're talking about Phabricator's webui. Then I don't know
> because I don't use it.
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Phillip Cohen  writes:

> I'm using the webui, though, so it all looks the same.

But with different mailing lists, it's now solvable through your own
client: filtering based on the headers.

Unless you're talking about Phabricator's webui. Then I don't know
because I don't use it.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

I'm using the webui, though, so it all looks the same.

On Wed, Jul 12, 2017 at 1:35 PM, Sean Farley  wrote:
>
> Phillip Cohen  writes:
>
>>> Also: Jun has suggested to me that we should avoid using "Accept
>>> Commit", because that should be a state applied only when a commit is
>>> queued to `hg-committed` (either by the reviewer or by a future bot).
>>> Once the commit goes to `hg` it should be automatically closed (this
>>> should happen today out of the box).
>>>
>>> Those two things make sense to me, but it's the first I've heard of
>>> it, and wanted to make sure there was consensus.
>>
>> Although, hm, I find this rather annoying because I have to check
>> whether a review is a core review or an fb-hgext review before I know
>> if I can use the Accept functionality. :/
>
> Historically, this is done via different mailing lists: hgsubversion,
> hg-git, etc. are all separate lists. I would think fb-hgext and whatnot
> could be separate lists, too, no?
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Phillip Cohen  writes:

>> Also: Jun has suggested to me that we should avoid using "Accept
>> Commit", because that should be a state applied only when a commit is
>> queued to `hg-committed` (either by the reviewer or by a future bot).
>> Once the commit goes to `hg` it should be automatically closed (this
>> should happen today out of the box).
>>
>> Those two things make sense to me, but it's the first I've heard of
>> it, and wanted to make sure there was consensus.
>
> Although, hm, I find this rather annoying because I have to check
> whether a review is a core review or an fb-hgext review before I know
> if I can use the Accept functionality. :/

Historically, this is done via different mailing lists: hgsubversion,
hg-git, etc. are all separate lists. I would think fb-hgext and whatnot
could be separate lists, too, no?


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

> Augie just added a herald rule that adds the hg-reviewers as a
> blocking reviewer to all HG reviews.  So you can accept
> whatever diffs you want, to indicate you > are happy with them,
> but the diff as a whole won't be "Accepted" until someone from
> hg-reviewers accepts it.

That works for me! Thanks.

> Another thing that I'd like: can we drop the "[Differential] [Foo
> blah]"? It seems mostly useless information and wastes precious
> horizontal real estate.

I think the only limitation is that (to my knowledge, based on Jun's
phabsend API choices) we're trying to avoid patching our local
Phabricator instance and keep it in sync with the upstream version
(maintainers of our Phab instance, please correct me if I'm wrong). If
so we'd have to achieve this via existing Phabricator configs or
extensions. Looking upstream (https://secure.phabricator.com/T5244,
https://secure.phabricator.com/D9342), it looks like this is something
they don't want to support. But curiously, in the latter link, the
creator suggested just patching it locally.

On Wed, Jul 12, 2017 at 12:44 PM, Durham Goode  wrote:
> On 7/12/17 12:41 PM, Phillip Cohen wrote:
>>>
>>> Also: Jun has suggested to me that we should avoid using "Accept
>>> Commit", because that should be a state applied only when a commit is
>>> queued to `hg-committed` (either by the reviewer or by a future bot).
>>> Once the commit goes to `hg` it should be automatically closed (this
>>> should happen today out of the box).
>>>
>>> Those two things make sense to me, but it's the first I've heard of
>>> it, and wanted to make sure there was consensus.
>>
>>
>> Although, hm, I find this rather annoying because I have to check
>> whether a review is a core review or an fb-hgext review before I know
>> if I can use the Accept functionality. :/
>
>
> Augie just added a herald rule that adds the hg-reviewers as a blocking
> reviewer to all HG reviews.  So you can accept whatever diffs you want, to
> indicate you are happy with them, but the diff as a whole won't be
> "Accepted" until someone from hg-reviewers accepts it.
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-12 Thread Durham Goode


On 7/12/17 12:41 PM, Phillip Cohen wrote:

Also: Jun has suggested to me that we should avoid using "Accept
Commit", because that should be a state applied only when a commit is
queued to `hg-committed` (either by the reviewer or by a future bot).
Once the commit goes to `hg` it should be automatically closed (this
should happen today out of the box).

Those two things make sense to me, but it's the first I've heard of
it, and wanted to make sure there was consensus.


Although, hm, I find this rather annoying because I have to check
whether a review is a core review or an fb-hgext review before I know
if I can use the Accept functionality. :/


Augie just added a herald rule that adds the hg-reviewers as a blocking 
reviewer to all HG reviews.  So you can accept whatever diffs you want, 
to indicate you are happy with them, but the diff as a whole won't be 
"Accepted" until someone from hg-reviewers accepts it.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

> Also: Jun has suggested to me that we should avoid using "Accept
> Commit", because that should be a state applied only when a commit is
> queued to `hg-committed` (either by the reviewer or by a future bot).
> Once the commit goes to `hg` it should be automatically closed (this
> should happen today out of the box).
>
> Those two things make sense to me, but it's the first I've heard of
> it, and wanted to make sure there was consensus.

Although, hm, I find this rather annoying because I have to check
whether a review is a core review or an fb-hgext review before I know
if I can use the Accept functionality. :/

> Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
> lurking the list).

I wouldn't despair; styling can be changed.

On Wed, Jul 12, 2017 at 11:55 AM, Durham Goode  wrote:
> On 7/12/17 9:21 AM, Durham Goode wrote:
>>
>>
>> On 7/10/17 1:00 PM, Kevin Bullock wrote:
>>>
>>> Greetings, Mercurial hackers!
>>>
>>> We've set up an instance of Phabricator that we're going to experiment
>>> with for code reviews. At this time, we're not planning on using any
>>> of the other features of Phabricator, and the use of Phabricator for
>>> reviews is very much *an experiment* - we intend to see how this is
>>> going in a little while (probably around the start of the 4.4 cycle).
>>> If you're more comfortable submitting and reviewing patches as plain
>>> email, that's fine - use of Phabricator is optional. In the meantime,
>>> you'll see e-mails coming to the mailing list when activity happens in
>>> Phabricator--hopefully we've hit the right settings to make this
>>> useful without being too chatty.
>>>
>>> See
>>>
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_Phabricator&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=QbsrhmbnW2idG1v6iKqrCkrKcr0GyzjxWOfT4annRhM&s=fFYzxmctyERZH1yeaOlZoajPSmBKUtxCO4mr2OzRwok&e=
>>> for a little more detail, including how to submit new reviews.
>>>
>>> Thanks,
>>> Augie and Kevin
>>
>>
>> I've created three projects in phabricator that can be used for tagging
>> code reviews and creating herald rules around.
>>
>> If you add yourself to the 'hg' project, you will get notified of any
>> review that contains #hg in the reviewers list.
>>
>> Add yourself to 'fbhgext' (#fb or #fbhgext) to get notified of patches
>> for the facebook hg experimental repo.
>>
>> Add yourself to 'east' (#easy) to get notified or see reviews that are
>> marked as easy.
>
>
> I've created two dashboards for Phabricator.
>
> One for HG reviews:
> https://phab.mercurial-scm.org/dashboard/view/1/
>
> One for fb-hgext reviews:
> https://phab.mercurial-scm.org/dashboard/view/2/
>
> You can add them to your home page by clicking the "Install Dashboard"
> button in the top right, then choosing "Personal". This way, when I go to
> https://phab.mercurial-scm.org/ I see the core HG reviews front and center.
>
> The only problem is they currently don't group them by stack, and there's no
> way to distinguish between a review that is receiving comments and one that
> hasn't received any comments at all.
>
> ___
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews

2017-07-12 Thread Durham Goode

On 7/12/17 9:21 AM, Durham Goode wrote:

On 7/10/17 1:00 PM, Kevin Bullock wrote:

Greetings, Mercurial hackers!

We've set up an instance of Phabricator that we're going to experiment
with for code reviews. At this time, we're not planning on using any
of the other features of Phabricator, and the use of Phabricator for
reviews is very much *an experiment* - we intend to see how this is
going in a little while (probably around the start of the 4.4 cycle).
If you're more comfortable submitting and reviewing patches as plain
email, that's fine - use of Phabricator is optional. In the meantime,
you'll see e-mails coming to the mailing list when activity happens in
Phabricator--hopefully we've hit the right settings to make this
useful without being too chatty.

See
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_wiki_Phabricator&d=DwIGaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=nuarHzhP1wi1T9iURRCj1A&m=QbsrhmbnW2idG1v6iKqrCkrKcr0GyzjxWOfT4annRhM&s=fFYzxmctyERZH1yeaOlZoajPSmBKUtxCO4mr2OzRwok&e=
for a little more detail, including how to submit new reviews.

Thanks,
Augie and Kevin

I've created three projects in phabricator that can be used for tagging
code reviews and creating herald rules around.

If you add yourself to the 'hg' project, you will get notified of any
review that contains #hg in the reviewers list.

Add yourself to 'fbhgext' (#fb or #fbhgext) to get notified of patches
for the facebook hg experimental repo.

Add yourself to 'east' (#easy) to get notified or see reviews that are
marked as easy.

I've created two dashboards for Phabricator.

One for HG reviews:
https://phab.mercurial-scm.org/dashboard/view/1/

One for fb-hgext reviews:
https://phab.mercurial-scm.org/dashboard/view/2/

You can add them to your home page by clicking the "Install Dashboard"
button in the top right, then choosing "Personal". This way, when I go
to https://phab.mercurial-scm.org/ I see the core HG reviews front and
center.

The only problem is they currently don't group them by stack, and
there's no way to distinguish between a review that is receiving
comments and one that hasn't received any comments at all.

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews


Sean Farley  writes:

> Adrian Buehlmann  writes:
>
>> On 2017-07-10 22:00, Kevin Bullock wrote:
>>> Greetings, Mercurial hackers!
>>> 
>>> We've set up an instance of Phabricator that we're going to experiment with 
>>> for code reviews. At this time, we're not planning on using any of the 
>>> other features of Phabricator, and the use of Phabricator for reviews is 
>>> very much *an experiment* - we intend to see how this is going in a little 
>>> while (probably around the start of the 4.4 cycle). If you're more 
>>> comfortable submitting and reviewing patches as plain email, that's fine - 
>>> use of Phabricator is optional. In the meantime, you'll see e-mails coming 
>>> to the mailing list when activity happens in Phabricator--hopefully we've 
>>> hit the right settings to make this useful without being too chatty.
>>> 
>>> See https://www.mercurial-scm.org/wiki/Phabricator for a little more 
>>> detail, including how to submit new reviews.
>>
>> Ugh. Looks way too chatty/cluttered for my taste so far (I'm mostly
>> lurking the list).
>>
>> I'm likely going to unsubscribe Mercurial-devel, if it stays like that...
>
> Yeah, I've found this to be pretty ugly as well; though I don't think
> it's a function of chatty-ness but rather lack of threading :-(

Another thing that I'd like: can we drop the "[Differential] [Foo
blah]"? It seems mostly useless information and wastes precious
horizontal real estate.


signature.asc
Description: PGP signature
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: Experimenting with Phabricator for reviews