Let me give you (and others) a bit food for thoughts on this topic:
https://gregoryszorc.com/blog/2020/01/07/problems-with-pull-requests-and-how-to-fix-them/
I think it covers a good chunk of the pros and cons of using GH PRs vs
Phabricator (or alternatives to Phabricator).
Joerg
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
Hi,
as a constructive addition to this topic I picked one thing from the long list of requests that seemed to pop up a lot and contacted the Github Support about this:
Diffs in emails.
To be fair, I'm not used to reading diffs in emails a lot but I hear that a lot of people here and also inside of Red Hat are doing so and always look for that feature in Github, Gerrit, Patchworks, or whatever tool they're using.
I hope nobody feels offended by this step to contact Github support but what else to do than to forward a request from the community to the one's that can implement it? Let's try to remain constructive.
Cheers
Konrad
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
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
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.
Renato Golin via llvm-dev <llvm-dev@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
Renato Golin via llvm-dev <llvm-dev@lists.llvm.org> writes:
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.
This brings something else to mind for me. Another advantage of GitHub
is that there are many tools out there that work with it. Not so much
for Phab. As an Emacs user, getting access to magit/forge's ability to
interact with GitHub PRs would be amazing.
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".
Absolutely. Reviewing a patch series has always seemed a bit counter to
LLVM's development philosophy of small, incremental changes. I
understand the need to provide context for large changes (I have posted
mega-patches in Phab for that purpose), but the path to get to the large
change has always been small, incremental changes.
We have used one-commit-per-PR in our downstream for quite some time now
and it works well.
-David
> 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.
+1
Having used both Phab and GitHub for doing a lot of code reviews, I
*strongly* prefer the Phab workflow and Ux. It's not that Phab doesn't
have its issues, it's that the issues with GitHub are so much worse in
my experience.
~Aaron
Renato Golin via llvm-dev <llvm-dev@lists.llvm.org> writes:
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.This brings something else to mind for me. Another advantage of GitHub
is that there are many tools out there that work with it. Not so much
for Phab. As an Emacs user, getting access to magit/forge’s ability to
interact with GitHub PRs would be amazing.
This is a reason I proposed using PR when we were looking to merge MLIR: GitHub has an ecosystem of third-party tooling, for example we were relying on this service: https://codecov.io/gh/tensorflow/mlir which provide incremental code coverage annotation in GitHub pull-request.
That said there has been investment in pre-merge testing on our Phabricator instance in the meantime, so I’m fairly happy about the improvement there right now.
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”.Absolutely. Reviewing a patch series has always seemed a bit counter to
LLVM’s development philosophy of small, incremental changes. I
understand the need to provide context for large changes (I have posted
mega-patches in Phab for that purpose), but the path to get to the large
change has always been small, incremental changes.
There is a counter point to this: when folks doing a change needs a refactoring, they are more likely to submit both in a single review in a workflow that does not support patch series.
Even though I proposed to use GitHub PR a couple of months ago, the code review experience seems not as good as Phabricator to me on some aspects (on some other aspects, GitHub has been improving beyond our version of Phabricator though, like proposing inline changes when reviewing).
Best,
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.
True, but that's not a bad thing. As I said earlier, I don't want to
approve some commits and not others without creating a whole new
series. I think that would be similarly clumsy on either.
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".
Agree.
GitHub's UI is a screen real-estate hog due to low information density even when collapsing things that you rather should see.
With Phab I spent most of the time scrolling to find the comment box,
or the link to reopen the history because search on the comments
doesn't work.
But I don't want to drag down the details of each flaw in each tool.
What we need to do is to weigh in the major pros and cons.
GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.
Phab has a very complex UI with a lot of features that people have
come to rely over the years. But it's far too complex for new people
and requires specially crafted tooling to work with.
Regardless of the shortcomings of both, I think those points speak
strongly for using GitHub PRs. We can adapt to a new simplified
process, and even build tools around, that will be used by other
projects. Win Win.
cheers,
--renato
GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.
Just like with the "Everyone knows git", I call bullshit on this. Sorry,
but the far majority of GitHub users know little more than the most barebone
functionality of Pull Requests and the typical use case in most projects
is to squash all changes. But at this point it seems clear that the real
goal is to just move everything to GitHub just for the sake of it.
Phab has a very complex UI with a lot of features that people have
come to rely over the years. But it's far too complex for new people
and requires specially crafted tooling to work with.
Can you at least try to make reasonable arguments without ridiculous
hyperbole? Using phabricator for the casual user requires no special
tooling. Patches can be easily submitted via patch upload. There are
better ways to integrate it into a workflow, arcanist and git-phab just
to mention two.
Joerg
GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.Just like with the "Everyone knows git", I call bullshit on this. Sorry,
but the far majority of GitHub users know little more than the most barebone
functionality of Pull Requests and the typical use case in most projects
is to squash all changes. But at this point it seems clear that the real
goal is to just move everything to GitHub just for the sake of it.
Agree with Joerg here. And thanks for the recommendation Gregory Szorc's Digital Home | Problems with Pull Requests and How to Fix Them It was a great reading!
We should find justifying reasons to migrate (it has a significant cost
for people used to the current system), not just for the sake of using
GitHub for everything.
Konrad Kleine's action is appreciated (contacted the Github Support
about a deficiency).
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
> 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!
Renato Golin via llvm-dev <llvm-dev@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 asked a similar question in this thread in the very beginning: What
actual problems do you have with Phab? There might be usable solutions
out there already. The last time someone actually listed problems we got
a lot of good responses, some of which I will try out myself.
I can't imagine how difficult it is for folks new to the project.
I am always in favor of improving the documentation. We need more
concrete problem descriptions though.
For all of GitHub's many flaws, its very strong advantage is that it is
a de facto standard. People understand it.
I do not. Arguably because I have not yet used it. However, "it is a de
facto standard" is a weird argument for anything. People are advocating
to move away from mailing lists towards other system though mailing
lists are, or at least were, "de facto standard". Is the idea to keep up
with the "de facto standard" or to improve the status quo (for group X*)?
* Substitute X as you see fit.
Cheers,
Johannes
> I still find Phab to be inscrutable. I don't use any of its advanced
> features. I'm a long-time contributor.I asked a similar question in this thread in the very beginning: What
actual problems do you have with Phab? There might be usable solutions
out there already. The last time someone actually listed problems we got
a lot of good responses, some of which I will try out myself.
This thread has fallen down to the following pattern:
1. I tell you what I don't like / can't stand
2. You tell me that's not a problem for you and why
3. You ask me to counter your argument
This is not a helpful way to conduct a fact checking exercise.
I respect the opinion of both sides, and I know some people have
gotten to like Phab and others to hate.
I ask that people refrain from attacking others for not engaging in
tit-for-tat "my fact is better than yours" discussion.
Phab is good for some things, Github is good for others. People are
allowed to like either.
I am always in favor of improving the documentation. We need more
concrete problem descriptions though.
More documentation or tooling won't fix the fact that much more people
know about GitHub PR than Phab.
It's the same reason why we moved to monorepo GitHub, because everyone
had their own tooling to handle multi-repo Git-SVN hybrid.
If we change the process to be more in tune with GitHub, then their PR
system will (obviously) be far more suitable.
What I'm asking is that we review both together. Current process with
Phab versus a GitHub process with GitHub PR.
> For all of GitHub's many flaws, its very strong advantage is that it is
> a de facto standard. People understand it.I do not. Arguably because I have not yet used it.
He said "most people". He is right, even if you don't, personally. Git
PR (GitHub, GitLab, Gerrit) is indeed the de facto standard.
However, "it is a de facto standard" is a weird argument for anything. People are advocating
to move away from mailing lists towards other system though mailing
lists are, or at least were, "de facto standard". Is the idea to keep up
with the "de facto standard" or to improve the status quo (for group X*)?
That's the very definition of "de facto". The vast majority of people
use Git, and of those, GitHub/GitLab, and of those, Git PRs.
Phab is niche compared to GitHub. It doesn't make it worse, but that is a fact.
I recently had the task of explaining to some other people in my organization how to submit their changes using GitHub's pull request model. That experience indicates to me that this model is far from intuitive and easy for people to understand, especially if one is not terribly strong in git. I would concur that a decent fraction of people, probably a majority, are using magic git incantations to get their work done here.
Is the Phabricator process any better in this regard, though? I can't say for certain. For most of the history of open source projects, contribution has generally progressed by means of the "get a diff of your change and email/upload it somewhere" (and note that Phabricator is a variant on the "upload it somewhere" portion of the spectrum). Contribution in this manner is still the norm for several large, complex open-source projects, such as Linux, Firefox, GCC, or OpenJDK. Indeed, looking at our peer projects (those that are of similar complexity and scale, or also tackle compiler-related problems), contribution via a pull request model appears to be accepted by only a small few.
My experience of using GitHub PRs in the past is that different projects have different expectations for how contributors should update their changes during the review process. This makes me skeptical that even moving to GitHub PRs would meaningfully reduce the friction for new contributors, instead changing what and where the friction lands. So, to me, it seems hard to justify moving to GitHub PRs.
Joerg Sonnenberger via llvm-dev <llvm-dev@lists.llvm.org> writes:
GitHub PR is the "de facto standard", everyone knows, the entry cost
is practically zero. The UI is lean and missing features, but the
large availability of tooling (either targeting GitHub directly or
plain git) makes up for a lot of it.Just like with the "Everyone knows git", I call bullshit on this. Sorry,
but the far majority of GitHub users know little more than the most barebone
functionality of Pull Requests and the typical use case in most projects
is to squash all changes. But at this point it seems clear that the real
goal is to just move everything to GitHub just for the sake of it.
Let's try to assume good faith on the part of all parties.
Phab has a very complex UI with a lot of features that people have
come to rely over the years. But it's far too complex for new people
and requires specially crafted tooling to work with.Can you at least try to make reasonable arguments without ridiculous
hyperbole? Using phabricator for the casual user requires no special
tooling. Patches can be easily submitted via patch upload. There are
better ways to integrate it into a workflow, arcanist and git-phab just
to mention two.
I for one cannot get arcanist to work because of its reliance on PHP
which for whatever reason refuses to install on my machine. It it's
hard for me to do a simple install of a tool for code review, I can't
imagine the frustration of new people trying to get everything working.
I wasn't aware of git-phab and don't recall seeing it mentioned in the
LLVM documentation. I just checked and there is no mention of it. I'm
not sure how new people are supposed to know about it. It does look
nice from a cursory review of its front page. I'll give it a try!
I have never used Phab's more complex features. There's nothing in the
LLVM documentation that talks about patch series or patch parents or
anything more advanced than "submit a patch and pick reviewers." For
many people that is enough. But if I were to run into a situation where
one of the reviewers asked to link my change to some other change, o
present things as a patch series I would have no idea how to do that.
If I were new to the project I would just give up at that point.
There's this bit in the documentation:
Phabricator has many useful features, for example allowing you to
select diffs between different versions of the patch as it was
reviewed in the Revision Update History. Most features are self
descriptive - explore, and if you have a question, drop by on LLVM Project in
IRC to get help.
As has been noted in other threads, new users find IRC intimidating. I
haven't used IRC since I don't know when. I'm not even sure I *can*
given company policy. It's another hurdle to getting things done. And
important features are not "self descriptive" (see below).
The Phab instance has very little documentation and what there is is
difficult to find (I have to go to "More Applications" and click
"Guides" under "Utilities" -- really, does this organization make any
sense at all?). And then the guides seem limited to configuration, not
actual use-cases.
"More Applications" looks very intimidating with nonsensical names for
things I might want to do ("Harbormaster?" "Drydock?").
GitHub uses Markdown, which is another de facto standard. Phab uses
"Remarkup" which I guess is similar but different.
The front-page of Phab makes little sense to a new user. What is
"Differential?" What is "Diffusion?" Do they both have something to do
with diffs? Oh wait, "Diffusion" brings me to a list of "Active
Repositories." What?
Ok, let's try "Differential." Looks promising. There's a list of "LLVM
Needs Review." Looks like a code review tool. But I just want to
create a patch and submit it. Grumble, grumble, look back at the LLVM
documentation. Oh, it's the tiny inconspicuous button on the very top
right corner "+ Create Diff." The documentation told me about the
button but not where it's located. Took me some time to find it.
The above journey to get a patch submitted was my actual first
experience with Phab. It did not leave a good taste in my mouth.
-David
Fangrui Song via llvm-dev <llvm-dev@lists.llvm.org> writes:
Agree with Joerg here. And thanks for the recommendation
Gregory Szorc's Digital Home | Problems with Pull Requests and How to Fix Them
It was a great reading!
It's a good read (I did not get through all of it yet) but I lost of bit
of interest when he talks about reviewing pull requests with multiple
commits. Some of his arguments come down to, "people don't submit clean
histories for merging" which is going to be a problem for any review
tool. It isn't limited to GitHub.
I do agree that pull requests should group review comments by commit. I
certainly don't claim that GitHub is perfect.
I can't really speak to the scalability arguments against the pull
request model other than to say that we haven't had any issues
downstream with our repositories.
We should find justifying reasons to migrate (it has a significant
cost for people used to the current system), not just for the sake of
using GitHub for everything.
People have posted multiple reasons so it's not fair to say that people
are pushing the move just for the sake of using GitHub. It is true that
current users of Phab would have to change workflows. That's the nature
of change. If the benefit is moving to something more universally
understood that is much easier and welcoming for new contributors, I am
all for it.
-David