[Reviewers] Recommending Phabricator

[Ryan McElroy]

On 2/8/18 2:19 PM, Augie Fackler wrote:
> (+Ryan since he's got a contact at Phacility)

Indeed, and I bought a contract with the Phacility company for a few 
months after the last sprint to talk about some possible changes. I got 
a special deal while I tried to convince FB to pay for the contract -- 
but since Evan Preistley (main guy at Phacility) wasn't into the 
directions we wanted to go, he only offered to produce one-off patches 
that wouldn't be maintained for many of the ideas I solicited from the 
community after that sprint.

>
>> On Feb 8, 2018, at 13:03, Yuya Nishihara <yuya at tcha.org> wrote:
>>
>> On Wed, 7 Feb 2018 09:54:39 -0800, Gregory Szorc wrote:
>>> One of the changes I made was to recommend Phabricator over emailing
>>> patches. I did this because I think Phabricator is more friendly to new
>>> contributors and provides a gateway for new contributors getting more
>>> involved.

I'm +1 on more Phabricator, but I'm not reviewing much right now so you 
should ignore my opinion on this.

>> I hope not advertising Phabricator more until its email interface gets fixed.
>> Proper threading and inline-reply are must IMO.

The main reason Evan pushed back on this is that he views emails from 
phabricator as "notifications" more than "mediums for consumption". He 
thinks that the main interface for phabricator should remain the 
website, and that it should not be a tool for review-over-email. He 
suggested that he could be paid to write a one-off patch that would 
produce emails the way we want, but that future changes might (and 
likely would) break this patch, and we'd have to maintain it, or pay for 
an updated patch.

>>
>>> In addition, I think the time has come for more of Mercurial's core
>>> contributors to transition from email to Phabricator.
>> I thought that was a temporary issue of email infrastructure at Google?

I'm not sure what Google's email infra has to do with this; I think Greg 
is suggesting that more core hg contributors (including everyone in this 
list) start submitting their patches to phabricator.

>>
>>> As a reviewer, I've noticed I'm biasing to looking at Phabricator first
>>> then falling back to Patchwork if I still want to do reviews. Phabricator
>>> is just a more pleasant experience for me as both a reviewer and a
>>> contributor.
>> I do truly the opposite. Ignoring almost all Phabricator emails, and pick
>> random patches, which I would have to review, from Yadda using phabread.

This may just be one the the "great debates" like emacs vs vim, that 
will never be fully resolved. I certainly have found it far easier to 
deal with patches from phabricator than I did dealing with email 
patches, but I have years of experience with a phabricator-like workflow.

The key difference for me is that email might be slightly better if I 
plan to read every patch. It's almost impossible, without more advanced 
tooling, to effectively ignore future versions of a patch I'm not 
interested in (because of v2, v3, etc).

However, during the times I have spent less time reviewing (like 
currently), phabricator is great for me to do "drive-by" reviews, 
occasional spurts of reviewing, etc. All the context is collected into a 
single place for the given patch; I don't have to search my inbox or set 
up tooling to make it work well.

I agree that using phabricator via email is not great. As I mentioned 
above, there are no plans at Phacility to make it great, or even 
particularly better. Instead, the expectation is that you mostly use the 
web interface, and that email will always be a somewhat supported but 
very second-class citizen.

> I still check both. I'm quicker at email reviews and find them preferable, but I've also made peace with the seeming reality that new contributors are significantly more comfortable with phabricator.
>
> I would like it if we could try and get some more improvements figured out, but IIRC the email threading thing was a complete nonstarter for them? Greg, do you know if Mozilla is likely to care about that?

Yeah, email threading was a non-starter. My request and Evan's full 
response is copied below:

"""

Consider achain of inline comments like this 
<https://phab.mercurial-scm.org/D821?id=2092#inline-2583>.

Today, phabricator sends out an email with the previous comment quoted 
like so:

ryanmce added inline comments.

INLINE COMMENTS

> ryanmce wrote in uncommit.py:260
> Test test

Tested nested comments, sorry for the spam.

One thing that at least some people would like would be for *all* the 
previous inline comments to be nested, so all the context is there in an 
email, for offline perusing (and other stuff I guess). So in this 
example, the generated email would be like this:

ryanmce added inline comments.

INLINE COMMENTS

>> durham wrote in uncommit.py:260
>> This should be in a with statement probably? Can we just have it be as part of the top level lock with statement?
> ryanmce wrote in uncommit.py:260
> Test test

Tested nested comments, sorry for the spam.

(yeah yeah, I wrote "tested" instead of "testing" -- feel free to rib me 
for that)

"""

Response:

"""

The current design is an explicit implementation decision. SeeT10694 
<https://secure.phabricator.com/T10694>andT10283 
<https://secure.phabricator.com/T10283>for history/context. Note that 
the install there wanted Phabricator to work like Review Board, vs 
wanting Phabricator to work like sending patch files to a mailing list.

The entire context is always available in the thread (e.g., some other 
mail in the thread has at durham's comment), and each mail is not intended 
to be a screenshot of the web UI at the time the mail was sent. If you 
want to see what the web UI looks like, click the link to the revision 
or the large button prominently available in the header of each email. 
If users have a good reason for needing an offline mode, let's look at 
what they need -- but I don't want to burden the 99% (99.9%?) of users 
who are usually online for a handful of outlying use cases, especially 
if the reason is primarily cost (e.g., lack of access to mobile uplinks 
while traveling). If Mercurial was a corporation, it would probably be 
much cheaper to pay for mobile uplinks for all affected developers than 
to engage us to develop offline mode support. If Mercurial wants to 
solve a cost problem by shifting the burden to us, that's a great 
cost-saving strategy, but this isn't something that makes any business 
sense for us.

Long ago (2012?), LLVM got a patch into the upstream for this but we 
wised up over the years and I removed it inD15850 
<https://secure.phabricator.com/D15850>. I believe no other install ever 
used it -- at the least, no one objected when it stopped working, and 
installs stopped complaining about the feature being awful and ugly and 
started complimenting the change. I think LLVM still maintains some kind 
of patch locally, andsee here 
<https://secure.phabricator.com/T10694#196864>for guesses about how to 
build such a patch. I can write this patch for you, but usual 
mana/unmainted caveats apply.

"""



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-reviewers/attachments/20180209/27e88218/attachment-0002.html>