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

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 Tue, Jan 7, 2020 at 5:56 PM Bill Wendling <isanbard at gmail.com>
wrote:
>
> 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.
My workflow is similar, although I upload the patches manually through
the web interface. If it's a single patch I use an alias for `show
HEAD -U999999 ` which I then copy/paste in Phabricator. If I'm working
on a say a set of 3 changes and I need to update the oldest one, I use
git rebase -i HEAD~3 on the feature branch, edit the last commit and
use `git show -U999999 <sha>`. Most of the time I don't bother
updating the child revisions unless it really matters. I land the
changes from the commits (by cherry-picking them to master) rather
than the phabricator diffs and then rebase the feature branch.
>> > 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.
Do you mean quoting text in inline comments? Non-inline comments you
can quote by using the dropdown at the top right of a comment and
selecting "Quote Comment" which just puts the plaintext in the form.
Like I said earlier I usually trim the history to keep things on
point.
>> > 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.
>
I'm not sure how many open revisions you have, but the Edit Child
Revision window is pretty easy to use imho. If you upload the patches
one by one, the most recent one is always at the top. I also really
like the open stack view. It's much more explicit than mentioning one
PR in another.
>>
>> > 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.
Like I said I don't use arcanist. I think it works great if you have a
one-off patch and know how to spell the name of your reviewers but
otherwise I prefer the web interface.
> -bw

On 01/07, Jonas Devlieghere wrote:
> On Tue, Jan 7, 2020 at 5:56 PM Bill Wendling <isanbard at gmail.com>
wrote:
> >
> > 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.
> 
> My workflow is similar, although I upload the patches manually through
> the web interface. If it's a single patch I use an alias for `show
> HEAD -U999999 ` which I then copy/paste in Phabricator. If I'm working
> on a say a set of 3 changes and I need to update the oldest one, I use
> git rebase -i HEAD~3 on the feature branch, edit the last commit and
> use `git show -U999999 <sha>`. Most of the time I don't bother
> updating the child revisions unless it really matters. I land the
> changes from the commits (by cherry-picking them to master) rather
> than the phabricator diffs and then rebase the feature branch.
My workflow is similar but with arcanist. Most of the time I do an
interactive rebase and then `arc diff HEAD~`.
> >> > 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.
> 
> Do you mean quoting text in inline comments? Non-inline comments you
> can quote by using the dropdown at the top right of a comment and
> selecting "Quote Comment" which just puts the plaintext in the
form.
> Like I said earlier I usually trim the history to keep things on
> point.
Inline comments can be "quoted" by copy+paste and putting them after a
`>`

> >> > 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.
> >
> I'm not sure how many open revisions you have, but the Edit Child
> Revision window is pretty easy to use imho. If you upload the patches
> one by one, the most recent one is always at the top. I also really
> like the open stack view. It's much more explicit than mentioning one
> PR in another.
I have quite a few *active* open patches and interactions with patches
of other people in different parts of the compiler. The child/parent
thing needs to be clicked on once per patch manually but it has good
default suggestions and a search that works.

Click on the "Stack" tab on a patch like
https://reviews.llvm.org/D72025
to see how multiple (>3) people managed to connect the patches in their
dependence order.

> >> > 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.
> 
> Like I said I don't use arcanist. I think it works great if you have a
> one-off patch and know how to spell the name of your reviewers but
> otherwise I prefer the web interface.
I use arcanist all the time. While it has a lot of flaws it works for me
mostly fine. I like that I don't need to go to the web interface all the
time and while I use the browser to lookup usernames of people, it will
tell me if I misspelled one. Also, I can add reviewers later as well.
Usually I just look at the last commit if I create a new one to see what
reviewers I had there.
-------------- 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/20200108/a1b49404/attachment.sig>

This is very belated (and I don’t want the revive the entire discussion), but
https://reviews.llvm.org/D74990 will make arc only upload your current patch by
default (instead of the entire series).

From: cfe-dev <cfe-dev-bounces at lists.llvm.org> on behalf of cfe-dev
<cfe-dev at lists.llvm.org>
Reply-To: Bill Wendling <isanbard at gmail.com>
Date: Tuesday, January 7, 2020 at 5:57 PM
To: Jonas Devlieghere <jonas at devlieghere.com>
Cc: llvm-dev <llvm-dev at lists.llvm.org>, cfe-dev <cfe-dev at
lists.llvm.org>
Subject: Re: [cfe-dev] [llvm-dev] Phabricator -> GitHub PRs?

On Tue, Jan 7, 2020 at 5:35 PM Jonas Devlieghere <jonas at
devlieghere.com<mailto: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<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.
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/<https://urldefense.proofpoint.com/v2/url?u=https-3A__reviews.llvm.org_differential_diff_create_&d=DwMFaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=o3kDXzdBUE3ljQXKeTWOMw&m=frqwLflsBTC4S7ZBA1YhH2N0m5t_d6SbSsjn0E1b2_E&s=tx6sJfZLYG_PvgKOyxpAU160F32vryX4U1MkXuO1zVw&e=>.
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/20200221/19611319/attachment.html>