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

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? 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?
Why can't I *easily* relate changes to each other? 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?

These are only off the top of my head. There are far more problems I've had
with them.

-bw
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200107/41c39cd8/attachment-0001.html>

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

On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <jonas at
devlieghere.com>
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.
>
> What's your workflow for doing this? My current workflow is:
$ vim ...
$ git commit -a -m f
$ git rebase -i
<move the commit I want to upload to the top of the change list>
$ arc diff --update <update ID> --head <sha1 of the commit>

Of course, that doesn't work if the patch relies upon other patches.
However, if I keep the patches in order in git, then arc will upload all of
the parent changes to that Phab revision.
> 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.
>
> FWIW, the inline comments are fine, but I don't see a way to quote text
easily.
> Why can't I *easily* relate changes to each other?
>
> What issues do you experience with parent/child revisions?
>
> It was difficult to find out how to do it. I would expect it to have
suggested revisions in the list, but it didn't. It would also be nice to do
it through the "arc" tool, but there's no option to do that.

> > 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.
>
This is part of my issue. The arcanist tool is meant to be used with
Phabricator, especially if you develop in a terminal, but people seem to
actively avoid using it. I personally find it far too easy to push the
wrong changes to a revision.

-bw
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200107/2492a67f/attachment.html>

> 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

On Wed, Jan 8, 2020 at 2:16 AM Bill Wendling via llvm-dev
<llvm-dev at lists.llvm.org> wrote:
> How do you use [Phabricator] to keep multiple, related changes in order?
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
commits in a pull request. And unlike Phabricator, GitHub does not
allow you to "stack" pull requests.

When I work with a series of changes, I use Phabricator's stack like
several others here mentioned, plus the little trick of:

$ git rebase -i master -x "arc diff @^"

to update the Phabricator reviews.

I then still have to manually edit the rebase script (to remove the
arc diff @^ line for those commits that did not actually change).
However, it does work reasonably well.

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

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
> commits in a pull request. And unlike Phabricator, GitHub does not
> allow you to "stack" pull requests.
That is true, but the stacking in Phab is less than obvious, and I
always ask people to add "[N/M]" in the series' titles anyway.

Honestly, the fact that I have to either user Arcanist or edit the
comments metadata by hand or use the UI to do a simple task is asking
a bit too much.

We rarely approve some patches and not others in a series, and when we
do, we ask people to create a new series without the approved patch,
or split them, so that we can continue reviewing the series.

Once the approved patches are committed, the series has already
changed and the representation in Phab is not always true anymore.

I think we need to move from "how apt is GitHub's PR UI in emulating
what Phab does" to "what do we really need from a review UI and how
simple it is the process for both contributors and reviewers".

My honest opinion, after using Phab for too many years, is that it is
too much hassle to both sides. Git (and GitHub) PR has the benefit
that most developers out there use it already, including LLVM
developers contributing to other projects.

The name of the arcane tool we have to use to automate our cul-de-sac
review technology is just the cherry on the cake. :)

--renato

On 1/8/20 2:15 AM, Bill Wendling via cfe-dev wrote:
> It's not hyperbole, but fine. How do you use it to keep multiple, 
> related changes in order? 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? Why can't I *easily* relate changes to each other? 
> 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?
FWIW, Mozilla moved to Phabricator not long ago and the revision stack 
thing was a huge annoyance.

Nowadays we have tools to manage the stacks for us [1][2], and that as a 
plus don't require arc/php to be installed on your system.

I don't do much LLVM / clang stuff, but submitting stuff with [1] just 
works (with `moz-phab HEAD~N --no-bug`), and it should submit your last 
N commits separately automatically, without having to submit them one-by 
one and linking them via the web interface / annotate stuff in the 
commit message.

Sorry if this is just noise, though maybe it helps.

Cheers,

  -- Emilio

[1]: https://github.com/mozilla-conduit/review
[2]: https://github.com/mystor/phlay

On 01/15, Emilio Cobos Álvarez wrote:
> On 1/8/20 2:15 AM, Bill Wendling via cfe-dev wrote:
> > It's not hyperbole, but fine. How do you use it to keep multiple,
> > related changes in order? 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? Why can't I *easily* relate changes to each
other?
> > 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?
> FWIW, Mozilla moved to Phabricator not long ago and the revision stack
thing
> was a huge annoyance.
> 
> Nowadays we have tools to manage the stacks for us [1][2], and that as a
> plus don't require arc/php to be installed on your system.
> 
> I don't do much LLVM / clang stuff, but submitting stuff with [1] just
works
> (with `moz-phab HEAD~N --no-bug`), and it should submit your last N commits
> separately automatically, without having to submit them one-by one and
> linking them via the web interface / annotate stuff in the commit message.
> 
> Sorry if this is just noise, though maybe it helps.
This looks pretty cool, thanks! I'll for sure give it a try!


> 
> [1]: https://github.com/mozilla-conduit/review
> [2]: https://github.com/mystor/phlay
-- 

Johannes Doerfert
Researcher

Argonne National Laboratory
Lemont, IL 60439, USA

jdoerfert at anl.gov
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200115/710009b0/attachment.sig>