llvm dev - Jun 2020 - [cfe-dev] Phabricator Maintenance

Relatedly, Phabricator doesn't stop you continuing a comment chain for
reasons I have yet to follow, which Github sometimes does.

Some others:
1) I believe Github also doesn't have an easy way to respond to multiple
comments simultaneously, if you are not in "review" mode, (which is
always
the case if you are replying to out-of-line comments).
2) Typically in our Phabricator, the description and summary of a patch is
considered to be roughly what goes in the final commit message (same with
Github). However, updating said commit message isn't possible to my
knowledge without force-pushing in Github (which as discussed elsewhere in
the thread causes other problems).
3) Marking comments as "Done" in Phabricator does not auto-hide them,
whereas in Github marking a comment as "Resolved" does. Spoken from
experience, this leads to situations in Github where a developer thinks
they've resolved a reviewers comments, marks them as Resolved, but actually
they weren't. The reviewer in turn then has to browse the comments in the
summary page, to see if they have any unaddressed comments that were
supposedly resolved, which given the limited context available there is a
non-trivial task sometimes.

There are probably others, but those are the ones not already mentioned
that I can come up with so far.

P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to
working with Github towards getting the feature parity with Phabricator
sorted, I don't want to switch until the two are on a par with each other,
which in my opinion is not yet, so being forced to do so prematurely due to
lack of maintainers would have made me very sad (P.P.S thank you Manuel for
Phabricator in the first place!).

On Thu, 25 Jun 2020 at 22:39, Hubert Tong via cfe-dev <
cfe-dev at lists.llvm.org> wrote:
> On Thu, Jun 25, 2020 at 12:29 PM Mehdi AMINI via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>>
>> Re: patch chains - perhaps we can ask github on support for that?
>>> I think what would help is somebody providing a doc with the
features
>>> that phab provides that github PR doesn't, so we can get some
consensus on
>>> what the diff is.
>>>
>>
>> Indeed, I'll try to start a doc on this.
>>
> Thanks. Some things that are high on my list:
> Phabricator has persistent per-user inline comment collapsing.
> Phabricator can show all inline comments over the lifetime of the patch
> in-line (even if the placement is not perfect) in the full diff view.
>
> _______________________________________________
> cfe-dev mailing list
> cfe-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200626/879a3623/attachment.html>

On 2020-06-26 09:53, James Henderson via llvm-dev wrote:
> Relatedly, Phabricator doesn't stop you continuing a comment chain for
> reasons I have yet to follow, which Github sometimes does.
> 
> Some others:
> 1) I believe Github also doesn't have an easy way to respond to 
> multiple
> comments simultaneously, if you are not in "review" mode, (which
is
> always
> the case if you are replying to out-of-line comments).
> 2) Typically in our Phabricator, the description and summary of a patch 
> is
> considered to be roughly what goes in the final commit message (same 
> with
> Github). However, updating said commit message isn't possible to my
> knowledge without force-pushing in Github (which as discussed elsewhere 
> in
> the thread causes other problems).
> 3) Marking comments as "Done" in Phabricator does not auto-hide
them,
> whereas in Github marking a comment as "Resolved" does. Spoken
from
> experience, this leads to situations in Github where a developer thinks
> they've resolved a reviewers comments, marks them as Resolved, but 
> actually
> they weren't. The reviewer in turn then has to browse the comments in 
> the
> summary page, to see if they have any unaddressed comments that were
> supposedly resolved, which given the limited context available there is 
> a
> non-trivial task sometimes.
In practice, I’ve found that these two functionalities are used in 
different ways. In Phabricator, the implementer marks the comments as 
done. In GitHub, the reviewer marks comments are resolved. Actually, I 
would like to have both, but I’m not aware of a review software that has 
both.
> There are probably others, but those are the ones not already mentioned
> that I can come up with so far.
> 
> P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to
> working with Github towards getting the feature parity with Phabricator
> sorted, I don't want to switch until the two are on a par with each 
> other,
> which in my opinion is not yet, so being forced to do so prematurely 
> due to
> lack of maintainers would have made me very sad (P.P.S thank you Manuel 
> for
> Phabricator in the first place!).
> 
> On Thu, 25 Jun 2020 at 22:39, Hubert Tong via cfe-dev <
> cfe-dev at lists.llvm.org> wrote:
> 
>> On Thu, Jun 25, 2020 at 12:29 PM Mehdi AMINI via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> 
>>> 
>>> Re: patch chains - perhaps we can ask github on support for that?
>>>> I think what would help is somebody providing a doc with the 
>>>> features
>>>> that phab provides that github PR doesn't, so we can get
some
>>>> consensus on
>>>> what the diff is.
>>>> 
>>> 
>>> Indeed, I'll try to start a doc on this.
>>> 
>> Thanks. Some things that are high on my list:
>> Phabricator has persistent per-user inline comment collapsing.
>> Phabricator can show all inline comments over the lifetime of the 
>> patch
>> in-line (even if the placement is not perfect) in the full diff view.
>> 
>> _______________________________________________
>> cfe-dev mailing list
>> cfe-dev at lists.llvm.org
>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>> 
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On Fri, 26 Jun 2020 at 09:53, Manuel Jacob <me at manueljacob.de> wrote:
> On 2020-06-26 09:53, James Henderson via llvm-dev wrote:
> > Relatedly, Phabricator doesn't stop you continuing a comment chain
for
> > reasons I have yet to follow, which Github sometimes does.
> >
> > Some others:
> > 1) I believe Github also doesn't have an easy way to respond to
> > multiple
> > comments simultaneously, if you are not in "review" mode,
(which is
> > always
> > the case if you are replying to out-of-line comments).
> > 2) Typically in our Phabricator, the description and summary of a
patch
> > is
> > considered to be roughly what goes in the final commit message (same
> > with
> > Github). However, updating said commit message isn't possible to
my
> > knowledge without force-pushing in Github (which as discussed
elsewhere
> > in
> > the thread causes other problems).
> > 3) Marking comments as "Done" in Phabricator does not
auto-hide them,
> > whereas in Github marking a comment as "Resolved" does.
Spoken from
> > experience, this leads to situations in Github where a developer
thinks
> > they've resolved a reviewers comments, marks them as Resolved, but
> > actually
> > they weren't. The reviewer in turn then has to browse the comments
in
> > the
> > summary page, to see if they have any unaddressed comments that were
> > supposedly resolved, which given the limited context available there
is
> > a
> > non-trivial task sometimes.
>
> In practice, I’ve found that these two functionalities are used in
> different ways. In Phabricator, the implementer marks the comments as
> done. In GitHub, the reviewer marks comments are resolved. Actually, I
> would like to have both, but I’m not aware of a review software that has
> both.
>
That might be a cultural thing then, and would need documenting under some
sort of "Best Practices when Reviewing" doc, since I ran into this
exact
problem in our downstream Github repo, where we use PRs for reviews.

>
> > There are probably others, but those are the ones not already
mentioned
> > that I can come up with so far.
> >
> > P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed
to
> > working with Github towards getting the feature parity with
Phabricator
> > sorted, I don't want to switch until the two are on a par with
each
> > other,
> > which in my opinion is not yet, so being forced to do so prematurely
> > due to
> > lack of maintainers would have made me very sad (P.P.S thank you
Manuel
> > for
> > Phabricator in the first place!).
> >
> > On Thu, 25 Jun 2020 at 22:39, Hubert Tong via cfe-dev <
> > cfe-dev at lists.llvm.org> wrote:
> >
> >> On Thu, Jun 25, 2020 at 12:29 PM Mehdi AMINI via llvm-dev <
> >> llvm-dev at lists.llvm.org> wrote:
> >>
> >>>
> >>> Re: patch chains - perhaps we can ask github on support for
that?
> >>>> I think what would help is somebody providing a doc with
the
> >>>> features
> >>>> that phab provides that github PR doesn't, so we can
get some
> >>>> consensus on
> >>>> what the diff is.
> >>>>
> >>>
> >>> Indeed, I'll try to start a doc on this.
> >>>
> >> Thanks. Some things that are high on my list:
> >> Phabricator has persistent per-user inline comment collapsing.
> >> Phabricator can show all inline comments over the lifetime of the
> >> patch
> >> in-line (even if the placement is not perfect) in the full diff
view.
> >>
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at lists.llvm.org
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
> >>
> >
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200626/aa4d4fba/attachment.html>

On Fri, Jun 26, 2020 at 12:54 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
>
> 2) Typically in our Phabricator, the description and summary of a patch is
> considered to be roughly what goes in the final commit message (same with
> Github). However, updating said commit message isn't possible to my
> knowledge without force-pushing in Github (which as discussed elsewhere in
> the thread causes other problems).
>
you can definitely edit your PR description in GH.  The thing is that your
commit message and PR description are mostly unrelated.

Your PR description is initialized from the commit message of the first
patch you push.  But you can edit it via the GH UI by clicking the three
dots.

When you go to finally merge your PR, you then have another chance to edit
it (and you usually have to since it joins all commit messages together
into a single string, whereas I almost always just want the original PR
description)
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200626/e3d39283/attachment.html>

On Fri, Jun 26, 2020 at 9:54 AM James Henderson via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> 2) Typically in our Phabricator, the description and summary of a patch is
considered to be roughly what goes in the final commit message (same with
Github). However, updating said commit message isn't possible to my
knowledge without force-pushing in Github (which as discussed elsewhere in the
thread causes other problems).
Rebasing _should_ be how commit messages are changed. Ideally, we'd
treat commit messages as part of the artefact under review, since good
commit messages matter. Phabricator isn't ideal here either: by
default, updating a patch doesn't update its commit message.

On the GitHub side, the real problem here is how easily it loses the
plot when you rebase.

> 3) Marking comments as "Done" in Phabricator does not auto-hide
them, whereas in Github marking a comment as "Resolved" does. Spoken
from experience, this leads to situations in Github where a developer thinks
they've resolved a reviewers comments, marks them as Resolved, but actually
they weren't. The reviewer in turn then has to browse the comments in the
summary page, to see if they have any unaddressed comments that were supposedly
resolved, which given the limited context available there is a non-trivial task
sometimes.
Ideally, the "Resolved" state should be per-user. When multiple people
review a patch, I might not want to duplicate a comment that another
reviewer made, but I would like to confirm for myself whether an issue
was resolved or not.

> P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed to
working with Github towards getting the feature parity with Phabricator sorted,
I don't want to switch until the two are on a par with each other, which in
my opinion is not yet, so being forced to do so prematurely due to lack of
maintainers would have made me very sad (P.P.S thank you Manuel for Phabricator
in the first place!).
Seconded on all counts!

Cheers,
Nicolai

-- 
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.

I want to bubble out of this discussion, because most of the conversation has
been about merit of various tools, how much the cloud license costs, etc.


In my opinion, none of this actually matters.  There are much larger strategic
questions that we should be talking about instead:

1) Why is LLVM special?  We are a tiny community compared to the larger GitHub
community - anything that makes us “weird” (even if weird is better in some
ways) increases impedance mismatch, reduces collaboration with other communities
etc.  Many users of LLVM are already using GitHub for code reviews and PRs etc.

2) Why should compiler nerds :-) be working on this sort of infrastructure?  We
have many talented people who are capable of doing many impressive things, why
spent that energy on this?

3) Even if someone is willing to keep this going, what ongoing liability is this
for the project as a whole?  Phabricator was done for the weekend, did someone’s
pager go off?  What is the SLA/SLO for the new system?

As I mentioned upthread, I like Phabricator and its workflow from a personal
perspective, but from a project perspective, I can’t see any reason to defend
bespoke infra like this.

-Chris



> On Jun 29, 2020, at 4:11 AM, Nicolai Hähnle via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On Fri, Jun 26, 2020 at 9:54 AM James Henderson via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
>> 2) Typically in our Phabricator, the description and summary of a patch
is considered to be roughly what goes in the final commit message (same with
Github). However, updating said commit message isn't possible to my
knowledge without force-pushing in Github (which as discussed elsewhere in the
thread causes other problems).
> 
> Rebasing _should_ be how commit messages are changed. Ideally, we'd
> treat commit messages as part of the artefact under review, since good
> commit messages matter. Phabricator isn't ideal here either: by
> default, updating a patch doesn't update its commit message.
> 
> On the GitHub side, the real problem here is how easily it loses the
> plot when you rebase.
> 
> 
>> 3) Marking comments as "Done" in Phabricator does not
auto-hide them, whereas in Github marking a comment as "Resolved"
does. Spoken from experience, this leads to situations in Github where a
developer thinks they've resolved a reviewers comments, marks them as
Resolved, but actually they weren't. The reviewer in turn then has to browse
the comments in the summary page, to see if they have any unaddressed comments
that were supposedly resolved, which given the limited context available there
is a non-trivial task sometimes.
> 
> Ideally, the "Resolved" state should be per-user. When multiple
people
> review a patch, I might not want to duplicate a comment that another
> reviewer made, but I would like to confirm for myself whether an issue
> was resolved or not.
> 
> 
>> P.S. Thanks Mehdi/Fangrui for stepping up! Whilst I'm not opposed
to working with Github towards getting the feature parity with Phabricator
sorted, I don't want to switch until the two are on a par with each other,
which in my opinion is not yet, so being forced to do so prematurely due to lack
of maintainers would have made me very sad (P.P.S thank you Manuel for
Phabricator in the first place!).
> 
> Seconded on all counts!
> 
> Cheers,
> Nicolai
> 
> -- 
> Lerne, wie die Welt wirklich ist,
> aber vergiss niemals, wie sie sein sollte.
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev