llvm dev - Jan 2020 - [cfe-dev] Phabricator -> GitHub PRs?

> On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> <cfe-dev at lists.llvm.org> wrote:
>> 
>> 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 that.
>>> 
>> It's not hyperbole, but fine.  How do you use it to keep multiple,
related changes in order?
> 
> You can use parent/child revisions. Phabricator encourages a
> patches-based approach with small changes. For me that corresponds to
> one commit per code review. When I address code review feedback in a
> parent revision I use git's interactive rebase.
It's worth mentioning that Phabricator can read strings of the format
'Depends on D1234' from commit messages and create those relationships
for you.

Also just because it's not easy to find unless you know it's there. You
can view the parent/child relationships in the 'Revision Contents'
section on the 'Stack' tab.
>> The interface for reviewing and responding to reviews is horrific, e.g.
quoting text from a review is rather bad, the email it sends is badly formatted
and hard to read. How do you make it reasonably useful?
> 
> Inline comments are super useful, they can be marked as done and
> hidden. I agree that sometimes there's a lot of context when quoting
> text, but the format is very simple (similar to e-mail) so it's easy
> to trim.
> 
>> Why can't I *easily* relate changes to each other?
> 
> What issues do you experience with parent/child revisions?
> 
>> Why can't I submit through the Phabricator interface, but have to
go to the command line, place the change in a new branch, pull to top-of-tree,
rebase, and only then push while hoping it doesn't give fail because the
tree became out of date? How can I do a rebase through Phabricator?
> 
> You can upload patches through
> https://reviews.llvm.org/differential/diff/create/. I personally don't
> use arcanist even though I found it pretty useful in the past.
> 
>> 
>> These are only off the top of my head. There are far more problems
I've had with them.
>> 
>> -bw
>> _______________________________________________
>> 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

Thanks for the tip.

I didn't know that the decision had already been made to stay with
Phabricator. I find Phabricator hard to use and it makes me actively avoid
contributing to LLVM knowing that I'll have to use it. I've read the
documentation, but still have issues. And it's not like git where
there's
an obscure command not everyone knows about that will fix the problem. The
advice I got so far has been to use Phabricator/Arcanist as little
as possible within my workflow. I'll try doing that.

I apologize to anyone I may have offended.

-bw

On Tue, Jan 7, 2020 at 6:26 PM Daniel Sanders <daniel_l_sanders at
apple.com>
wrote:
>
>
> > On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >
> > On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> > <cfe-dev at lists.llvm.org> wrote:
> >>
> >> 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 that.
> >>>
> >> It's not hyperbole, but fine.  How do you use it to keep
multiple,
> related changes in order?
> >
> > You can use parent/child revisions. Phabricator encourages a
> > patches-based approach with small changes. For me that corresponds to
> > one commit per code review. When I address code review feedback in a
> > parent revision I use git's interactive rebase.
>
> It's worth mentioning that Phabricator can read strings of the format
> 'Depends on D1234' from commit messages and create those
relationships for
> you.
>
> Also just because it's not easy to find unless you know it's there.
You
> can view the parent/child relationships in the 'Revision Contents'
section
> on the 'Stack' tab.
>
> >> The interface for reviewing and responding to reviews is horrific,
e.g.
> quoting text from a review is rather bad, the email it sends is badly
> formatted and hard to read. How do you make it reasonably useful?
> >
> > Inline comments are super useful, they can be marked as done and
> > hidden. I agree that sometimes there's a lot of context when
quoting
> > text, but the format is very simple (similar to e-mail) so it's
easy
> > to trim.
> >
> >> Why can't I *easily* relate changes to each other?
> >
> > What issues do you experience with parent/child revisions?
> >
> >> Why can't I submit through the Phabricator interface, but have
to go to
> the command line, place the change in a new branch, pull to top-of-tree,
> rebase, and only then push while hoping it doesn't give fail because
the
> tree became out of date? How can I do a rebase through Phabricator?
> >
> > You can upload patches through
> > https://reviews.llvm.org/differential/diff/create/. I personally
don't
> > use arcanist even though I found it pretty useful in the past.
> >
> >>
> >> These are only off the top of my head. There are far more problems
I've
> had with them.
> >>
> >> -bw
> >> _______________________________________________
> >> 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/20200107/643da04c/attachment.html>

I'm not sure a decision was already made as such. I think it's more that
there was a flurry of conversation last time with lots of conflicting opinions,
and then the conversation just fizzled out.

FWIW, I like Phabricator but I'm willing to try GitHub. Overall I think we
should take the same approach that eventually led to Phabricator being widely
adopted: We should allow GitHub PR's and see if the community generally
settles on one or the other.
> On Jan 7, 2020, at 19:13, Bill Wendling <isanbard at gmail.com>
wrote:
> 
> Thanks for the tip.
> 
> I didn't know that the decision had already been made to stay with
Phabricator. I find Phabricator hard to use and it makes me actively avoid
contributing to LLVM knowing that I'll have to use it. I've read the
documentation, but still have issues. And it's not like git where
there's an obscure command not everyone knows about that will fix the
problem. The advice I got so far has been to use Phabricator/Arcanist as little
as possible within my workflow. I'll try doing that.
> 
> I apologize to anyone I may have offended.
> 
> -bw
> 
> On Tue, Jan 7, 2020 at 6:26 PM Daniel Sanders <daniel_l_sanders at
apple.com <mailto:daniel_l_sanders at apple.com>> wrote:
> 
> 
> > On Jan 7, 2020, at 17:35, Jonas Devlieghere via llvm-dev <llvm-dev
at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> > 
> > On Tue, Jan 7, 2020 at 5:16 PM Bill Wendling via cfe-dev
> > <cfe-dev at lists.llvm.org <mailto:cfe-dev at
lists.llvm.org>> wrote:
> >> 
> >> On Tue, Jan 7, 2020 at 4:59 PM Doerfert, Johannes <jdoerfert at
anl.gov <mailto: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 that.
> >>> 
> >> It's not hyperbole, but fine.  How do you use it to keep
multiple, related changes in order?
> > 
> > You can use parent/child revisions. Phabricator encourages a
> > patches-based approach with small changes. For me that corresponds to
> > one commit per code review. When I address code review feedback in a
> > parent revision I use git's interactive rebase.
> 
> It's worth mentioning that Phabricator can read strings of the format
'Depends on D1234' from commit messages and create those relationships
for you.
> 
> Also just because it's not easy to find unless you know it's there.
You can view the parent/child relationships in the 'Revision Contents'
section on the 'Stack' tab.
> 
> >> The interface for reviewing and responding to reviews is horrific,
e.g. quoting text from a review is rather bad, the email it sends is badly
formatted and hard to read. How do you make it reasonably useful?
> > 
> > Inline comments are super useful, they can be marked as done and
> > hidden. I agree that sometimes there's a lot of context when
quoting
> > text, but the format is very simple (similar to e-mail) so it's
easy
> > to trim.
> > 
> >> Why can't I *easily* relate changes to each other?
> > 
> > What issues do you experience with parent/child revisions?
> > 
> >> Why can't I submit through the Phabricator interface, but have
to go to the command line, place the change in a new branch, pull to
top-of-tree, rebase, and only then push while hoping it doesn't give fail
because the tree became out of date? How can I do a rebase through Phabricator?
> > 
> > You can upload patches through
> > https://reviews.llvm.org/differential/diff/create/
<https://reviews.llvm.org/differential/diff/create/>. I personally
don't
> > use arcanist even though I found it pretty useful in the past.
> > 
> >> 
> >> These are only off the top of my head. There are far more problems
I've had with them.
> >> 
> >> -bw
> >> _______________________________________________
> >> cfe-dev mailing list
> >> cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
<https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
<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/20200107/a3c3f59e/attachment.html>

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.
Side note: I find that there's too many unknown features of Phab that
require manual annotations on the commit message to work.

I don't think that's a good strategy. New contributors get lost in the
specifics and reviewers forget to do them.
> Also just because it's not easy to find unless you know it's there.
You can view the parent/child relationships in the 'Revision Contents'
section on the 'Stack' tab.
The parental relationship in Phab is not obvious. I can't easily see
it in the snapshot and often ask people to link the commits in order
when they already are.

I'd also like to see the whole series without having to navigate the
linked list.

Git allows multiple commits per pull request, so does GitHub's PR UI,
as well as showing all the other changes (force push, rebase, reorder,
additional fixups), which makes it much simpler to see what changed.

Phab is better at keeping track of old comments, but where GitHub
completely looses the comment (on history change), Phab moves the
comment to a random place in the file, which is equally broken.

Granted, GitHub's UI is much "simpler" than Phab, but to my view,
this
is not a problem, but a benefit.

If we moved to GitHub PRs today, I wouldn't miss a thing.

cheers,
--renato

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.
>
> Side note: I find that there's too many unknown features of Phab that
> require manual annotations on the commit message to work.
>
> I don't think that's a good strategy. New contributors get lost in
the
> specifics and reviewers forget to do them.
>
> > Also just because it's not easy to find unless you know it's
there. You
> can view the parent/child relationships in the 'Revision Contents'
section
> on the 'Stack' tab.
>
> The parental relationship in Phab is not obvious. I can't easily see
> it in the snapshot and often ask people to link the commits in order
> when they already are.
>
> I'd also like to see the whole series without having to navigate the
> linked list.
>
> Git allows multiple commits per pull request, so does GitHub's PR UI,
> as well as showing all the other changes (force push, rebase, reorder,
> additional fixups), which makes it much simpler to see what changed.
>
Except the GitHub individual commit views are rather terrible for adding
comments to. The individual commits are not treated as separable commits
for approval purposes, and GitHub decides on its own that freshly added
comments are outdated due to surrounding noise.

>
> Phab is better at keeping track of old comments, but where GitHub
> completely looses the comment (on history change),
That aspect of GitHub makes longer running reviews quickly unusable. It
pretty much only works if all reviewers ensure that the PR originator
addresses all comments before they "scroll off".

> Phab moves the
> comment to a random place in the file, which is equally broken.
>
Much less broken IMO.

>
> Granted, GitHub's UI is much "simpler" than Phab, but to my
view, this
> is not a problem, but a benefit.
>
GitHub's UI is a screen real-estate hog due to low information density even
when collapsing things that you rather should see.

>
> If we moved to GitHub PRs today, I wouldn't miss a thing.
>
> cheers,
> --renato
> _______________________________________________
> 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/20200114/f36ba65a/attachment.html>

Renato Golin via llvm-dev <llvm-dev at lists.llvm.org> writes:
> Granted, GitHub's UI is much "simpler" than Phab, but to my
view, this
> is not a problem, but a benefit.
>
> If we moved to GitHub PRs today, I wouldn't miss a thing.
+1.

I still find Phab to be inscrutable.  I don't use any of its advanced
features.  I'm a long-time contributor.  I can't imagine how difficult
it is for folks new to the project.

For all of GitHub's many flaws, its very strong advantage is that it is
a de facto standard.  People understand it.

                   -David