Now that we’re on GitHub, can we please move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They’re super clunky and makes working in git that much worse.
-bw
Now that we're on GitHub, can we *please* move to GitHub PRs? As much as I hate git, I hate Phabricator/Archanist even more. They're super clunky and makes working in git that much worse.
FYI: We did have a thread on this a couple of months ago: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136579.html
-Hal
-bw
What was the verdict? Any plans to move? I hate coding anything knowing that I’ll have to use Phabricator. It’s like nails on a chalkboard.
-bw
What was the verdict? Any plans to move? I hate coding anything knowing that I'll have to use Phabricator. It's like nails on a chalkboard.
As you might imagine, not everyone agrees with you. My thoughts on how to move forward were here: http://lists.llvm.org/pipermail/llvm-dev/2019-November/136591.html - and I do think that we should move forward. It will take some work, however.
-Hal
-bw
Then perhaps those opposed could suggest how to use Phabricator/Arcanist so that I don’t throw my keyboard through my monitor?
-bw
Hi Bill,
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
+1
A list of arguments I extracted from previous discussions. I really hope
these points can be improved before we consider the migration.
* No diffs in mail - *super* inconvenient.
One has to open each pr in browser (or fetch via git etc etc)
(https://lists.llvm.org/pipermail/llvm-dev/2019-November/136583.html)
4) Phabricator's ability to see what has changed since the previous time you commented seems to be much more reliable than what Github provides.
(https://lists.llvm.org/pipermail/llvm-dev/2019-November/136589.html)
In contrast, I regularly find github decides not to load comments, or marks them as resolved before they are (and hides them). I do not wish the quality of LLVM’s codebase to be beholden to the choices github makes around which parts of the discussion to show me. And the issues with subscribing to github issues carry over to subscribing to github pull requests.
(https://lists.llvm.org/pipermail/llvm-dev/2019-November/136685.html)
The lack of a Herald replacement may be quite serious. There is no good
way to subscribe certain topics based on file paths or commit
descriptions.
(https://lists.llvm.org/pipermail/llvm-dev/2019-November/136608.html)
Hi Bill,
> 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
Login. I personally don't
use arcanist even though I found it pretty useful in the past.
Hi Bill,
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
$ arc diff --update --head
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
Hi Bill,
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.
>
>>
>> Hi Bill,
>>
>> > 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
Login. 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.
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
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.
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.
+1
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 believe that technically sending patches to the mailing list is
still a valid way to get your code reviewed. Not everyone monitors the
mailing list actively though so that might turn out to be more
frustrating than Phabricator.
This means that people proposing patches control the apparent behaviour. How is someone that is primarily a reviewer meant to voice their opinion under such a system?
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.
Sounds good to me as long as there is a way to contribute that doesn’t require a GitHub account, since there are people who can’t/won’t use GitHub for various reasons.
Jacob
When Phabricator was being introduced there were a few groups of reviewers who would ask for Phabricator patches to be re-sent by email and a few who would ask for emails to re-posted on Phabricator. To get my code reviewed I’d go along with whatever the reviewers in the area I wanted to change preferred. Over time, more reviewers asked for Phabricator and fewer asked for email.
>
>>
>> >
>> >>
>> >> Hi Bill,
>> >>
>> >> > 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 ⚙ D72025 [PM][CGSCC] Add a helper to update the call graph from SCC passes
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
>> Login. 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.
I would disagree with your characterisation that Phabricator is a superior tool:
Phabricator implements a large superset of the features that most users want. This extra complexity provides a barrier to entry for a lot of users (in fact, this morning I had a colleague drop into my office for help having done something wrong with Phabricator and mangled the diff, and I was unable to help him).
GitHub PRs implement a subset of the functionality that some people want. This causes a problem for people wanting who rely on the other features, most commonly reviewers.
Phabricator has the same problem as git: the learning curve is steep and there are always new things to learn. It also has the same advantage as git: it probably can do anything that you want it to do, as long as you're willing to invest the time to learn.
Favouring Phabricator over GitHub PRs is a decision to prioritise ease of use for some reviewers' workflows over that of patch contributors. That's fine, if it's a conscious decision (and not even one that I'd necessarily disagree with: code reviewers are probably the most scarce resource in the LLVM ecosystem and I'd be willing to lose some potential contributors if it increased the likelihood of timely and thorough code review), but it is misleading to claim that one tool is 'superior' in the abstract, without defining the requirements.
David